| Message ID | 20210721215642.19866-2-LinoSanfilippo@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for KSZ DSA switch | expand |
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 <olteanv@gmail.com> > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > 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 <olteanv@gmail.com> >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> >> --- >> 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 <f.fainelli@gmail.com> -- Florian
> Agreed, with those fixed: > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> 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 <f.fainelli@gmail.com> > > 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 <f.fainelli@gmail.com> > > 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
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);
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 <olteanv@gmail.com> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- net/dsa/slave.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)