[PATCH 1/2] sfc: Remove requirement to set tx-usecs-irq for shared channels when modifying coalescing parameters

[PATCH 1/2] sfc: Remove requirement to set tx-usecs-irq for shared channels when modifying coalescing parameters

[Ripduman Sohan]

Shared TX/RX channels possess a single channel timer controlled by the
rx-usecs-irq parameter.  Changing coalescing parameters required
explicitly setting the tx-usecs-irq parameter to 0.  Ethtool (to HEAD
of tree) does not do this and instead retrieves and re-submits the
current tx-usecs-irq value resulting in an unsupported operation
error.  I found this behaviour counter-intuitive and was only able to
work out correct moderation parameters by studying the driver code.

This patch relaxes the requirement to set tx-usecs-irq to 0 by only
erring if the presented tx-usecs-irq value differs from the current
value.  I acknowledge, however, that there may be existing scripts
relying on the old behaviour and so this condition is only triggered
if a value for tx-usecs-irq is actually presented.
---
 drivers/net/sfc/efx.c     |    6 +++---
 drivers/net/sfc/efx.h     |    1 +
 drivers/net/sfc/ethtool.c |    4 +++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index faca764..9a313cd 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1556,7 +1556,7 @@ static void efx_remove_all(struct efx_nic *efx)
  *
  **************************************************************************/
 
-static unsigned irq_mod_ticks(int usecs, int resolution)
+unsigned efx_irq_mod_ticks(int usecs, int resolution)
 {
 	if (usecs <= 0)
 		return 0; /* cannot receive interrupts ahead of time :-) */
@@ -1570,8 +1570,8 @@ void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs, int rx_usecs,
 			     bool rx_adaptive)
 {
 	struct efx_channel *channel;
-	unsigned tx_ticks = irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
-	unsigned rx_ticks = irq_mod_ticks(rx_usecs, EFX_IRQ_MOD_RESOLUTION);
+	unsigned tx_ticks = efx_irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
+	unsigned rx_ticks = efx_irq_mod_ticks(rx_usecs, EFX_IRQ_MOD_RESOLUTION);
 
 	EFX_ASSERT_RESET_SERIALISED(efx);
 
diff --git a/drivers/net/sfc/efx.h b/drivers/net/sfc/efx.h
index b0d1209..ddfcc7e 100644
--- a/drivers/net/sfc/efx.h
+++ b/drivers/net/sfc/efx.h
@@ -113,6 +113,7 @@ extern int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok);
 extern void efx_schedule_reset(struct efx_nic *efx, enum reset_type type);
 extern void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs,
 				    int rx_usecs, bool rx_adaptive);
+extern unsigned efx_irq_mod_ticks(int usecs, int resolution);
 
 /* Dummy PHY ops for PHY drivers */
 extern int efx_port_dummy_op_int(struct efx_nic *efx);
diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
index bc4643a..0a52447 100644
--- a/drivers/net/sfc/ethtool.c
+++ b/drivers/net/sfc/ethtool.c
@@ -644,7 +644,9 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 	efx_for_each_channel(channel, efx) {
 		if (efx_channel_has_rx_queue(channel) &&
 		    efx_channel_has_tx_queues(channel) &&
-		    tx_usecs) {
+		    tx_usecs &&
+		    efx_irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION) !=
+		    channel->irq_moderation) {
 			netif_err(efx, drv, efx->net_dev, "Channel is shared. "
 				  "Only RX coalescing may be set\n");
 			return -EOPNOTSUPP;
-- 
1.7.1

Re: [PATCH 1/2] sfc: Remove requirement to set tx-usecs-irq for shared channels when modifying coalescing parameters

[Ben Hutchings]

On Wed, 2011-08-31 at 10:52 +0100, Ripduman Sohan wrote:
> Shared TX/RX channels possess a single channel timer controlled by the
> rx-usecs-irq parameter.  Changing coalescing parameters required
> explicitly setting the tx-usecs-irq parameter to 0.  Ethtool (to HEAD
> of tree) does not do this and instead retrieves and re-submits the
> current tx-usecs-irq value resulting in an unsupported operation
> error.  I found this behaviour counter-intuitive and was only able to
> work out correct moderation parameters by studying the driver code.
> 
> This patch relaxes the requirement to set tx-usecs-irq to 0 by only
> erring if the presented tx-usecs-irq value differs from the current
> value.  I acknowledge, however, that there may be existing scripts
> relying on the old behaviour and so this condition is only triggered
> if a value for tx-usecs-irq is actually presented.

After you first reminded me about this in email, I had a good look at
our ethtool coalescing control functions and tried to fix them
thoroughly.

Eli Cohen also recently queried about the semantics of the fields in
struct ethtool_coalesce
<http://thread.gmane.org/gmane.linux.network/202368>, so I updated the
kernel-doc comments for it (19e2f6f..a27fc96).

So I have some changes of my own in preparation, which I'll send as
follow-ups.

> ---
>  drivers/net/sfc/efx.c     |    6 +++---
>  drivers/net/sfc/efx.h     |    1 +
>  drivers/net/sfc/ethtool.c |    4 +++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
> index faca764..9a313cd 100644
> --- a/drivers/net/sfc/efx.c
> +++ b/drivers/net/sfc/efx.c
> @@ -1556,7 +1556,7 @@ static void efx_remove_all(struct efx_nic *efx)
>   *
>   **************************************************************************/
>  
> -static unsigned irq_mod_ticks(int usecs, int resolution)
> +unsigned efx_irq_mod_ticks(int usecs, int resolution)
>  {
>  	if (usecs <= 0)
>  		return 0; /* cannot receive interrupts ahead of time :-) */
> @@ -1570,8 +1570,8 @@ void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs, int rx_usecs,
>  			     bool rx_adaptive)
>  {
>  	struct efx_channel *channel;
> -	unsigned tx_ticks = irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
> -	unsigned rx_ticks = irq_mod_ticks(rx_usecs, EFX_IRQ_MOD_RESOLUTION);
> +	unsigned tx_ticks = efx_irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
> +	unsigned rx_ticks = efx_irq_mod_ticks(rx_usecs, EFX_IRQ_MOD_RESOLUTION);
>  
>  	EFX_ASSERT_RESET_SERIALISED(efx);
>  

I would rather add a efx_get_irq_moderation() function for use in
ethtool.c.

> diff --git a/drivers/net/sfc/efx.h b/drivers/net/sfc/efx.h
> index b0d1209..ddfcc7e 100644
> --- a/drivers/net/sfc/efx.h
> +++ b/drivers/net/sfc/efx.h
> @@ -113,6 +113,7 @@ extern int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok);
>  extern void efx_schedule_reset(struct efx_nic *efx, enum reset_type type);
>  extern void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs,
>  				    int rx_usecs, bool rx_adaptive);
> +extern unsigned efx_irq_mod_ticks(int usecs, int resolution);
>  
>  /* Dummy PHY ops for PHY drivers */
>  extern int efx_port_dummy_op_int(struct efx_nic *efx);
> diff --git a/drivers/net/sfc/ethtool.c b/drivers/net/sfc/ethtool.c
> index bc4643a..0a52447 100644
> --- a/drivers/net/sfc/ethtool.c
> +++ b/drivers/net/sfc/ethtool.c
> @@ -644,7 +644,9 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
>  	efx_for_each_channel(channel, efx) {
>  		if (efx_channel_has_rx_queue(channel) &&
>  		    efx_channel_has_tx_queues(channel) &&
> -		    tx_usecs) {
> +		    tx_usecs &&
> +		    efx_irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION) !=
> +		    channel->irq_moderation) {
>  			netif_err(efx, drv, efx->net_dev, "Channel is shared. "
>  				  "Only RX coalescing may be set\n");
>  			return -EOPNOTSUPP;

channel->irq_moderation will be the value selected by the adaptive
moderation algorithm.  efx->rx_irq_moderation is the appropriate value
to use here.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 1/5] sfc: Correct error code for unsupported interrupt coalescing parameters

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/ethtool.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index bc4643a..6de2715 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -628,12 +628,12 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 	unsigned tx_usecs, rx_usecs, adaptive;
 
 	if (coalesce->use_adaptive_tx_coalesce)
