login
Search
ethtool_op_set_flags() does not check for unsupported flags, and has
no way of doing so. This means it is not suitable for use as a
default implementation of ethtool_ops::set_flags.
Add a 'supported' parameter specifying the flags that the driver and
hardware support, validate the requested flags against this, and
change all current callers to pass this parameter.
Change some other trivial implementations of ethtool_ops::set_flags to
call ethtool_op_set_flags().
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Reviewed-by: Stanislaw Gruszka <sgruszka@redhat.com>
Acked-by: Jeff Garzik <jgarzik@redhat.com>
---
drivers/net/cxgb4/cxgb4_main.c | 9 +--------
drivers/net/enic/enic_main.c | 1 -
drivers/net/ixgbe/ixgbe_ethtool.c | 5 ++++-
drivers/net/mv643xx_eth.c | 7 ++++++-
drivers/net/myri10ge/myri10ge.c | 10 +++++++---
drivers/net/niu.c | 9 +--------
drivers/net/sfc/ethtool.c | 5 +----
drivers/net/sky2.c | 16 ++++++----------
include/linux/ethtool.h | 2 +-
net/core/ethtool.c | 28 +++++-----------------------
10 files changed, 32 insertions(+), 60 deletions(-)
diff --git a/drivers/net/cxgb4/cxgb4_main.c b/drivers/net/cxgb4/cxgb4_main.c
index 6528167..55a720e 100644
--- a/drivers/net/cxgb4/cxgb4_main.c
+++ b/drivers/net/cxgb4/cxgb4_main.c
@@ -1799,14 +1799,7 @@ static int set_tso(struct net_device *dev, u32 value)
static int set_flags(struct net_device *dev, u32 flags)
{
- if (flags & ~ETH_FLAG_RXHASH)
- return -EOPNOTSUPP;
-
- if (flags & ETH_FLAG_RXHASH)
- dev->features |= NETIF_F_RXHASH;
- else
- dev->features &= ~NETIF_F_RXHASH;
- return 0;
+ return ethtool_op_set_flags(dev, flags, ETH_FLAG_RXHASH);
}
static struct ethtool_ops cxgb_ethtool_ops = {
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 6c6795b..77a7f87 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ ...Only some netdev feature flags correspond directly to ethtool feature
flags. ethtool_op_get_flags() does the right thing.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/vmxnet3/vmxnet3_ethtool.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 8a71a21..de1ba14 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -275,12 +275,6 @@ vmxnet3_get_strings(struct net_device *netdev, u32 stringset, u8 *buf)
}
}
-static u32
-vmxnet3_get_flags(struct net_device *netdev)
-{
- return netdev->features;
-}
-
static int
vmxnet3_set_flags(struct net_device *netdev, u32 data)
{
@@ -559,7 +553,7 @@ static struct ethtool_ops vmxnet3_ethtool_ops = {
.get_tso = ethtool_op_get_tso,
.set_tso = ethtool_op_set_tso,
.get_strings = vmxnet3_get_strings,
- .get_flags = vmxnet3_get_flags,
+ .get_flags = ethtool_op_get_flags,
.set_flags = vmxnet3_set_flags,
.get_sset_count = vmxnet3_get_sset_count,
.get_ethtool_stats = vmxnet3_get_ethtool_stats,
--
1.6.2.5
--
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.
--
Thanks for fixing this, Ben! Had to look at ethtool-2.6.34 src to convince myself of the correctness. Looks good to me. Signed-off-by: Bhavesh Davda <bhavesh@vmware.com> ps: I do wonder, however, why not always use ethtool_op_get_flags for all drivers, and mask whatever is returned by the driver specific dev->ethtool_ops->get_flags with flags_dup_features instead of this approach? - Bhavesh --
I think you're right that ethtool_op_get_flags could be the implicit default (i.e. used if ethtool_ops::get_flags is NULL) and drivers should not have to set it. Following this change, no drivers in net-next-2.6 will be using any other implementation. However, I don't think ethtool_ops::get_flags should be removed - in future there are likely to be additional ethtool flags that do not correspond to net device feature flags, and some drivers will need a different implementation. 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. --
Hi Ben,
I'm not suggesting nuking ethtool_ops::get_flags. What I was suggesting is *always* calling ethtool_ops_get_flags from dev_ethtool for case ETHTOOL_GFLAGS, but doing something like this simple patch:
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index a0f4964..f5da6ed 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -139,8 +139,9 @@ u32 ethtool_op_get_flags(struct net_device *dev)
* handling for flags which are not so easily handled
* by a simple masking operation
*/
-
- return dev->features & flags_dup_features;
+ return (dev->ethtool_ops->get_flags ?
+ dev->ethtool_ops->get_flags(dev) :
+ dev->features) & flags_dup_features;
}
EXPORT_SYMBOL(ethtool_op_get_flags);
@@ -1495,9 +1496,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
break;
case ETHTOOL_GFLAGS:
rc = ethtool_get_value(dev, useraddr, ethcmd,
- (dev->ethtool_ops->get_flags ?
- dev->ethtool_ops->get_flags :
- ethtool_op_get_flags));
+ ethtool_op_get_flags);
break;
case ETHTOOL_SFLAGS:
rc = ethtool_set_value(dev, useraddr,
Plus nuking all such code in the netdev drivers:
.get_flags = ethtool_op_get_flags,
This is still pretty fragile and could lead to infinite recursion if some drivers are missed. But you get the idea.
Thanks
- Bhavesh
--
From: Ben Hutchings <bhutchings@solarflare.com> Applied. --
The documented error code for attempts to set unsupported flags (or
to clear flags that cannot be disabled) is EINVAL, not EOPNOTSUPP.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/bnx2x_main.c | 2 +-
drivers/net/netxen/netxen_nic_ethtool.c | 2 +-
drivers/net/qlcnic/qlcnic_ethtool.c | 2 +-
drivers/net/s2io.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 0809f6c..29e293f 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -10983,7 +10983,7 @@ static int bnx2x_set_flags(struct net_device *dev, u32 data)
int rc = 0;
if (data & ~(ETH_FLAG_LRO | ETH_FLAG_RXHASH))
- return -EOPNOTSUPP;
+ return -EINVAL;
if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
printk(KERN_ERR "Handling parity error recovery. Try again later\n");
diff --git a/drivers/net/netxen/netxen_nic_ethtool.c b/drivers/net/netxen/netxen_nic_ethtool.c
index 6d94ee5..b30de24 100644
--- a/drivers/net/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/netxen/netxen_nic_ethtool.c
@@ -888,7 +888,7 @@ static int netxen_nic_set_flags(struct net_device *netdev, u32 data)
int hw_lro;
if (data & ~ETH_FLAG_LRO)
- return -EOPNOTSUPP;
+ return -EINVAL;
if (!(adapter->capabilities & NX_FW_CAPABILITY_HW_LRO))
return -EINVAL;
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index b9d5acb..f8e39e4 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -984,7 +984,7 @@ static int qlcnic_set_flags(struct net_device *netdev, u32 data)
int hw_lro;
if (data & ~ETH_FLAG_LRO)
- return -EOPNOTSUPP;
+ return -EINVAL;
if (!(adapter->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO))
return -EINVAL;
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index a032d72..22371f1 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ ...If that's what the document say... Acked-by: Eilon Greenstein <eilong@broadcom.com> Thanks Ben! --
From: Ben Hutchings <bhutchings@solarflare.com> Applied. --
From: Ben Hutchings <bhutchings@solarflare.com> Applied. --
That one-line change is missing from linux-next-20100702, causing: drivers/infiniband/ulp/ipoib/ipoib_ethtool.c:157: warning: initialization from incompatible pointer type but the change (below) to net/core/ethtool.c is merged. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
From: Randy Dunlap <randy.dunlap@oracle.com> Strange, it's in net-next-2.6 for sure: davem@sunset:~/src/GIT/net-next-2.6$ egrep ethtool_op_set_flags include/linux/ethtool.h int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported); --
Yep, my bad. In include/linux/ethtool.h, struct ethtool_ops, field/member 'set_flags': int (*set_flags)(struct net_device *, u32); Does that need another u32 for 'supported'? This is where the linux-next warnings are coming from. --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
No, this is intentional. I just missed the users of ethtool_op_set_flags() in drivers/infiniband. I'll send a patch for those shortly. 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. --
Following commit 1437ce3983bcbc0447a0dedcd644c14fe833d266 "ethtool:
Change ethtool_op_set_flags to validate flags", ethtool_op_set_flags
takes a third parameter and cannot be used directly as an
implementation of ethtool_ops::set_flags.
Changes nes and ipoib driver to pass in the appropriate value.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This is compile-tested only.
Dave, Roland, you'd better decide between yourselves should apply this.
Ben.
drivers/infiniband/hw/nes/nes_nic.c | 8 +++++++-
drivers/infiniband/ulp/ipoib/ipoib_ethtool.c | 7 ++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/nes/nes_nic.c b/drivers/infiniband/hw/nes/nes_nic.c
index 5cc0a9a..42e7aad 100644
--- a/drivers/infiniband/hw/nes/nes_nic.c
+++ b/drivers/infiniband/hw/nes/nes_nic.c
@@ -1567,6 +1567,12 @@ static int nes_netdev_set_settings(struct net_device *netdev, struct ethtool_cmd
}
+static int nes_netdev_set_flags(struct net_device *netdev, u32 flags)
+{
+ return ethtool_op_set_flags(netdev, flags, ETH_FLAG_LRO);
+}
+
+
static const struct ethtool_ops nes_ethtool_ops = {
.get_link = ethtool_op_get_link,
.get_settings = nes_netdev_get_settings,
@@ -1588,7 +1594,7 @@ static const struct ethtool_ops nes_ethtool_ops = {
.get_tso = ethtool_op_get_tso,
.set_tso = ethtool_op_set_tso,
.get_flags = ethtool_op_get_flags,
- .set_flags = ethtool_op_set_flags,
+ .set_flags = nes_netdev_set_flags,
};
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c b/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c
index 40e8584..1a1657c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c
@@ -147,6 +147,11 @@ static void ipoib_get_ethtool_stats(struct net_device *dev,
data[index++] = priv->lro.lro_mgr.stats.no_desc;
}
+static int ipoib_set_flags(struct net_device *dev, u32 flags)
+{
+ return ethtool_op_set_flags(dev, flags, ...> Following commit 1437ce3983bcbc0447a0dedcd644c14fe833d266 "ethtool:
> Change ethtool_op_set_flags to validate flags", ethtool_op_set_flags
> takes a third parameter and cannot be used directly as an
> implementation of ethtool_ops::set_flags.
I assume this commit is in one of Dave's trees and breaks nes and ipoib
because it changed the ethtool_op_set_flags prototype?
> Dave, Roland, you'd better decide between yourselves should apply this.
Since this depends on a commit already in Dave's tree, Dave should
probably take it into the same tree. I guess there's no way to roll
this into the original patch to avoid breaking bisects once this ends up
upstream?
anyway,
Acked-by: Roland Dreier <rolandd@cisco.com>
> +static int nes_netdev_set_flags(struct net_device *netdev, u32 flags)
> +{
> + return ethtool_op_set_flags(netdev, flags, ETH_FLAG_LRO);
> +}
> +
> +
> static const struct ethtool_ops nes_ethtool_ops = {
Also would have been a bit nicer to avoid adding a double blank line
here, although that's obviously a trivial issue.
- R.
--
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
This spacing is consistent with the surrounding code. 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. --
From: Roland Dreier <rdreier@cisco.com> Applied, thanks guys. --
-- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** --
