All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
@ 2010-06-30 12:44 Ben Hutchings
  2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Ben Hutchings @ 2010-06-30 12:44 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, linux-net-drivers, Stanislaw Gruszka, Amit Salecha,
	Amerigo Wang, Anirban Chakraborty, Dimitris Michailidis,
	Scott Feldman, Vasanthy Kolluri, Roopa Prabhu, e1000-devel,
	Lennert Buytenhek, Andrew Gallatin, Brice Goglin,
	Stephen Hemminger, Jeff Garzik

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
@@ -365,7 +365,6 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.get_coalesce = enic_get_coalesce,
 	.set_coalesce = enic_set_coalesce,
 	.get_flags = ethtool_op_get_flags,
-	.set_flags = ethtool_op_set_flags,
 };
 
 static void enic_free_wq_buf(struct vnic_wq *wq, struct vnic_wq_buf *buf)
diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 873b45e..7d2e5ea 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -2205,8 +2205,11 @@ static int ixgbe_set_flags(struct net_device *netdev, u32 data)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	bool need_reset = false;
+	int rc;
 
-	ethtool_op_set_flags(netdev, data);
+	rc = ethtool_op_set_flags(netdev, data, ETH_FLAG_LRO | ETH_FLAG_NTUPLE);
+	if (rc)
+		return rc;
 
 	/* if state changes we need to update adapter->flags and reset */
 	if (adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE) {
diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index e345ec8..82b720f 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -1636,6 +1636,11 @@ static void mv643xx_eth_get_ethtool_stats(struct net_device *dev,
 	}
 }
 
+static int mv643xx_eth_set_flags(struct net_device *dev, u32 data)
+{
+	return ethtool_op_set_flags(dev, data, ETH_FLAG_LRO);
+}
+
 static int mv643xx_eth_get_sset_count(struct net_device *dev, int sset)
 {
 	if (sset == ETH_SS_STATS)
@@ -1661,7 +1666,7 @@ static const struct ethtool_ops mv643xx_eth_ethtool_ops = {
 	.get_strings		= mv643xx_eth_get_strings,
 	.get_ethtool_stats	= mv643xx_eth_get_ethtool_stats,
 	.get_flags		= ethtool_op_get_flags,
-	.set_flags		= ethtool_op_set_flags,
+	.set_flags		= mv643xx_eth_set_flags,
 	.get_sset_count		= mv643xx_eth_get_sset_count,
 };
 
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index e0b47cc..d771d16 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -1730,8 +1730,7 @@ static int myri10ge_set_rx_csum(struct net_device *netdev, u32 csum_enabled)
 	if (csum_enabled)
 		mgp->csum_flag = MXGEFW_FLAGS_CKSUM;
 	else {
-		u32 flags = ethtool_op_get_flags(netdev);
-		err = ethtool_op_set_flags(netdev, (flags & ~ETH_FLAG_LRO));
+		netdev->features &= ~NETIF_F_LRO;
 		mgp->csum_flag = 0;
 
 	}
@@ -1900,6 +1899,11 @@ static u32 myri10ge_get_msglevel(struct net_device *netdev)
 	return mgp->msg_enable;
 }
 
