login
Search
This is a second attempt at defining a unified offload-setting interface
for network drivers. The changes are influenced by comments on the original
proposal and deeper look into what drivers do.
The idea:
Introduce two new fields to struct net_device to hold toggable and
user-requested feature sets. Offload features can be in:
hw_features
features user can toggle (e.g., via ethtool)
wanted_features
features requested by user - his/her wish if you prefer
(subset of .hw_features + software-only features)
features
features currently active; if some offloads depend
on other conditions, the set in .features & .hw_features
might be not equal to .wanted_features & .hw_features
To check and transfer offload settings from .wanted_features to
.features, there are two new hooks in struct net_device_ops:
features = ndo_fix_features(netdev, features);
modifies passed feature set according to device-specific
conditions
err = ndo_set_features(netdev, features);
should reconfigure the hardware for a new feature set
Core code:
netdev_update_features(netdev);
should be called after feature conditions changed, i.e.
after MTU change, or slave device reconfiguration
features = netdev_get_wanted_features(netdev);
convenience function: replaces bits from .hw_features
(plus software-only features) in .features with .wanted_features
Other things added:
- a compatibility layer for old ethtool ops for drivers converted
to new offload setting API
- transitional calls to old ethtool_ops for not-yet-converted drivers
- a new flag for RX checksumming to replace driver-private fields/flags
- request software offloads (GSO and GRO) by default
- example conversions of a few drivers
Things to do after:
- convert all drivers
- remove redundant offload state in drivers
- remove old ethtool_ops hooks (set_tso, set_flags, set_hw_csum, etc.)
This is only build-tested on x86 allyesconfig, so probably not for general
use, yet.
Best ...Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
net/core/dev.c | 32 +++++++++++++++++---------------
1 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index f937726..6823275 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5020,6 +5020,23 @@ static void rollback_registered(struct net_device *dev)
unsigned long netdev_fix_features(unsigned long features, const char *name)
{
+ /* Fix illegal checksum combinations */
+ if ((features & NETIF_F_HW_CSUM) &&
+ (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+ if (name)
+ printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
+ name);
+ features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+ }
+
+ if ((features & NETIF_F_NO_CSUM) &&
+ (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+ if (name)
+ printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
+ name);
+ features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
+ }
+
/* Fix illegal SG+CSUM combinations. */
if ((features & NETIF_F_SG) &&
!(features & NETIF_F_ALL_CSUM)) {
@@ -5195,21 +5212,6 @@ int register_netdevice(struct net_device *dev)
if (dev->iflink == -1)
dev->iflink = dev->ifindex;
- /* Fix illegal checksum combinations */
- if ((dev->features & NETIF_F_HW_CSUM) &&
- (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
- printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
- dev->name);
- dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
- }
-
- if ((dev->features & NETIF_F_NO_CSUM) &&
- (dev->features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
- printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
- dev->name);
- dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
- }
-
dev->features = netdev_fix_features(dev->features, dev->name);
/* Enable software GSO if SG is supported. */
-- ...Private rx_csum flags are now duplicate of netdev->features & NETIF_F_RXCSUM.
Removing this needs deeper surgery.
Since ixgbevf doesn't change hardware state on RX csum enable/disable
its reset is avoided.
Things noticed:
- e1000, e1000e and ixgb have RX csum disabled by default
- HW VLAN acceleration probably can be toggled, but it's left as is
- the resets on RX csum offload change can probably be avoided
- there is A LOT of copy-and-pasted code here
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/e1000/e1000_ethtool.c | 69 ------------------------------------
drivers/net/e1000/e1000_main.c | 31 ++++++++++++++--
drivers/net/e1000e/ethtool.c | 62 --------------------------------
drivers/net/e1000e/netdev.c | 31 ++++++++++++++--
drivers/net/igb/igb_ethtool.c | 67 ----------------------------------
drivers/net/igb/igb_main.c | 34 ++++++++++++++----
drivers/net/igbvf/ethtool.c | 57 -----------------------------
drivers/net/igbvf/netdev.c | 26 +++++++++++---
drivers/net/ixgb/ixgb.h | 2 +
drivers/net/ixgb/ixgb_ethtool.c | 59 +------------------------------
drivers/net/ixgb/ixgb_main.c | 31 ++++++++++++++--
drivers/net/ixgbe/ixgbe_ethtool.c | 65 ---------------------------------
drivers/net/ixgbe/ixgbe_main.c | 32 ++++++++++++-----
drivers/net/ixgbevf/ethtool.c | 46 ------------------------
drivers/net/ixgbevf/ixgbevf_main.c | 27 +++++++++++---
15 files changed, 178 insertions(+), 461 deletions(-)
diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index f4d0922..8002b5b 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -288,69 +288,6 @@ static int e1000_set_pauseparam(struct net_device *netdev,
return retval;
}
-static u32 e1000_get_rx_csum(struct net_device *netdev)
-{
- struct e1000_adapter *adapter = netdev_priv(netdev);
- return ...Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
include/linux/netdevice.h | 2 +
net/core/dev.c | 8 ++
net/core/ethtool.c | 252 +++++++++++++++++++-------------------------
3 files changed, 119 insertions(+), 143 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4b20944..7634cab 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -941,6 +941,8 @@ struct net_device {
#define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
#define NETIF_F_ALL_CSUM (NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
+#define NETIF_F_ALL_TSO (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
+
/*
* If one device supports one of these features, then enable them
* for all in netdev_increment_features.
diff --git a/net/core/dev.c b/net/core/dev.c
index 1e616bb..95d0a49 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5054,6 +5054,14 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
features &= ~NETIF_F_TSO;
}
+ /* Software GSO depends on SG. */
+ if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
+ if (name)
+ printk(KERN_NOTICE "%s: Dropping NETIF_F_GSO since no "
+ "SG feature.\n", name);
+ features &= ~NETIF_F_GSO;
+ }
+
if (features & NETIF_F_UFO) {
/* maybe split UFO into V4 and V6? */
if (!((features & NETIF_F_GEN_CSUM) ||
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f08e7f1..b068738 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -55,6 +55,7 @@ int ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
return 0;
}
+EXPORT_SYMBOL(ethtool_op_set_tx_csum);
int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data)
{
@@ -220,6 +221,85 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
return 0;
}
+static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
+{
+ switch (eth_cmd) {
+ case ETHTOOL_GTXCSUM:
+ case ...The severity of these messages will need to be reduced if ethtool relies on this function to propagate feature changes. (And I wonder why some I wonder whether this should cover NETIF_F_FCOE_CRC as well (ixgbe currently doesn't seem to allow toggling it). [...] This deserves some comments. Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. --
This mask is only for 'legacy' set_tx_csum ethtool commands. Other checksumming options - or whatever bits are included in hw_features - can be Sure. This is compatibility layer for current ethtool tools, and drivers not converted to ndo_fix_features() and friends. Best Regards, Michał Mirosław --
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
net/bridge/br_device.c | 57 +++++-----------------------------------------
net/bridge/br_if.c | 17 ++++++-------
net/bridge/br_notify.c | 2 +-
net/bridge/br_private.h | 4 +-
4 files changed, 18 insertions(+), 62 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 5564435..78193ad 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -78,7 +78,7 @@ static int br_dev_open(struct net_device *dev)
{
struct net_bridge *br = netdev_priv(dev);
- br_features_recompute(br);
+ netdev_update_features(dev);
netif_start_queue(dev);
br_stp_enable_bridge(br);
br_multicast_open(br);
@@ -173,48 +173,12 @@ static void br_getinfo(struct net_device *dev, struct ethtool_drvinfo *info)
strcpy(info->bus_info, "N/A");
}
-static int br_set_sg(struct net_device *dev, u32 data)
+static unsigned long br_fix_features(struct net_device *dev,
+ unsigned long features)
{
struct net_bridge *br = netdev_priv(dev);
- if (data)
- br->feature_mask |= NETIF_F_SG;
- else
- br->feature_mask &= ~NETIF_F_SG;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_tso(struct net_device *dev, u32 data)
-{
- struct net_bridge *br = netdev_priv(dev);
-
- if (data)
- br->feature_mask |= NETIF_F_TSO;
- else
- br->feature_mask &= ~NETIF_F_TSO;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_tx_csum(struct net_device *dev, u32 data)
-{
- struct net_bridge *br = netdev_priv(dev);
-
- if (data)
- br->feature_mask |= NETIF_F_NO_CSUM;
- else
- br->feature_mask &= ~NETIF_F_ALL_CSUM;
-
- br_features_recompute(br);
- return 0;
-}
-
-static int br_set_flags(struct net_device *netdev, u32 data)
-{
- return ethtool_op_set_flags(netdev, data, ETH_FLAG_TXVLAN);
+ return br_features_recompute(br, features);
}
#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -300,16 +264,6 @@ void br_netpoll_disable(struct net_bridge_port *p)
...Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/virtio_net.c | 46 +++++++++++++++++++---------------------------
1 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 90a23e4..a6b3914 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -705,17 +705,6 @@ static int virtnet_close(struct net_device *dev)
return 0;
}
-static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
-{
- struct virtnet_info *vi = netdev_priv(dev);
- struct virtio_device *vdev = vi->vdev;
-
- if (data && !virtio_has_feature(vdev, VIRTIO_NET_F_CSUM))
- return -ENOSYS;
-
- return ethtool_op_set_tx_hw_csum(dev, data);
-}
-
static void virtnet_set_rx_mode(struct net_device *dev)
{
struct virtnet_info *vi = netdev_priv(dev);
@@ -817,10 +806,6 @@ static void virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
}
static const struct ethtool_ops virtnet_ethtool_ops = {
- .set_tx_csum = virtnet_set_tx_csum,
- .set_sg = ethtool_op_set_sg,
- .set_tso = ethtool_op_set_tso,
- .set_ufo = ethtool_op_set_ufo,
.get_link = ethtool_op_get_link,
};
@@ -907,22 +892,29 @@ static int virtnet_probe(struct virtio_device *vdev)
SET_NETDEV_DEV(dev, &vdev->dev);
/* Do we support "hardware" checksums? */
- if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
/* This opens up the world of extra features. */
- dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
- if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
- dev->features |= NETIF_F_TSO | NETIF_F_UFO
+ dev->hw_features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+ if (csum)
+ dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
+ dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
| NETIF_F_TSO_ECN | NETIF_F_TSO6;
}
/* Individual feature bits: ...Introduce NETIF_F_RXCSUM to replace device-private flags for RX checksum
offload. Integrate it with ndo_fix_features.
ethtool_op_get_rx_csum() is removed altogether as nothing in-tree uses it.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
include/linux/ethtool.h | 1 -
include/linux/netdevice.h | 3 +++
net/core/ethtool.c | 34 +++++++++++-----------------------
3 files changed, 14 insertions(+), 24 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 0267d45..2475b41 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -568,7 +568,6 @@ struct net_device;
/* Some generic methods drivers may use in their ethtool_ops */
u32 ethtool_op_get_link(struct net_device *dev);
-u32 ethtool_op_get_rx_csum(struct net_device *dev);
u32 ethtool_op_get_tx_csum(struct net_device *dev);
int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7634cab..ce35514 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -920,6 +920,7 @@ struct net_device {
#define NETIF_F_FCOE_MTU (1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
#define NETIF_F_NTUPLE (1 << 27) /* N-tuple filters supported */
#define NETIF_F_RXHASH (1 << 28) /* Receive hashing offload */
+#define NETIF_F_RXCSUM (1 << 29) /* Receive checksumming offload */
/* Segmentation offload features */
#define NETIF_F_GSO_SHIFT 16
@@ -2379,6 +2380,8 @@ static inline int dev_ethtool_get_settings(struct net_device *dev,
static inline u32 dev_ethtool_get_rx_csum(struct net_device *dev)
{
+ if (dev->hw_features & NETIF_F_RXCSUM)
+ return !!(dev->features & NETIF_F_RXCSUM);
if (!dev->ethtool_ops || !dev->ethtool_ops->get_rx_csum)
return 0;
return dev->ethtool_ops->get_rx_csum(dev);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b068738..15e00b8 100644
--- ...Not explicitly, but what about drivers that turn TX and RX checksumming on and off together? They are implicitly covered by the case which you're removing: [...] Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. --
I had an impression that drivers not defining get_rx_csum() had no RX checksumming or didn't allow it to be toggled. I'll have a closer look on this. Best Regards, Michał Mirosław --
All drivers that have set_rx_csum() also define matching get_rx_csum(). Anyway, the code this is removing is broken in that it assumes TX csum capability implying RX csum capability. As a side note, GRO receive functions (TCPv4 and v6 - no other protocols use it) are checking skb->ip_summed != CHECKSUM_NONE before allowing the packet to be passed on, so it's not even necessary to force disabling GRO when RX csum is off. Best Regards, Michał Mirosław --
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
net/core/ethtool.c | 22 ++++++++++++++++++++--
1 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 15e00b8..2f532d5 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -215,6 +215,25 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
return 0;
}
+static int __ethtool_set_flags(struct net_device *dev, u32 data)
+{
+ if (data & ~flags_dup_features)
+ return -EINVAL;
+
+ if (!(dev->hw_features & flags_dup_features)) {
+ if (!dev->ethtool_ops->set_flags)
+ return -EOPNOTSUPP;
+ return dev->ethtool_ops->set_flags(dev, data);
+ }
+
+ dev->wanted_features =
+ (dev->wanted_features & ~flags_dup_features) | data;
+
+ netdev_update_features(dev);
+
+ return 0;
+}
+
static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
{
switch (eth_cmd) {
@@ -1635,8 +1654,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
ethtool_op_get_flags));
break;
case ETHTOOL_SFLAGS:
- rc = ethtool_set_value(dev, useraddr,
- dev->ethtool_ops->set_flags);
+ rc = ethtool_set_value(dev, useraddr, __ethtool_set_flags);
break;
case ETHTOOL_GPFLAGS:
rc = ethtool_get_value(dev, useraddr, ethcmd,
--
1.7.2.3
--
