[PATCH v3] realtek: fix tx checks

[Sander Vanheule]

Hi Arınç,

On Mon, 2022-06-13 at 14:25 +0000, Arinc UNAL (Xeront) wrote:
> There is a bug on the ethernet driver where the checks for the DSA tag and
> tag generation don't include port 0. This causes any frame egressing from
> the first port of the switch to have a VLAN 0 tag.
> 
> Fix this by extending the checks to include port 0.
> 
> RTL93xx tag generation functions rtl930x_decode_tag() and
> rtl931x_decode_tag() do not check for the destination port. Therefore, move
> the check for all 4 SoC families to be done directly in rtl838x_eth_tx().
> 
> Suggested-by: Birger Koblitz <mail at birger-koblitz.de>
> Signed-off-by: Arınç ÜNAL <arinc.unal at arinc9.com>
> ---
> 
> v3: Resend because patchwork didn't catch the patch properly. Sorry for the
> noise. My company uses Microsoft Exchange and I've yet to figure out how to
> configure git-send-email with it. Owl for Thunderbird left me hanging.
> Let's try sending over Outlook.
> 
> ---
>  .../drivers/net/ethernet/rtl838x_eth.c        | 59 +++++++++----------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/target/linux/realtek/files-
> 5.10/drivers/net/ethernet/rtl838x_eth.c b/target/linux/realtek/files-
> 5.10/drivers/net/ethernet/rtl838x_eth.c
> index cf6aabc614..f3b7c994c3 100644
> --- a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
> +++ b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c
> @@ -96,42 +96,38 @@ static void rtl838x_create_tx_header(struct p_hdr *h, int
> dest_port, int prio)
>  {
>         prio &= 0x7;
>  
> -       if (dest_port > 0) {
> -               // cpu_tag[0] is reserved on the RTL83XX SoCs
> -               h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0:
> L2LEARNING on
> -               h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM
> settings below
> -               h->cpu_tag[3] = 0x0000;
> -               h->cpu_tag[4] = BIT(dest_port) >> 16;
> -               h->cpu_tag[5] = BIT(dest_port) & 0xffff;
> -               // Set internal priority and AS_PRIO
> -               if (prio >= 0)
> -                       h->cpu_tag[2] |= (prio | 0x8) << 12;
> -       }
> +       // cpu_tag[0] is reserved on the RTL83XX SoCs
> +       h->cpu_tag[1] = 0x0401;  // BIT 10: RTL8380_CPU_TAG, BIT0: L2LEARNING
> on
> +       h->cpu_tag[2] = 0x0200;  // Set only AS_DPM, to enable DPM settings
> below
> +       h->cpu_tag[3] = 0x0000;
> +       h->cpu_tag[4] = BIT(dest_port) >> 16;
> +       h->cpu_tag[5] = BIT(dest_port) & 0xffff;
> +       // Set internal priority and AS_PRIO
> +       if (prio >= 0)
> +               h->cpu_tag[2] |= (prio | 0x8) << 12;

What meaning to negative dest_port (and prio) values have? From the code it
looks like neither dest_port, nor prio, should ever be negative. So if you're
dropping the value check entirely, these parameters should be unsigned ints. 

>  }
>  
>  static void rtl839x_create_tx_header(struct p_hdr *h, int dest_port, int
> prio)
>  {
>         prio &= 0x7;
>  
> -       if (dest_port > 0) {
> -               // cpu_tag[0] is reserved on the RTL83XX SoCs
> -               h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
> -               h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5]
> = 0;
> -               // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
> -               if (dest_port >= 32) {
> -                       dest_port -= 32;
> -                       h->cpu_tag[2] = BIT(dest_port) >> 16;
> -                       h->cpu_tag[3] = BIT(dest_port) & 0xffff;
> -               } else {
> -                       h->cpu_tag[4] = BIT(dest_port) >> 16;
> -                       h->cpu_tag[5] = BIT(dest_port) & 0xffff;
> -               }
> -               h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
> -               h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
> -               // Set internal priority and AS_PRIO
> -               if (prio >= 0)
> -                       h->cpu_tag[1] |= prio | BIT(3);
> +       // cpu_tag[0] is reserved on the RTL83XX SoCs
> +       h->cpu_tag[1] = 0x0100; // RTL8390_CPU_TAG marker
> +       h->cpu_tag[2] = h->cpu_tag[3] = h->cpu_tag[4] = h->cpu_tag[5] = 0;
> +       // h->cpu_tag[1] |= BIT(1) | BIT(0); // Bypass filter 1/2
> +       if (dest_port >= 32) {
> +               dest_port -= 32;
> +               h->cpu_tag[2] = BIT(dest_port) >> 16;
> +               h->cpu_tag[3] = BIT(dest_port) & 0xffff;
> +       } else {
> +               h->cpu_tag[4] = BIT(dest_port) >> 16;
> +               h->cpu_tag[5] = BIT(dest_port) & 0xffff;
>         }
> +       h->cpu_tag[2] |= BIT(5); // Enable destination port mask use
> +       h->cpu_tag[2] |= BIT(8); // Enable L2 Learning
> +       // Set internal priority and AS_PRIO
> +       if (prio >= 0)
> +               h->cpu_tag[1] |= prio | BIT(3);

Same issue as above.

>  }
>  
>  static void rtl930x_create_tx_header(struct p_hdr *h, int dest_port, int
> prio)
> @@ -1135,6 +1131,9 @@ static int rtl838x_eth_tx(struct sk_buff *skb, struct
> net_device *dev)
>         int dest_port = -1;
>         int q = skb_get_queue_mapping(skb) % TXRINGS;
>  
> +       if (dest_port >= 0)
> +               priv->r->create_tx_header(h, dest_port, skb->priority >> 1);
> +

dest_port was just initialised to -1 here, so this code will never run. Maybe
you actually wanted to get the TX port number from somewhere else?

Best,
Sander

>         if (q) // Check for high prio queue
>                 pr_debug("SKB priority: %d\n", skb->priority);
>  
> @@ -1142,7 +1141,7 @@ static int rtl838x_eth_tx(struct sk_buff *skb, struct
> net_device *dev)
>         len = skb->len;
>  
>         /* Check for DSA tagging at the end of the buffer */
> -       if (netdev_uses_dsa(dev) && skb->data[len-4] == 0x80 && skb->data[len-
> 3] > 0
> +       if (netdev_uses_dsa(dev) && skb->data[len-4] == 0x80 && skb->data[len-
> 3] >= 0
>                         && skb->data[len-3] < priv->cpu_port &&  skb-
> >data[len-2] == 0x10
>                         &&  skb->data[len-1] == 0x00) {
>                 /* Reuse tag space for CRC if possible */