+static int myri10ge_set_flags(struct net_device *netdev, u32 value)
+{
+	return ethtool_op_set_flags(netdev, value, ETH_FLAG_LRO);
+}
+
 static const struct ethtool_ops myri10ge_ethtool_ops = {
 	.get_settings = myri10ge_get_settings,
 	.get_drvinfo = myri10ge_get_drvinfo,
@@ -1920,7 +1924,7 @@ static const struct ethtool_ops myri10ge_ethtool_ops = {
 	.set_msglevel = myri10ge_set_msglevel,
 	.get_msglevel = myri10ge_get_msglevel,
 	.get_flags = ethtool_op_get_flags,
-	.set_flags = ethtool_op_set_flags
+	.set_flags = myri10ge_set_flags
 };
 
 static int myri10ge_allocate_rings(struct myri10ge_slice_state *ss)
diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 63e8e38..3d523cb 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -7920,14 +7920,7 @@ static int niu_phys_id(struct net_device *dev, u32 data)
 
 static int niu_set_flags(struct net_device *dev, u32 data)
 {
-	if (data & (ETH_FLAG_LRO | ETH_FLAG_NTUPLE))
-		return -EOPNOTSUPP;
-
-	if (data & ETH_FLAG_RXHASH)
-		dev->features |= NETIF_F_RXHASH;
-	else
-		dev->features &= ~NETIF_F_RXHASH;
-	return 0;
+	return ethtool_op_set_flags(dev, data, ETH_FLAG_RXHASH);
 }
 
 static const struct ethtool_ops niu_ethtool_ops = {
diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
index 7693cfb..23372bf 100644
--- a/drivers/net/sfc/ethtool.c
+++ b/drivers/net/sfc/ethtool.c
@@ -551,10 +551,7 @@ static int efx_ethtool_set_flags(struct net_device *net_dev, u32 data)
 	struct efx_nic *efx = netdev_priv(net_dev);
 	u32 supported = efx->type->offload_features & ETH_FLAG_RXHASH;
 
-	if (data & ~supported)
-		return -EOPNOTSUPP;
-
-	return ethtool_op_set_flags(net_dev, data);
+	return ethtool_op_set_flags(net_dev, data, supported);
 }
 
 static void efx_ethtool_self_test(struct net_device *net_dev,
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 7985165..c762c6a 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -4188,17 +4188,13 @@ static int sky2_set_eeprom(struct net_device *dev, struct ethtool_eeprom *eeprom
 static int sky2_set_flags(struct net_device *dev, u32 data)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
+	u32 supported =
+		(sky2->hw->flags & SKY2_HW_RSS_BROKEN) ? 0 : ETH_FLAG_RXHASH;
+	int rc;
 
-	if (data & ~ETH_FLAG_RXHASH)
-		return -EOPNOTSUPP;
-
-	if (data & ETH_FLAG_RXHASH) {
-		if (sky2->hw->flags & SKY2_HW_RSS_BROKEN)
-			return -EINVAL;
-
-		dev->features |= NETIF_F_RXHASH;
-	} else
-		dev->features &= ~NETIF_F_RXHASH;
+	rc = ethtool_op_set_flags(dev, data, supported);
+	if (rc)
+		return rc;
 
 	rx_set_rss(dev);
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 2c8af09..084ddb3 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -457,7 +457,7 @@ int ethtool_op_set_tso(struct net_device *dev, u32 data);
 u32 ethtool_op_get_ufo(struct net_device *dev);
 int ethtool_op_set_ufo(struct net_device *dev, u32 data);
 u32 ethtool_op_get_flags(struct net_device *dev);
-int ethtool_op_set_flags(struct net_device *dev, u32 data);
+int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);
 void ethtool_ntuple_flush(struct net_device *dev);
 
 /**
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index a0f4964..5d42fae 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -144,31 +144,13 @@ u32 ethtool_op_get_flags(struct net_device *dev)
 }
 EXPORT_SYMBOL(ethtool_op_get_flags);
 
-int ethtool_op_set_flags(struct net_device *dev, u32 data)
+int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
 {
-	const struct ethtool_ops *ops = dev->ethtool_ops;
-	unsigned long features = dev->features;
-
-	if (data & ETH_FLAG_LRO)
-		features |= NETIF_F_LRO;
-	else
-		features &= ~NETIF_F_LRO;
-
-	if (data & ETH_FLAG_NTUPLE) {
-		if (!ops->set_rx_ntuple)
-			return -EOPNOTSUPP;
-		features |= NETIF_F_NTUPLE;
-	} else {
-		/* safe to clear regardless */
-		features &= ~NETIF_F_NTUPLE;
-	}
-
-	if (data & ETH_FLAG_RXHASH)
-		features |= NETIF_F_RXHASH;
-	else
-		features &= ~NETIF_F_RXHASH;
+	if (data & ~supported)
+		return -EINVAL;
 
-	dev->features = features;
+	dev->features = ((dev->features & ~flags_dup_features) |
+			 (data & flags_dup_features));
 	return 0;
 }
 EXPORT_SYMBOL(ethtool_op_set_flags);
