Re: [PATCH 1/2] sfc: Remove requirement to set tx-usecs-irq for shared channels when modifying coalescing parameters
From: Ben Hutchings <hidden>
Date: 2011-09-05 14:12:05
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.
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
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.