On 11/3/2020 2:51 AM, Vladimir Oltean wrote:
> On Mon, Nov 02, 2020 at 12:34:11PM -0800, Florian Fainelli wrote:
>> On 11/1/2020 11:16 AM, Vladimir Oltean wrote:
>>> Now that we have a central TX reallocation procedure that accounts for
>>> the tagger's needed headroom in a generic way, we can remove the
>>> skb_cow_head call.
>>>
>>> Cc: Florian Fainelli <f.faine...@gmail.com>
>>> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
>>
>> Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
>
> Florian, I just noticed that tag_brcm.c has an __skb_put_padto call,
> even though it is not a tail tagger. This comes from commit:
>
> commit bf08c34086d159edde5c54902dfa2caa4d9fbd8c
> Author: Florian Fainelli <f.faine...@gmail.com>
> Date: Wed Jan 3 22:13:00 2018 -0800
>
> net: dsa: Move padding into Broadcom tagger
>
> Instead of having the different master network device drivers
> potentially used by DSA/Broadcom tags, move the padding necessary for
> the switches to accept short packets where it makes most sense: within
> tag_brcm.c. This avoids multiplying the number of similar commits to
> e.g: bgmac, bcmsysport, etc.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> Signed-off-by: David S. Miller <da...@davemloft.net>
>
> Do you remember why this was needed?
Yes, it is explained in the comment surrounding the padding:
/* The Ethernet switch we are interfaced with needs packets to
be at
* least 64 bytes (including FCS) otherwise they will be
discarded when
* they enter the switch port logic. When Broadcom tags are
enabled, we
* need to make sure that packets are at least 68 bytes
* (including FCS and tag) because the length verification is
done after
* the Broadcom tag is stripped off the ingress packet.
> As far as I understand, either the DSA master driver or the MAC itself
> should pad frames automatically. Is that not happening on Broadcom SoCs,
> or why do you need to pad from DSA?
Some of the Ethernet MACs were not doing that automatic padding and/or
had no option to turn this on. This is true for at least SYSTEMPORT (not
Lite) and BGMAC. GENET is also commonly used but does support automatic
RUNT frame padding.
> How should we deal with this? Having tag_brcm.c still do some potential
> reallocation defeats the purpose of doing it centrally, in a way. I was
> trying to change the prototype of struct dsa_device_ops::xmit to stop
> returning a struct sk_buff *, and I stumbled upon this.
> Should we just go ahead and pad everything unconditionally in DSA?
>
This is really a problem specific to Broadcom tags and how the switch
operate so it seems reasonable to leave those details down to the tagger.
--
Florian