-		return -EOPNOTSUPP;
+		return -EINVAL;
 
 	if (coalesce->rx_coalesce_usecs || coalesce->tx_coalesce_usecs) {
 		netif_err(efx, drv, efx->net_dev, "invalid coalescing setting. "
 			  "Only rx/tx_coalesce_usecs_irq are supported\n");
-		return -EOPNOTSUPP;
+		return -EINVAL;
 	}
 
 	rx_usecs = coalesce->rx_coalesce_usecs_irq;
@@ -647,7 +647,7 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 		    tx_usecs) {
 			netif_err(efx, drv, efx->net_dev, "Channel is shared. "
 				  "Only RX coalescing may be set\n");
-			return -EOPNOTSUPP;
+			return -EINVAL;
 		}
 	}
 
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 2/5] sfc: Use consistent types for interrupt coalescing parameters

[Ben Hutchings]

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c     |   10 +++++-----
 drivers/net/ethernet/sfc/efx.h     |    4 ++--
 drivers/net/ethernet/sfc/ethtool.c |    3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index b6b0e71..097ed8b 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1556,18 +1556,18 @@ static void efx_remove_all(struct efx_nic *efx)
  *
  **************************************************************************/
 
-static unsigned irq_mod_ticks(int usecs, int resolution)
+static unsigned int irq_mod_ticks(unsigned int usecs, unsigned int resolution)
 {
-	if (usecs <= 0)
-		return 0; /* cannot receive interrupts ahead of time :-) */
+	if (usecs == 0)
+		return 0;
 	if (usecs < resolution)
 		return 1; /* never round down to 0 */
 	return usecs / resolution;
 }
 
 /* Set interrupt moderation parameters */
-void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs, int rx_usecs,
-			     bool rx_adaptive)
+void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
+			     unsigned int rx_usecs, bool rx_adaptive)
 {
 	struct efx_channel *channel;
 	unsigned tx_ticks = irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index b0d1209..8f5acae 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -111,8 +111,8 @@ extern int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok);
 
 /* Global */
 extern void efx_schedule_reset(struct efx_nic *efx, enum reset_type type);
-extern void efx_init_irq_moderation(struct efx_nic *efx, int tx_usecs,
-				    int rx_usecs, bool rx_adaptive);
+extern void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
+				    unsigned int rx_usecs, bool rx_adaptive);
 
 /* Dummy PHY ops for PHY drivers */
 extern int efx_port_dummy_op_int(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 6de2715..dedaa2c 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -625,7 +625,8 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct efx_channel *channel;
-	unsigned tx_usecs, rx_usecs, adaptive;
+	unsigned int tx_usecs, rx_usecs;
+	bool adaptive;
 
 	if (coalesce->use_adaptive_tx_coalesce)
 		return -EINVAL;
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 3/5] sfc: Correct reporting and validation of TX interrupt coalescing

[Ben Hutchings]

The reported TX IRQ moderation is generated in a completely crazy way.
Make it simple and correct.

When channels are shared between RX and TX, TX IRQ moderation must be
the same as RX IRQ moderation, but must be specified as 0!  Allow it
to be either specified as the same, or left at its previous value
in which case it will be quietly overridden.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c     |   18 ++++++++++
 drivers/net/ethernet/sfc/efx.h     |    2 +
 drivers/net/ethernet/sfc/ethtool.c |   67 +++++++++++++++++------------------
 3 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 097ed8b..e0157c0 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1585,6 +1585,24 @@ void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
 	}
 }
 
