[PATCH v3] realtek: fix tx checks

[Arinc UNAL (Xeront)]

On 13.06.2022 17:33, Sander Vanheule wrote:
> 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?

Oops, thanks for pointing that out! I don't know this driver very well 
(I'm relatively new at coding too). I made this off of Birger's 
suggestion. I'm going to need a directive to fix the patch with all the 
comments you've made.

Arınç