On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
> The function skb_put() that is used by tail taggers to make room for the
> DSA tag must only be called for linearized SKBS. However in case that the
> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
> SKB passed to the slaves transmit function may not be linearized.
> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
> for tail taggers.
> Furthermore since the tagging protocol can be changed at runtime move the
> code for setting up the slaves features into dsa_slave_setup_tagger().
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> net/dsa/slave.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 22ce11cd770e..ae2a648ed9be 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
> struct dsa_slave_priv *p = netdev_priv(slave);
> const struct dsa_port *cpu_dp = dp->cpu_dp;
> struct net_device *master = cpu_dp->master;
> + const struct dsa_switch *ds = dp->ds;
>
> slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
> slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
> slave->needed_tailroom += master->needed_tailroom;
>
> p->xmit = cpu_dp->tag_ops->xmit;
> +
> + slave->features = master->vlan_features | NETIF_F_HW_TC;
> + if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> + slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> + slave->hw_features |= NETIF_F_HW_TC;
> + slave->features |= NETIF_F_LLTX;
> + if (slave->needed_tailroom)
> + slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
> }
>
> static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
> if (slave_dev == NULL)
> return -ENOMEM;
>
> - slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
> - if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> - slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> - slave_dev->hw_features |= NETIF_F_HW_TC;
> - slave_dev->features |= NETIF_F_LLTX;
> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
> if (!is_zero_ether_addr(port->mac))
> ether_addr_copy(slave_dev->dev_addr, port->mac);
> --
> 2.32.0
>
I would have probably changed the code in dsa_slave_create just like
this:
- slave->features = master->vlan_features | NETIF_F_HW_TC;
+ slave->features = NETIF_F_HW_TC;
...
- slave_dev->vlan_features = master->vlan_features;
and in dsa_slave_setup_tagger:
+ vlan_features = master->vlan_features;
+ slave->features &= ~vlan_features;
+ if (slave->needed_tailroom)
+ vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
+ slave->features |= vlan_features;
+ slave->vlan_features = vlan_features;
no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?
And I would probably add:
Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")
On 7/21/2021 4:35 PM, Vladimir Oltean wrote:
> On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
>> The function skb_put() that is used by tail taggers to make room for the
>> DSA tag must only be called for linearized SKBS. However in case that the
>> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
>> SKB passed to the slaves transmit function may not be linearized.
>> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
>> for tail taggers.
>> Furthermore since the tagging protocol can be changed at runtime move the
>> code for setting up the slaves features into dsa_slave_setup_tagger().
>>
>> Suggested-by: Vladimir Oltean <[email protected]>
>> Signed-off-by: Lino Sanfilippo <[email protected]>
>> ---
>> net/dsa/slave.c | 14 +++++++++-----
>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 22ce11cd770e..ae2a648ed9be 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>> struct dsa_slave_priv *p = netdev_priv(slave);
>> const struct dsa_port *cpu_dp = dp->cpu_dp;
>> struct net_device *master = cpu_dp->master;
>> + const struct dsa_switch *ds = dp->ds;
>>
>> slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
>> slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
>> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>> slave->needed_tailroom += master->needed_tailroom;
>>
>> p->xmit = cpu_dp->tag_ops->xmit;
>> +
>> + slave->features = master->vlan_features | NETIF_F_HW_TC;
>> + if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> + slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> + slave->hw_features |= NETIF_F_HW_TC;
>> + slave->features |= NETIF_F_LLTX;
>> + if (slave->needed_tailroom)
>> + slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
>> }
>>
>> static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
>> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
>> if (slave_dev == NULL)
>> return -ENOMEM;
>>
>> - slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>> - if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> - slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> - slave_dev->hw_features |= NETIF_F_HW_TC;
>> - slave_dev->features |= NETIF_F_LLTX;
>> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>> if (!is_zero_ether_addr(port->mac))
>> ether_addr_copy(slave_dev->dev_addr, port->mac);
>> --
>> 2.32.0
>>
>
> I would have probably changed the code in dsa_slave_create just like
> this:
>
> - slave->features = master->vlan_features | NETIF_F_HW_TC;
> + slave->features = NETIF_F_HW_TC;
> ...
> - slave_dev->vlan_features = master->vlan_features;
>
> and in dsa_slave_setup_tagger:
>
> + vlan_features = master->vlan_features;
> + slave->features &= ~vlan_features;
> + if (slave->needed_tailroom)
> + vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
> + slave->features |= vlan_features;
> + slave->vlan_features = vlan_features;
>
> no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?
>
> And I would probably add:
>
> Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")
Agreed, with those fixed:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
> Agreed, with those fixed:
>
> Reviewed-by: Florian Fainelli <[email protected]>
Hi Florian, Vladimir
I would suggest stop adding Reviewed-by: when you actual want changes
made. The bot does not seem to be reading the actual emails, it just
looks for tags. And when there are sufficient tags, it merges,
independent of requests for change, open questions, etc.
Andrew
On 7/22/21 7:14 AM, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <[email protected]>
>
> Hi Florian, Vladimir
>
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.
Yes, I will definitively stop doing that. I did not think that the
merging was handled by a bot, but if it is, that makes me seriously
nervous about the whole process.
--
Florian
On 22.07.21 at 16:14, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <[email protected]>
>
> Hi Florian, Vladimir
>
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.
>
> Andrew
>
Hi,
since I got a message that the patches have already been applied to netdev/net.git.
How should I proceed if I want to send a new version of the series? Just ignore the
merge to netdev and send the patches nevertheless?
Regards,
Lino
On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
> since I got a message that the patches have already been applied to netdev/net.git.
> How should I proceed if I want to send a new version of the series? Just ignore the
> merge to netdev and send the patches nevertheless?
Since the git history is immutable you need to work with what is already
in the current net/master branch. What do you want to change, just
address the feedback I gave? If that is all, just don't bother, I intend
to look at adding a framework through which the DSA master can declare
what features it supports in conjunction with specific DSA tagging protocols.
That is material for net-next, and Dave took your patch at the last
minute for the "net" pull request towards Linus' tree. If you send
another patch on "net" in that area now, we'd have to wait for another
week or two until "net" will be merged again into "net-next". Not sure
if it's worth it. The only thing that was of concern to me is that you
assign the DSA interface's slave->vlan_features = master->vlan_features.
So even though you clear the NETIF_F_SG feature for the DSA slave
interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
However, those skbs will be linearized during the dev_queue_xmit call
done by the 8021q driver towards DSA, so in the end, the way in which
you restructured the code may not be cosmetically ideal, but also
appears to not be functionally problematic.
Anyway, your patch will probably conflict with the stable trees (the
tag_ops->needed_tailroom was introduced very recently), so we will have
another chance to fix it up when Greg sends the email that the patch
failed to apply.
Hi Vladimir,
On 23.07.21 at 14:22, Vladimir Oltean wrote:
> On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
>> since I got a message that the patches have already been applied to netdev/net.git.
>> How should I proceed if I want to send a new version of the series? Just ignore the
>> merge to netdev and send the patches nevertheless?
>
> Since the git history is immutable you need to work with what is already
> in the current net/master branch. What do you want to change, just
> address the feedback I gave? If that is all, just don't bother, I intend
> to look at adding a framework through which the DSA master can declare
> what features it supports in conjunction with specific DSA tagging protocols.
> That is material for net-next, and Dave took your patch at the last
> minute for the "net" pull request towards Linus' tree. If you send
> another patch on "net" in that area now, we'd have to wait for another
> week or two until "net" will be merged again into "net-next". Not sure
> if it's worth it. The only thing that was of concern to me is that you
> assign the DSA interface's slave->vlan_features = master->vlan_features.
> So even though you clear the NETIF_F_SG feature for the DSA slave
> interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
> However, those skbs will be linearized during the dev_queue_xmit call
> done by the 8021q driver towards DSA, so in the end, the way in which
> you restructured the code may not be cosmetically ideal, but also
> appears to not be functionally problematic.
> Anyway, your patch will probably conflict with the stable trees (the
> tag_ops->needed_tailroom was introduced very recently), so we will have
> another chance to fix it up when Greg sends the email that the patch
> failed to apply.
>
Yes, I just wanted to address your feedback concerning the feature assignment
in a new patch version. But as you explained this is not needed and would make
things just unnecessary complicated. So lets wait and see if there are any
conflicts with Gregs stable tree. Thanks for the explanation.
Best Regards,
Lino