+void efx_get_irq_moderation(struct efx_nic *efx, unsigned int *tx_usecs,
+			    unsigned int *rx_usecs, bool *rx_adaptive)
+{
+	*rx_adaptive = efx->irq_rx_adaptive;
+	*rx_usecs = efx->irq_rx_moderation * EFX_IRQ_MOD_RESOLUTION;
+
+	/* If channels are shared between RX and TX, so is IRQ
+	 * moderation.  Otherwise, IRQ moderation is the same for all
+	 * TX channels and is not adaptive.
+	 */
+	if (efx->tx_channel_offset == 0)
+		*tx_usecs = *rx_usecs;
+	else
+		*tx_usecs =
+			efx->channel[efx->tx_channel_offset]->irq_moderation *
+			EFX_IRQ_MOD_RESOLUTION;
+}
+
 /**************************************************************************
  *
  * Hardware monitor
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index 8f5acae..8ca6863 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -113,6 +113,8 @@ extern int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok);
 extern void efx_schedule_reset(struct efx_nic *efx, enum reset_type type);
 extern void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
 				    unsigned int rx_usecs, bool rx_adaptive);
+extern void efx_get_irq_moderation(struct efx_nic *efx, unsigned int *tx_usecs,
+				   unsigned int *rx_usecs, bool *rx_adaptive);
 
 /* Dummy PHY ops for PHY drivers */
 extern int efx_port_dummy_op_int(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index dedaa2c..1cb6ed0 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -586,40 +586,37 @@ static int efx_ethtool_nway_reset(struct net_device *net_dev)
 	return mdio45_nway_restart(&efx->mdio);
 }
 
+/*
+ * Each channel has a single IRQ and moderation timer, started by any
+ * completion (or other event).  Unless the module parameter
+ * separate_tx_channels is set, IRQs and moderation are therefore
+ * shared between RX and TX completions.  In this case, when RX IRQ
+ * moderation is explicitly changed then TX IRQ moderation is
+ * automatically changed too, but otherwise we fail if the two values
+ * are requested to be different.
+ *
+ * We implement adaptive IRQ moderation, but use a different algorithm
+ * from that assumed in the definition of struct ethtool_coalesce.
+ * Therefore we do not use any of the adaptive moderation parameters
+ * in it.
+ */
+
 static int efx_ethtool_get_coalesce(struct net_device *net_dev,
 				    struct ethtool_coalesce *coalesce)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
-	struct efx_channel *channel;
-
-	memset(coalesce, 0, sizeof(*coalesce));
-
-	/* Find lowest IRQ moderation across all used TX queues */
-	coalesce->tx_coalesce_usecs_irq = ~((u32) 0);
-	efx_for_each_channel(channel, efx) {
-		if (!efx_channel_has_tx_queues(channel))
-			continue;
-		if (channel->irq_moderation < coalesce->tx_coalesce_usecs_irq) {
-			if (channel->channel < efx->n_rx_channels)
-				coalesce->tx_coalesce_usecs_irq =
-					channel->irq_moderation;
-			else
-				coalesce->tx_coalesce_usecs_irq = 0;
-		}
-	}
+	unsigned int tx_usecs, rx_usecs;
+	bool rx_adaptive;
 
-	coalesce->use_adaptive_rx_coalesce = efx->irq_rx_adaptive;
-	coalesce->rx_coalesce_usecs_irq = efx->irq_rx_moderation;
+	efx_get_irq_moderation(efx, &tx_usecs, &rx_usecs, &rx_adaptive);
 
-	coalesce->tx_coalesce_usecs_irq *= EFX_IRQ_MOD_RESOLUTION;
-	coalesce->rx_coalesce_usecs_irq *= EFX_IRQ_MOD_RESOLUTION;
+	coalesce->tx_coalesce_usecs_irq = tx_usecs;
+	coalesce->rx_coalesce_usecs_irq = rx_usecs;
+	coalesce->use_adaptive_rx_coalesce = rx_adaptive;
 
 	return 0;
 }
 
-/* Set coalescing parameters
- * The difficulties occur for shared channels
- */
 static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 				    struct ethtool_coalesce *coalesce)
 {
@@ -637,20 +634,22 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 		return -EINVAL;
 	}
 
+	efx_get_irq_moderation(efx, &tx_usecs, &rx_usecs, &adaptive);
+
 	rx_usecs = coalesce->rx_coalesce_usecs_irq;
-	tx_usecs = coalesce->tx_coalesce_usecs_irq;
 	adaptive = coalesce->use_adaptive_rx_coalesce;
 
