| Message ID | 66135e57d38599c0dfce347643858558c4f026c4.1297594674.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Sun, 13 Feb 2011 12:11:45 +0100 (CET) > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> This is not appropriate. Now, every driver that lacks SG support will spit out that warning message in netdev_fix_features(). That's why the check is there conditionalizing NETIF_F_GSO on NETIF_F_SG in register_netdevice(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 13, 2011 at 10:50:23AM -0800, David Miller wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Date: Sun, 13 Feb 2011 12:11:45 +0100 (CET) > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > This is not appropriate. > > Now, every driver that lacks SG support will spit out that warning > message in netdev_fix_features(). > > That's why the check is there conditionalizing NETIF_F_GSO on > NETIF_F_SG in register_netdevice(). I think all those messages should be converted to DEBUG level. Those conditions are constant and can be better described in Documentation/networking/ or ethtool manpage. Preferably along the device-specific conditions when implemented. Or I could just drop the message for the GSO case as it's something new here anyway (I added it to make it consistent with handling of other features). Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: Tue, 15 Feb 2011 22:46:49 +0100 > On Sun, Feb 13, 2011 at 10:50:23AM -0800, David Miller wrote: >> From: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> Date: Sun, 13 Feb 2011 12:11:45 +0100 (CET) >> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> >> This is not appropriate. >> >> Now, every driver that lacks SG support will spit out that warning >> message in netdev_fix_features(). >> >> That's why the check is there conditionalizing NETIF_F_GSO on >> NETIF_F_SG in register_netdevice(). > > I think all those messages should be converted to DEBUG level. Those > conditions are constant and can be better described in > Documentation/networking/ or ethtool manpage. Preferably along the > device-specific conditions when implemented. > > Or I could just drop the message for the GSO case as it's something new here > anyway (I added it to make it consistent with handling of other features). The messages exist to let driver authors know they've constructed an illegal set of feature bits. Since you're now adding the GSO bit yourself, you should perform due diligence and prevent the illegal combination yourself. This has no other impact on the other messages and cases, which definitely should stay intact. Your change is just wrong and knowingly introduces useless log messages, please just fix it up. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 15, 2011 at 02:00:43PM -0800, David Miller wrote: > From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > Date: Tue, 15 Feb 2011 22:46:49 +0100 > > On Sun, Feb 13, 2011 at 10:50:23AM -0800, David Miller wrote: > >> From: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> Date: Sun, 13 Feb 2011 12:11:45 +0100 (CET) > >> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > >> This is not appropriate. > >> > >> Now, every driver that lacks SG support will spit out that warning > >> message in netdev_fix_features(). > >> > >> That's why the check is there conditionalizing NETIF_F_GSO on > >> NETIF_F_SG in register_netdevice(). > > I think all those messages should be converted to DEBUG level. Those > > conditions are constant and can be better described in > > Documentation/networking/ or ethtool manpage. Preferably along the > > device-specific conditions when implemented. > > > > Or I could just drop the message for the GSO case as it's something new here > > anyway (I added it to make it consistent with handling of other features). > The messages exist to let driver authors know they've constructed an > illegal set of feature bits. Only multiple checksum flags are actual driver bugs - those are handled in register_netdevice(). netdev_fix_features() represents limitations of core networking code . Hardware might as well handle TSO without SG or SG without checksumming. What's the point in duplicating those restrictions in drivers? > Since you're now adding the GSO bit yourself, you should perform > due diligence and prevent the illegal combination yourself. > > This has no other impact on the other messages and cases, which > definitely should stay intact. > > Your change is just wrong and knowingly introduces useless log > messages, please just fix it up. The messages from netdev_fix_features() will also be triggered by user disabling required feature (ie. HW checksum for most of TX offloads) for a device or its slave or sometimes on MTU changes (ie. for jme and possibly others). Even after I fix triggering of "GSO needs SG" message in register_netdevice(), this and other messages will show up more often because of the way the new scheme works. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index c7d7074..45fb17f 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -972,6 +972,9 @@ struct net_device { NETIF_F_SG | NETIF_F_HIGHDMA | \ NETIF_F_FRAGLIST) + /* changeable features with no special hardware requirements */ +#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) + /* Interface index. Unique device identifier */ int ifindex; int iflink; diff --git a/net/core/dev.c b/net/core/dev.c index 6392ea0..bced624 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5248,6 +5248,12 @@ u32 netdev_fix_features(struct net_device *dev, u32 features) features &= ~NETIF_F_TSO; } + /* Software GSO depends on SG. */ + if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) { + netdev_info(dev, "Dropping NETIF_F_GSO since no SG feature.\n"); + features &= ~NETIF_F_GSO; + } + /* UFO needs SG and checksumming */ if (features & NETIF_F_UFO) { /* maybe split UFO into V4 and V6? */ @@ -5404,12 +5410,12 @@ int register_netdevice(struct net_device *dev) if (dev->iflink == -1) dev->iflink = dev->ifindex; + /* Enable software offloads by default - will be stripped in + * netdev_fix_features() if not supported. */ + dev->features |= NETIF_F_SOFT_FEATURES; + dev->features = netdev_fix_features(dev, dev->features); - /* Enable software GSO if SG is supported. */ - if (dev->features & NETIF_F_SG) - dev->features |= NETIF_F_GSO; - /* Enable GRO and NETIF_F_HIGHDMA for vlans by default, * vlan_dev_init() will do the dev->features check, so these features * are enabled only if supported by underlying device.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- include/linux/netdevice.h | 3 +++ net/core/dev.c | 14 ++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-)