[PATCH v3] realtek: fix tx checks

[Birger Koblitz]

Hi,

dest_port -1 means flood all ports with a broadcast packet in the tx routine,
the tag functions can only be called with dest_port >= 0, however.
In the case of broadcast packets there should not be a CPU-tag with a destination port,
or all bits in the destination port mask would need to be set.
So within the tx routine dest_port < 0 is OK, but the tag routines should not
be called with such a value currently, since for RTL93xx the >= 0 check was missing.
For RTL93xx there is currently a bug for dest_port < 0 here, which does however
not seem to have any obvious negative impact.

In principle for RTL83xx dest_port < 0 could be provided, in which case AS_DPM
(assert Destination Port Mask) should not be set and dest_port would be ignored,
meaning the packet is either broadcast or is making use of the Forwarding DB.
Alternatively all destination port bits could be set and AS_DPM set.

For RTL93xx it is not clear how this works as the AS_DPM bit is missing and
there is a different mechanism which involves stacked devices.

The destination port is read from the DSA tag at the beginning of the tx routine:

	/* Check for DSA tagging at the end of the buffer */
	if (netdev_uses_dsa(dev) && skb->data[len-4] == 0x80
		&& skb->data[len-3] < priv->cpu_port && skb->data[len-2] == 0x10
		&& skb->data[len-1] == 0x00) {
		/* Reuse tag space for CRC if possible */
		dest_port = skb->data[len-3];   <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
		skb->data[len-4] = skb->data[len-3] = skb->data[len-2] = skb->data[len-1] = 0x00;
		len -= 4;
	}

The tx routines can be called with a priority < 0 in which case the default priority for
the CPU-port and that type of packet would be used. All SoC generations accept
a bit (AS_PRIO for assert priority) that if not set means the priority is ignored
and the default is applied. The tx routine currently does not make use of this
feature, it always calls the header create routines with prio >= 0.

Cheers,
  Birger


On 13.06.22 17:08, Arinc UNAL (Xeront) wrote:
>>>   
>>>   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ç