-	/* If the channel is shared only allow RX parameters to be set */
-	efx_for_each_channel(channel, efx) {
-		if (efx_channel_has_rx_queue(channel) &&
-		    efx_channel_has_tx_queues(channel) &&
-		    tx_usecs) {
-			netif_err(efx, drv, efx->net_dev, "Channel is shared. "
-				  "Only RX coalescing may be set\n");
-			return -EINVAL;
-		}
+	/* If channels are shared, TX IRQ moderation can be quietly
+	 * overridden unless it is changed from its old value.
+	 */
+	if (efx->tx_channel_offset == 0 &&
+	    coalesce->tx_coalesce_usecs_irq != tx_usecs &&
+	    coalesce->tx_coalesce_usecs_irq != rx_usecs) {
+		netif_err(efx, drv, efx->net_dev, "Channels are shared. "
+			  "RX and TX IRQ moderation must be equal\n");
+		return -EINVAL;
 	}
+	tx_usecs = coalesce->tx_coalesce_usecs_irq;
 
 	efx_init_irq_moderation(efx, tx_usecs, rx_usecs, adaptive);
 	efx_for_each_channel(channel, efx)
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 4/5] sfc: Validate IRQ moderation parameters in efx_init_irq_moderation()

[Ben Hutchings]

Add a range check, and move the check that RX and TX are consistent
from efx_ethtool_set_coalesce().

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c     |   20 +++++++++++++++++---
 drivers/net/ethernet/sfc/efx.h     |    5 +++--
 drivers/net/ethernet/sfc/ethtool.c |   17 ++++++++---------
 drivers/net/ethernet/sfc/falcon.c  |    2 ++
 drivers/net/ethernet/sfc/nic.h     |    3 ++-
 drivers/net/ethernet/sfc/siena.c   |    2 ++
 6 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index e0157c0..76dcadf 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1357,7 +1357,8 @@ static int efx_probe_nic(struct efx_nic *efx)
 	netif_set_real_num_rx_queues(efx->net_dev, efx->n_rx_channels);
 
 	/* Initialise the interrupt moderation settings */
-	efx_init_irq_moderation(efx, tx_irq_mod_usec, rx_irq_mod_usec, true);
+	efx_init_irq_moderation(efx, tx_irq_mod_usec, rx_irq_mod_usec, true,
+				true);
 
 	return 0;
 
@@ -1566,8 +1567,9 @@ static unsigned int irq_mod_ticks(unsigned int usecs, unsigned int resolution)
 }
 
 /* Set interrupt moderation parameters */
-void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
-			     unsigned int rx_usecs, bool rx_adaptive)
+int efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
+			    unsigned int rx_usecs, bool rx_adaptive,
+			    bool rx_may_override_tx)
 {
 	struct efx_channel *channel;
 	unsigned tx_ticks = irq_mod_ticks(tx_usecs, EFX_IRQ_MOD_RESOLUTION);
@@ -1575,6 +1577,16 @@ void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
 
 	EFX_ASSERT_RESET_SERIALISED(efx);
 
+	if (tx_ticks > EFX_IRQ_MOD_MAX || rx_ticks > EFX_IRQ_MOD_MAX)
+		return -EINVAL;
+
+	if (tx_ticks != rx_ticks && efx->tx_channel_offset == 0 &&
+	    !rx_may_override_tx) {
+		netif_err(efx, drv, efx->net_dev, "Channels are shared. "
+			  "RX and TX IRQ moderation must be equal\n");
+		return -EINVAL;
+	}
+
 	efx->irq_rx_adaptive = rx_adaptive;
 	efx->irq_rx_moderation = rx_ticks;
 	efx_for_each_channel(channel, efx) {
@@ -1583,6 +1595,8 @@ void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
 		else if (efx_channel_has_tx_queues(channel))
 			channel->irq_moderation = tx_ticks;
 	}
+
+	return 0;
 }
 
 void efx_get_irq_moderation(struct efx_nic *efx, unsigned int *tx_usecs,
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index 8ca6863..442f4d0 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -111,8 +111,9 @@ extern int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok);
 
 /* Global */
 extern void efx_schedule_reset(struct efx_nic *efx, enum reset_type type);