-- 
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.


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags
  2010-06-30 12:44 [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags Ben Hutchings
@ 2010-06-30 12:46 ` Ben Hutchings
  2010-06-30 15:01   ` Eilon Greenstein
  2010-06-30 21:10   ` David Miller
  2010-06-30 12:47 ` [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags() Ben Hutchings
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 19+ messages in thread
From: Ben Hutchings @ 2010-06-30 12:46 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

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
@@ -6692,7 +6692,7 @@ static int s2io_ethtool_set_flags(struct net_device *dev, u32 data)
 	int changed = 0;
 
 	if (data & ~ETH_FLAG_LRO)
-		return -EOPNOTSUPP;
+		return -EINVAL;
 
 	if (data & ETH_FLAG_LRO) {
 		if (lro_enable) {
-- 
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.


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags()
  2010-06-30 12:44 [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags Ben Hutchings
  2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
@ 2010-06-30 12:47 ` Ben Hutchings
  2010-06-30 15:44   ` [Pv-drivers] " Bhavesh Davda
  2010-06-30 21:10   ` David Miller
  2010-06-30 21:10 ` [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags David Miller
  2010-07-02 16:55 ` Randy Dunlap
  3 siblings, 2 replies; 19+ messages in thread
From: Ben Hutchings @ 2010-06-30 12:47 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Shreyas Bhatewara, VMware, Inc.

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.


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags
  2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
@ 2010-06-30 15:01   ` Eilon Greenstein
  2010-06-30 21:10   ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Eilon Greenstein @ 2010-06-30 15:01 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev

On Wed, 2010-06-30 at 05:46 -0700, Ben Hutchings wrote:
> 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>

If that's what the document say...
Acked-by: Eilon Greenstein <eilong@broadcom.com>

Thanks Ben!




^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags()
  2010-06-30 12:47 ` [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags() Ben Hutchings
@ 2010-06-30 15:44   ` Bhavesh Davda
  2010-06-30 15:58     ` Ben Hutchings
  2010-06-30 21:10   ` David Miller
  1 sibling, 1 reply; 19+ messages in thread
From: Bhavesh Davda @ 2010-06-30 15:44 UTC (permalink / raw)
  To: Ben Hutchings, David Miller; +Cc: VMware, Inc., netdev

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
 
Bhavesh P. Davda

> -----Original Message-----
> From: pv-drivers-bounces@vmware.com [mailto:pv-drivers-
> bounces@vmware.com] On Behalf Of Ben Hutchings
> Sent: Wednesday, June 30, 2010 5:48 AM
> To: David Miller
> Cc: VMware, Inc.; netdev@vger.kernel.org
> Subject: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove
> incorrect implementation of ethtool_ops::get_flags()
> 
> 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.
> 
> _______________________________________________
> Pv-drivers mailing list
> Pv-drivers@vmware.com
> http://mailman2.vmware.com/mailman/listinfo/pv-drivers

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags()
  2010-06-30 15:44   ` [Pv-drivers] " Bhavesh Davda
@ 2010-06-30 15:58     ` Ben Hutchings
  2010-06-30 16:46       ` Bhavesh Davda
  0 siblings, 1 reply; 19+ messages in thread
From: Ben Hutchings @ 2010-06-30 15:58 UTC (permalink / raw)
  To: Bhavesh Davda; +Cc: David Miller, VMware, Inc., netdev

On Wed, 2010-06-30 at 08:44 -0700, Bhavesh Davda wrote:
> 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?

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags()
  2010-06-30 15:58     ` Ben Hutchings
@ 2010-06-30 16:46       ` Bhavesh Davda
  0 siblings, 0 replies; 19+ messages in thread
From: Bhavesh Davda @ 2010-06-30 16:46 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, VMware, Inc., netdev

> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Wednesday, June 30, 2010 8:59 AM
> 
> On Wed, 2010-06-30 at 08:44 -0700, Bhavesh Davda wrote:
> > 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?
> 
> 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.
> 

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
  2010-06-30 12:44 [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags Ben Hutchings
  2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
  2010-06-30 12:47 ` [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags() Ben Hutchings
@ 2010-06-30 21:10 ` David Miller
  2010-07-02 16:55 ` Randy Dunlap
  3 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2010-06-30 21:10 UTC (permalink / raw)
  To: bhutchings
  Cc: netdev, linux-net-drivers, sgruszka, amit.salecha, amwang,
	anirban.chakraborty, dm, scofeldm, vkolluri, roprabhu,
	e1000-devel, buytenh, gallatin, brice, shemminger, jgarzik

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 30 Jun 2010 13:44:32 +0100

> 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>

Applied.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags
  2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
  2010-06-30 15:01   ` Eilon Greenstein
@ 2010-06-30 21:10   ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2010-06-30 21:10 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 30 Jun 2010 13:46:56 +0100

> 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>

Applied.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags()
  2010-06-30 12:47 ` [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags() Ben Hutchings
  2010-06-30 15:44   ` [Pv-drivers] " Bhavesh Davda
@ 2010-06-30 21:10   ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2010-06-30 21:10 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, sbhatewara, pv-drivers

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 30 Jun 2010 13:47:40 +0100

> 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>

Applied.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
  2010-06-30 12:44 [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags Ben Hutchings
                   ` (2 preceding siblings ...)
  2010-06-30 21:10 ` [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags David Miller
@ 2010-07-02 16:55 ` Randy Dunlap
  2010-07-03  5:07   ` David Miller
  3 siblings, 1 reply; 19+ messages in thread
From: Randy Dunlap @ 2010-07-02 16:55 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Amit Salecha, linux-net-drivers, Dimitris Michailidis,
	Stanislaw Gruszka, Amerigo Wang, Jeff, e1000-devel, netdev,
	Anirban Chakraborty, Garzik, Vasanthy Kolluri, Brice Goglin,
	Andrew Gallatin, Scott Feldman, Stephen Hemminger, David Miller,
	Lennert Buytenhek, Roopa Prabhu

On Wed, 30 Jun 2010 13:44:32 +0100 Ben Hutchings wrote:

> 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/include/linux/ethtool.h b/include/linux/ethtool.h
> index 2c8af09..084ddb3 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -457,7 +457,7 @@ int ethtool_op_set_tso(struct net_device *dev, u32 data);
>  u32 ethtool_op_get_ufo(struct net_device *dev);
>  int ethtool_op_set_ufo(struct net_device *dev, u32 data);
>  u32 ethtool_op_get_flags(struct net_device *dev);
> -int ethtool_op_set_flags(struct net_device *dev, u32 data);
> +int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);

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.
I don't quite see how this happened...


>  void ethtool_ntuple_flush(struct net_device *dev);
>  
>  /**
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index a0f4964..5d42fae 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -144,31 +144,13 @@ u32 ethtool_op_get_flags(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(ethtool_op_get_flags);
>  
> -int ethtool_op_set_flags(struct net_device *dev, u32 data)
> +int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
>  {
> -	const struct ethtool_ops *ops = dev->ethtool_ops;
> -	unsigned long features = dev->features;
> -
> -	if (data & ETH_FLAG_LRO)
> -		features |= NETIF_F_LRO;
> -	else
> -		features &= ~NETIF_F_LRO;
> -
> -	if (data & ETH_FLAG_NTUPLE) {
> -		if (!ops->set_rx_ntuple)
> -			return -EOPNOTSUPP;
> -		features |= NETIF_F_NTUPLE;
> -	} else {
> -		/* safe to clear regardless */
> -		features &= ~NETIF_F_NTUPLE;
> -	}
> -
> -	if (data & ETH_FLAG_RXHASH)
> -		features |= NETIF_F_RXHASH;
> -	else
> -		features &= ~NETIF_F_RXHASH;
> +	if (data & ~supported)
> +		return -EINVAL;
>  
> -	dev->features = features;
> +	dev->features = ((dev->features & ~flags_dup_features) |
> +			 (data & flags_dup_features));
>  	return 0;
>  }
>  EXPORT_SYMBOL(ethtool_op_set_flags);
> -- 

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
  2010-07-02 16:55 ` Randy Dunlap
@ 2010-07-03  5:07   ` David Miller
  2010-07-03 19:07     ` Randy Dunlap
  0 siblings, 1 reply; 19+ messages in thread
From: David Miller @ 2010-07-03  5:07 UTC (permalink / raw)
  To: randy.dunlap
  Cc: bhutchings, netdev, linux-net-drivers, sgruszka, amit.salecha,
	amwang, anirban.chakraborty, dm, scofeldm, vkolluri, roprabhu,
	e1000-devel, buytenh, gallatin, brice, shemminger, jgarzik

From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Fri, 2 Jul 2010 09:55:14 -0700

> On Wed, 30 Jun 2010 13:44:32 +0100 Ben Hutchings wrote:
>> @@ -457,7 +457,7 @@ int ethtool_op_set_tso(struct net_device *dev, u32 data);
>>  u32 ethtool_op_get_ufo(struct net_device *dev);
>>  int ethtool_op_set_ufo(struct net_device *dev, u32 data);
>>  u32 ethtool_op_get_flags(struct net_device *dev);
>> -int ethtool_op_set_flags(struct net_device *dev, u32 data);
>> +int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);
> 
> 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

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);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
  2010-07-03  5:07   ` David Miller
@ 2010-07-03 19:07     ` Randy Dunlap
  2010-07-03 19:21       ` Ben Hutchings
  0 siblings, 1 reply; 19+ messages in thread
From: Randy Dunlap @ 2010-07-03 19:07 UTC (permalink / raw)
  To: David Miller
  Cc: amit.salecha, linux-net-drivers, dm, sgruszka, amwang,
	e1000-devel, netdev, anirban.chakraborty, vkolluri, brice,
	gallatin, bhutchings, scofeldm, shemminger, jgarzik, buytenh,
	roprabhu

On Fri, 02 Jul 2010 22:07:11 -0700 (PDT) David Miller wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> Date: Fri, 2 Jul 2010 09:55:14 -0700
> 
> > On Wed, 30 Jun 2010 13:44:32 +0100 Ben Hutchings wrote:
> >> @@ -457,7 +457,7 @@ int ethtool_op_set_tso(struct net_device *dev, u32 data);
> >>  u32 ethtool_op_get_ufo(struct net_device *dev);
> >>  int ethtool_op_set_ufo(struct net_device *dev, u32 data);
> >>  u32 ethtool_op_get_flags(struct net_device *dev);
> >> -int ethtool_op_set_flags(struct net_device *dev, u32 data);
> >> +int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);
> > 
> > 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
> 
> 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 ***

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags
  2010-07-03 19:07     ` Randy Dunlap
@ 2010-07-03 19:21       ` Ben Hutchings
  2010-07-03 19:41         ` [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags() Ben Hutchings
  0 siblings, 1 reply; 19+ messages in thread
From: Ben Hutchings @ 2010-07-03 19:21 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: David Miller, netdev, linux-net-drivers, sgruszka, amit.salecha,
	amwang, anirban.chakraborty, dm, scofeldm, vkolluri, roprabhu,
	e1000-devel, buytenh, gallatin, brice, shemminger, jgarzik

On Sat, 2010-07-03 at 12:07 -0700, Randy Dunlap wrote:
> On Fri, 02 Jul 2010 22:07:11 -0700 (PDT) David Miller wrote:
> 
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > Date: Fri, 2 Jul 2010 09:55:14 -0700
> > 
> > > On Wed, 30 Jun 2010 13:44:32 +0100 Ben Hutchings wrote:
> > >> @@ -457,7 +457,7 @@ int ethtool_op_set_tso(struct net_device *dev, u32 data);
> > >>  u32 ethtool_op_get_ufo(struct net_device *dev);
> > >>  int ethtool_op_set_ufo(struct net_device *dev, u32 data);
> > >>  u32 ethtool_op_get_flags(struct net_device *dev);
> > >> -int ethtool_op_set_flags(struct net_device *dev, u32 data);
> > >> +int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported);
> > > 
> > > 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
> > 
> > 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.

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags()
  2010-07-03 19:21       ` Ben Hutchings
@ 2010-07-03 19:41         ` Ben Hutchings
  2010-07-03 20:08           ` Roland Dreier
  2010-07-06 16:22           ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Randy Dunlap
  0 siblings, 2 replies; 19+ messages in thread
From: Ben Hutchings @ 2010-07-03 19:41 UTC (permalink / raw)
  To: David Miller, Roland Dreier
  Cc: Randy Dunlap, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-net-drivers-s/n/eUQHGBpZroRs9YW3xA,
	sgruszka-H+wXaHxf7aLQT0dZR+AlfA,
	amit.salecha-h88ZbnxC6KDQT0dZR+AlfA,
	amwang-H+wXaHxf7aLQT0dZR+AlfA,
	anirban.chakraborty-h88ZbnxC6KDQT0dZR+AlfA,
	dm-ut6Up61K2wZBDgjK7y7TUQ, scofeldm-FYB4Gu1CFyUAvxtiuMwx3w,
	vkolluri-FYB4Gu1CFyUAvxtiuMwx3w, roprabhu-FYB4Gu1CFyUAvxtiuMwx3w,
	e1000-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	buytenh-OLH4Qvv75CYX/NnBR394Jw, gallatin-vV262kQ/Wyo,
	brice-vV262kQ/Wyo, shemminger-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	jgarzik-H+wXaHxf7aLQT0dZR+AlfA, Faisal Latif, Chien Tung,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

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-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org>
---
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, ETH_FLAG_LRO);
+}
+
 static const struct ethtool_ops ipoib_ethtool_ops = {
 	.get_drvinfo		= ipoib_get_drvinfo,
 	.get_rx_csum		= ipoib_get_rx_csum,
@@ -154,7 +159,7 @@ static const struct ethtool_ops ipoib_ethtool_ops = {
 	.get_coalesce		= ipoib_get_coalesce,
 	.set_coalesce		= ipoib_set_coalesce,
 	.get_flags		= ethtool_op_get_flags,
-	.set_flags		= ethtool_op_set_flags,
+	.set_flags		= ipoib_set_flags,
 	.get_strings		= ipoib_get_strings,
 	.get_sset_count		= ipoib_get_sset_count,
 	.get_ethtool_stats	= ipoib_get_ethtool_stats,
-- 
1.7.1


-- 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags()
  2010-07-03 19:41         ` [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags() Ben Hutchings
@ 2010-07-03 20:08           ` Roland Dreier
  2010-07-03 20:40             ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Ben Hutchings
  2010-07-04 18:48             ` [PATCH net-next-2.6] IB/{nes,ipoib}: " David Miller
  2010-07-06 16:22           ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Randy Dunlap
  1 sibling, 2 replies; 19+ messages in thread
From: Roland Dreier @ 2010-07-03 20:08 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: David Miller, Roland Dreier, Randy Dunlap,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-net-drivers-s/n/eUQHGBpZroRs9YW3xA,
	sgruszka-H+wXaHxf7aLQT0dZR+AlfA,
	amit.salecha-h88ZbnxC6KDQT0dZR+AlfA,
	amwang-H+wXaHxf7aLQT0dZR+AlfA,
	anirban.chakraborty-h88ZbnxC6KDQT0dZR+AlfA,
	dm-ut6Up61K2wZBDgjK7y7TUQ, scofeldm-FYB4Gu1CFyUAvxtiuMwx3w,
	vkolluri-FYB4Gu1CFyUAvxtiuMwx3w, roprabhu-FYB4Gu1CFyUAvxtiuMwx3w,
	e1000-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
	buytenh-OLH4Qvv75CYX/NnBR394Jw, gallatin-vV262kQ/Wyo,
	brice-vV262kQ/Wyo, shemminger-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	jgarzik-H+wXaHxf7aLQT0dZR+AlfA, Faisal Latif, Chien Tung,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

 > 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-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>

 > +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-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6] IB/{nes, ipoib}: Pass supported flags to ethtool_op_set_flags()
  2010-07-03 20:08           ` Roland Dreier
@ 2010-07-03 20:40             ` Ben Hutchings
  2010-07-04 18:48             ` [PATCH net-next-2.6] IB/{nes,ipoib}: " David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Ben Hutchings @ 2010-07-03 20:40 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Randy Dunlap, amit.salecha, Chien Tung, sgruszka, amwang, dm,
	brice, gallatin, shemminger, jgarzik, roprabhu, e1000-devel,
	scofeldm, Roland Dreier, buytenh, Faisal Latif, vkolluri,
	linux-net-drivers, netdev, anirban.chakraborty, linux-rdma,
	David Miller

On Sat, 2010-07-03 at 13:08 -0700, Roland Dreier wrote:
> > 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?

Right.

>  > 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?

Unfortunately it's too late to do that.

> 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.

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.


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags()
  2010-07-03 20:08           ` Roland Dreier
  2010-07-03 20:40             ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Ben Hutchings
@ 2010-07-04 18:48             ` David Miller
  1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2010-07-04 18:48 UTC (permalink / raw)
  To: rdreier
  Cc: bhutchings, rolandd, randy.dunlap, netdev, linux-net-drivers,
	sgruszka, amit.salecha, amwang, anirban.chakraborty, dm,
	scofeldm, vkolluri, roprabhu, e1000-devel, buytenh, gallatin,
	brice, shemminger, jgarzik, faisal.latif, chien.tin.tung,
	linux-rdma

From: Roland Dreier <rdreier@cisco.com>
Date: Sat, 03 Jul 2010 13:08:29 -0700

>  > 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.
> 
> Acked-by: Roland Dreier <rolandd@cisco.com>

Applied, thanks guys.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH net-next-2.6] IB/{nes, ipoib}: Pass supported flags to ethtool_op_set_flags()
  2010-07-03 19:41         ` [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags() Ben Hutchings
  2010-07-03 20:08           ` Roland Dreier
@ 2010-07-06 16:22           ` Randy Dunlap
  1 sibling, 0 replies; 19+ messages in thread
From: Randy Dunlap @ 2010-07-06 16:22 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: amit.salecha, linux-net-drivers, dm, sgruszka, amwang,
	Faisal Latif, e1000-devel, netdev, anirban.chakraborty, jgarzik,
	vkolluri, Chien Tung, brice, gallatin, linux-rdma, scofeldm,
	Roland Dreier, shemminger, David Miller, buytenh, roprabhu

On 07/03/10 12:41, Ben Hutchings wrote:
> 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.

Ack, thanks.

> 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, ETH_FLAG_LRO);
> +}
> +
>  static const struct ethtool_ops ipoib_ethtool_ops = {
>  	.get_drvinfo		= ipoib_get_drvinfo,
>  	.get_rx_csum		= ipoib_get_rx_csum,
> @@ -154,7 +159,7 @@ static const struct ethtool_ops ipoib_ethtool_ops = {
>  	.get_coalesce		= ipoib_get_coalesce,
>  	.set_coalesce		= ipoib_set_coalesce,
>  	.get_flags		= ethtool_op_get_flags,
> -	.set_flags		= ethtool_op_set_flags,
> +	.set_flags		= ipoib_set_flags,
>  	.get_strings		= ipoib_get_strings,
>  	.get_sset_count		= ipoib_get_sset_count,
>  	.get_ethtool_stats	= ipoib_get_ethtool_stats,


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2010-07-06 16:22 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-30 12:44 [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags Ben Hutchings
2010-06-30 12:46 ` [PATCH net-next-2.6 2/3] netdev: Make ethtool_ops::set_flags() return -EINVAL for unsupported flags Ben Hutchings
2010-06-30 15:01   ` Eilon Greenstein
2010-06-30 21:10   ` David Miller
2010-06-30 12:47 ` [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get_flags() Ben Hutchings
2010-06-30 15:44   ` [Pv-drivers] " Bhavesh Davda
2010-06-30 15:58     ` Ben Hutchings
2010-06-30 16:46       ` Bhavesh Davda
2010-06-30 21:10   ` David Miller
2010-06-30 21:10 ` [PATCH net-next-2.6 1/3] ethtool: Change ethtool_op_set_flags to validate flags David Miller
2010-07-02 16:55 ` Randy Dunlap
2010-07-03  5:07   ` David Miller
2010-07-03 19:07     ` Randy Dunlap
2010-07-03 19:21       ` Ben Hutchings
2010-07-03 19:41         ` [PATCH net-next-2.6] IB/{nes,ipoib}: Pass supported flags to ethtool_op_set_flags() Ben Hutchings
2010-07-03 20:08           ` Roland Dreier
2010-07-03 20:40             ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Ben Hutchings
2010-07-04 18:48             ` [PATCH net-next-2.6] IB/{nes,ipoib}: " David Miller
2010-07-06 16:22           ` [PATCH net-next-2.6] IB/{nes, ipoib}: " Randy Dunlap

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.