-extern void efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
-				    unsigned int rx_usecs, bool rx_adaptive);
+extern int efx_init_irq_moderation(struct efx_nic *efx, unsigned int tx_usecs,
+				   unsigned int rx_usecs, bool rx_adaptive,
+				   bool rx_may_override_tx);
 extern void efx_get_irq_moderation(struct efx_nic *efx, unsigned int *tx_usecs,
 				   unsigned int *rx_usecs, bool *rx_adaptive);
 
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 1cb6ed0..98b363b 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -623,7 +623,8 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct efx_channel *channel;
 	unsigned int tx_usecs, rx_usecs;
-	bool adaptive;
+	bool adaptive, rx_may_override_tx;
+	int rc;
 
 	if (coalesce->use_adaptive_tx_coalesce)
 		return -EINVAL;
@@ -642,16 +643,14 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 	/* If channels are shared, TX IRQ moderation can be quietly
 	 * overridden unless it is changed from its old value.
 	 */
-	if (efx->tx_channel_offset == 0 &&
-	    coalesce->tx_coalesce_usecs_irq != tx_usecs &&
-	    coalesce->tx_coalesce_usecs_irq != rx_usecs) {
-		netif_err(efx, drv, efx->net_dev, "Channels are shared. "
-			  "RX and TX IRQ moderation must be equal\n");
-		return -EINVAL;
-	}
+	rx_may_override_tx = coalesce->tx_coalesce_usecs_irq == tx_usecs;
 	tx_usecs = coalesce->tx_coalesce_usecs_irq;
 
-	efx_init_irq_moderation(efx, tx_usecs, rx_usecs, adaptive);
+	rc = efx_init_irq_moderation(efx, tx_usecs, rx_usecs, adaptive,
+				     rx_may_override_tx);
+	if (rc != 0)
+		return rc;
+
 	efx_for_each_channel(channel, efx)
 		efx->type->push_irq_moderation(channel);
 
diff --git a/drivers/net/ethernet/sfc/falcon.c b/drivers/net/ethernet/sfc/falcon.c
index 94bf4aa..4dd1748 100644
--- a/drivers/net/ethernet/sfc/falcon.c
+++ b/drivers/net/ethernet/sfc/falcon.c
@@ -104,6 +104,8 @@ static void falcon_push_irq_moderation(struct efx_channel *channel)
 	efx_dword_t timer_cmd;
 	struct efx_nic *efx = channel->efx;
 
+	BUILD_BUG_ON(EFX_IRQ_MOD_MAX > (1 << FRF_AB_TC_TIMER_VAL_WIDTH));
+
 	/* Set timer register */
 	if (channel->irq_moderation) {
 		EFX_POPULATE_DWORD_2(timer_cmd,
diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h
index 4bd1f28..b5b2886 100644
--- a/drivers/net/ethernet/sfc/nic.h
+++ b/drivers/net/ethernet/sfc/nic.h
@@ -204,7 +204,8 @@ extern irqreturn_t efx_nic_fatal_interrupt(struct efx_nic *efx);
 extern irqreturn_t falcon_legacy_interrupt_a1(int irq, void *dev_id);
 extern void falcon_irq_ack_a1(struct efx_nic *efx);
 
-#define EFX_IRQ_MOD_RESOLUTION 5
+#define EFX_IRQ_MOD_RESOLUTION	5
+#define EFX_IRQ_MOD_MAX		0x1000
 
 /* Global Resources */
 extern int efx_nic_flush_queues(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/siena.c b/drivers/net/ethernet/sfc/siena.c
index 5735e84..4fdd148 100644
--- a/drivers/net/ethernet/sfc/siena.c
+++ b/drivers/net/ethernet/sfc/siena.c
@@ -36,6 +36,8 @@ static void siena_push_irq_moderation(struct efx_channel *channel)
 {
 	efx_dword_t timer_cmd;
 
+	BUILD_BUG_ON(EFX_IRQ_MOD_MAX > (1 << FRF_CZ_TC_TIMER_VAL_WIDTH));
+
 	if (channel->irq_moderation)
 		EFX_POPULATE_DWORD_2(timer_cmd,
 				     FRF_CZ_TC_TIMER_MODE,
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 5/5] sfc: Use correct fields of struct ethtool_coalesce

[Ben Hutchings]

An earlier developer misunderstood the meaning of the 'irq' fields and
the driver did not support the standard fields.  To avoid invalidating
existing user documentation, we report and accept changes through
either the standard or 'irq' fields.  If both are changed at the same
time, we prefer the standard field.

Also explain why we don't currently use the 'max_frames' fields.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/ethernet/sfc/ethtool.c |   36 +++++++++++++++++++++++++++---------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 98b363b..93f1fb9 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -595,6 +595,20 @@ static int efx_ethtool_nway_reset(struct net_device *net_dev)
  * automatically changed too, but otherwise we fail if the two values
  * are requested to be different.
  *
+ * The hardware does not support a limit on the number of completions
+ * before an IRQ, so we do not use the max_frames fields.  We should
+ * report and require that max_frames == (usecs != 0), but this would
+ * invalidate existing user documentation.
+ *
+ * The hardware does not have distinct settings for interrupt
+ * moderation while the previous IRQ is being handled, so we should
+ * not use the 'irq' fields.  However, an earlier developer
+ * misunderstood the meaning of the 'irq' fields and the driver did
+ * not support the standard fields.  To avoid invalidating existing
+ * user documentation, we report and accept changes through either the
+ * standard or 'irq' fields.  If both are changed at the same time, we
+ * prefer the standard field.
+ *
  * We implement adaptive IRQ moderation, but use a different algorithm
  * from that assumed in the definition of struct ethtool_coalesce.
  * Therefore we do not use any of the adaptive moderation parameters
@@ -610,7 +624,9 @@ static int efx_ethtool_get_coalesce(struct net_device *net_dev,
 
 	efx_get_irq_moderation(efx, &tx_usecs, &rx_usecs, &rx_adaptive);
 
+	coalesce->tx_coalesce_usecs = tx_usecs;
 	coalesce->tx_coalesce_usecs_irq = tx_usecs;
+	coalesce->rx_coalesce_usecs = rx_usecs;
 	coalesce->rx_coalesce_usecs_irq = rx_usecs;
 	coalesce->use_adaptive_rx_coalesce = rx_adaptive;
 
@@ -629,22 +645,24 @@ static int efx_ethtool_set_coalesce(struct net_device *net_dev,
 	if (coalesce->use_adaptive_tx_coalesce)
 		return -EINVAL;
 
-	if (coalesce->rx_coalesce_usecs || coalesce->tx_coalesce_usecs) {
-		netif_err(efx, drv, efx->net_dev, "invalid coalescing setting. "
-			  "Only rx/tx_coalesce_usecs_irq are supported\n");
-		return -EINVAL;
-	}
-
 	efx_get_irq_moderation(efx, &tx_usecs, &rx_usecs, &adaptive);
 
-	rx_usecs = coalesce->rx_coalesce_usecs_irq;
+	if (coalesce->rx_coalesce_usecs != rx_usecs)
+		rx_usecs = coalesce->rx_coalesce_usecs;
+	else
+		rx_usecs = coalesce->rx_coalesce_usecs_irq;
+
 	adaptive = coalesce->use_adaptive_rx_coalesce;
 
 	/* If channels are shared, TX IRQ moderation can be quietly
 	 * overridden unless it is changed from its old value.
 	 */
-	rx_may_override_tx = coalesce->tx_coalesce_usecs_irq == tx_usecs;
-	tx_usecs = coalesce->tx_coalesce_usecs_irq;
+	rx_may_override_tx = (coalesce->tx_coalesce_usecs == tx_usecs &&
+			      coalesce->tx_coalesce_usecs_irq == tx_usecs);
+	if (coalesce->tx_coalesce_usecs != tx_usecs)
+		tx_usecs = coalesce->tx_coalesce_usecs;
+	else
+		tx_usecs = coalesce->tx_coalesce_usecs_irq;
 
 	rc = efx_init_irq_moderation(efx, tx_usecs, rx_usecs, adaptive,
 				     rx_may_override_tx);
-- 
1.7.4.4


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/2] sfc: Remove requirement to set tx-usecs-irq for shared channels when modifying coalescing parameters

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 05 Sep 2011 15:12:01 +0100

> So I have some changes of my own in preparation, which I'll send as
> follow-ups.

Ben, just FYI, I have applied those follow-up patches to SFC.

Thanks.