[PATCH 1/4] sfc: I2C adapter initialisation fixes

[PATCH 1/4] sfc: I2C adapter initialisation fixes

[Ben Hutchings]

As recommended by Jean Delvare:
- Increase timeout to 50 ms
- Leave adapter class clear so that unwanted drivers do not probe our bus
- Use strlcpy() for name initialisation

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
Jeff,

These are exactly the differences between net-2.6 and the version of sfc I
intended to submit to you before today.  If this doesn't apply then I'm
confused as to what you have.

Ben.

diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
index 630406e..9138ee5 100644
--- a/drivers/net/sfc/falcon.c
+++ b/drivers/net/sfc/falcon.c
@@ -223,13 +223,8 @@ static struct i2c_algo_bit_data falcon_i2c_bit_operations = {
 	.getsda		= falcon_getsda,
 	.getscl		= falcon_getscl,
 	.udelay		= 5,
-	/*
-	 * This is the number of system clock ticks after which
-	 * i2c-algo-bit gives up waiting for SCL to become high.
-	 * It must be at least 2 since the first tick can happen
-	 * immediately after it starts waiting.
-	 */
-	.timeout	= 2,
+	/* Wait up to 50 ms for slave to let us pull SCL high */
+	.timeout	= DIV_ROUND_UP(HZ, 20),
 };
 
 /**************************************************************************
@@ -2479,12 +2474,11 @@ int falcon_probe_nic(struct efx_nic *efx)
 
 	/* Initialise I2C adapter */
  	efx->i2c_adap.owner = THIS_MODULE;
- 	efx->i2c_adap.class = I2C_CLASS_HWMON;
 	nic_data->i2c_data = falcon_i2c_bit_operations;
 	nic_data->i2c_data.data = efx;
  	efx->i2c_adap.algo_data = &nic_data->i2c_data;
 	efx->i2c_adap.dev.parent = &efx->pci_dev->dev;
-	strcpy(efx->i2c_adap.name, "SFC4000 GPIO");
+	strlcpy(efx->i2c_adap.name, "SFC4000 GPIO", sizeof(efx->i2c_adap.name));
 	rc = i2c_bit_add_bus(&efx->i2c_adap);
 	if (rc)
 		goto fail5;

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 2/4] sfc: Use a separate workqueue for resets

[Ben Hutchings]

This avoids deadlock in case a reset is triggered during self-test.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/efx.c        |   20 +++++++++++++++++---
 drivers/net/sfc/net_driver.h |    5 ++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 0e5fecf..4e89c89 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -2046,7 +2046,7 @@ void efx_schedule_reset(struct efx_nic *efx, enum reset_type type)
 
 	efx->reset_pending = method;
 
-	queue_work(efx->workqueue, &efx->reset_work);
+	queue_work(efx->reset_workqueue, &efx->reset_work);
 }
 
 /**************************************************************************
@@ -2204,14 +2204,28 @@ static int efx_init_struct(struct efx_nic *efx, struct efx_nic_type *type,
 		goto fail1;
 	}
 
+	efx->reset_workqueue = create_singlethread_workqueue("sfc_reset");
+	if (!efx->reset_workqueue) {
+		rc = -ENOMEM;
+		goto fail2;
+	}
+
 	return 0;
 
+ fail2:
+	destroy_workqueue(efx->workqueue);
+	efx->workqueue = NULL;
+
  fail1:
 	return rc;
 }
 
 static void efx_fini_struct(struct efx_nic *efx)
 {
+	if (efx->reset_workqueue) {
+		destroy_workqueue(efx->reset_workqueue);
+		efx->reset_workqueue = NULL;
+	}
 	if (efx->workqueue) {
 		destroy_workqueue(efx->workqueue);
 		efx->workqueue = NULL;
@@ -2281,7 +2295,7 @@ static void efx_pci_remove(struct pci_dev *pci_dev)
 	 * scheduled from this point because efx_stop_all() has been
 	 * called, we are no longer registered with driverlink, and
 	 * the net_device's have been removed. */
-	flush_workqueue(efx->workqueue);
+	flush_workqueue(efx->reset_workqueue);
 
 	efx_pci_remove_main(efx);
 
@@ -2418,7 +2432,7 @@ static int __devinit efx_pci_probe(struct pci_dev *pci_dev,
 		 * scheduled since efx_stop_all() has been called, and we
 		 * have not and never have been registered with either
 		 * the rtnetlink or driverlink layers. */
-		cancel_work_sync(&efx->reset_work);
+		flush_workqueue(efx->reset_workqueue);
 
 		/* Retry if a recoverably reset event has been scheduled */
 		if ((efx->reset_pending != RESET_TYPE_INVISIBLE) &&
diff --git a/drivers/net/sfc/net_driver.h b/drivers/net/sfc/net_driver.h
index 535807b..c9ce460 100644
--- a/drivers/net/sfc/net_driver.h
+++ b/drivers/net/sfc/net_driver.h
@@ -733,7 +733,9 @@ struct efx_nic_errors {
  * @pci_dev: The PCI device
  * @type: Controller type attributes
  * @legacy_irq: IRQ number
- * @workqueue: Workqueue for resets, port reconfigures and the HW monitor
+ * @workqueue: Workqueue for port reconfigures and the HW monitor.
+ *	Work items do not hold and must not acquire RTNL.
+ * @reset_workqueue: Workqueue for resets.  Work item will acquire RTNL.
  * @reset_work: Scheduled reset workitem
  * @monitor_work: Hardware monitor workitem
  * @membase_phys: Memory BAR value as physical address
@@ -821,6 +823,7 @@ struct efx_nic {
 	const struct efx_nic_type *type;
 	int legacy_irq;
 	struct workqueue_struct *workqueue;
+	struct workqueue_struct *reset_workqueue;
 	struct work_struct reset_work;
 	struct delayed_work monitor_work;
 	unsigned long membase_phys;

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 3/4] sfc: resolve tx multiqueue bug

[Ben Hutchings]

With the recent changes to tx multiqueue, sfc was not calling
netif_start_queue() before calling netif_wake_queue().
This causes an oops when opening a device.

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

diff --git a/drivers/net/sfc/tx.c b/drivers/net/sfc/tx.c
index 5cdd082..798b06b 100644
--- a/drivers/net/sfc/tx.c
+++ b/drivers/net/sfc/tx.c
@@ -53,6 +53,7 @@ inline void efx_wake_queue(struct efx_nic *efx)
 	if (atomic_dec_and_lock(&efx->netif_stop_count,
 				&efx->netif_stop_lock)) {
 		EFX_TRACE(efx, "waking TX queue\n");
+		netif_start_queue(efx->net_dev);
 		netif_wake_queue(efx->net_dev);
 		spin_unlock(&efx->netif_stop_lock);
 	}

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 4/4] sfc: Create one RX queue and interrupt per CPU package by default

[Ben Hutchings]

Using multiple cores in the same package to handle received traffic
does not appear to provide a performance benefit.  Therefore use CPU
topology information to count CPU packages and use that as the default
number of RX queues and interrupts.  We rely on interrupt balancing to
spread the interrupts across packages.

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

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 7b2a818..45c72ee 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -19,6 +19,7 @@
 #include <linux/in.h>
 #include <linux/crc32.h>
 #include <linux/ethtool.h>
+#include <linux/topology.h>
 #include "net_driver.h"
 #include "gmii.h"
 #include "ethtool.h"
@@ -832,7 +833,23 @@ static void efx_probe_interrupts(struct efx_nic *efx)
 	if (efx->interrupt_mode == EFX_INT_MODE_MSIX) {
 		BUG_ON(!pci_find_capability(efx->pci_dev, PCI_CAP_ID_MSIX));
 
-		efx->rss_queues = rss_cpus ? rss_cpus : num_online_cpus();
+		if (rss_cpus == 0) {
+			cpumask_t core_mask;
+			int cpu;
+
+			cpus_clear(core_mask);
+			efx->rss_queues = 0;
+			for_each_online_cpu(cpu) {
+				if (!cpu_isset(cpu, core_mask)) {
+					++efx->rss_queues;
+					cpus_or(core_mask, core_mask,
+						topology_core_siblings(cpu));
+				}
+			}
+		} else {
+			efx->rss_queues = rss_cpus;
+		}
+
 		efx->rss_queues = min(efx->rss_queues, max_channel + 1);
 		efx->rss_queues = min(efx->rss_queues, EFX_MAX_CHANNELS);
 

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 3/4] sfc: resolve tx multiqueue bug

[Ben Hutchings]

Ben Hutchings wrote:
> With the recent changes to tx multiqueue, sfc was not calling
> netif_start_queue() before calling netif_wake_queue().
> This causes an oops when opening a device.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sfc/tx.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sfc/tx.c b/drivers/net/sfc/tx.c
> index 5cdd082..798b06b 100644
> --- a/drivers/net/sfc/tx.c
> +++ b/drivers/net/sfc/tx.c
> @@ -53,6 +53,7 @@ inline void efx_wake_queue(struct efx_nic *efx)
>  	if (atomic_dec_and_lock(&efx->netif_stop_count,
>  				&efx->netif_stop_lock)) {
>  		EFX_TRACE(efx, "waking TX queue\n");
> +		netif_start_queue(efx->net_dev);
>  		netif_wake_queue(efx->net_dev);
>  		spin_unlock(&efx->netif_stop_lock);
>  	}

Ahem... ignore this, it's obviously not right.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 3/4] sfc: resolve tx multiqueue bug

[Ben Hutchings]

The more I look at this issue of start vs wake, the more it seems like the
distinction should not be visible to drivers.

So long as a queue is only woken in response to TX completions, the current
arrangement is fine.  However, sfc needs to tear down and restart hardware TX
queues as part of some reconfiguration, self-test and recovery code, and I
doubt it's the only such driver.

I think the corresponding kernel queue must be stopped and then started when
we do this (otherwise we could return NETDEV_TX_BUSY in hard_start_xmit, but
I understand that to be deprecated).  So we must also wake the kernel queue
if and only if it's considered active - but we don't know whether that's the
case because deactivation is done asynchronously.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 3/4] sfc: resolve tx multiqueue bug

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 22 Jul 2008 19:41:19 +0100

> So long as a queue is only woken in response to TX completions, the current
> arrangement is fine.  However, sfc needs to tear down and restart hardware TX
> queues as part of some reconfiguration, self-test and recovery code, and I
> doubt it's the only such driver.

Mark the carrier off and packets will stop flowing to the driver.

Or, alternatively, take the necessary TX spinlocks around the
reconfiguration.

> I think the corresponding kernel queue must be stopped and then started when
> we do this (otherwise we could return NETDEV_TX_BUSY in hard_start_xmit, but
> I understand that to be deprecated).  So we must also wake the kernel queue
> if and only if it's considered active - but we don't know whether that's the
> case because deactivation is done asynchronously.

See above.

Re: [PATCH 3/4] sfc: resolve tx multiqueue bug

[Jeff Garzik]

David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 22 Jul 2008 19:41:19 +0100
> 
>> So long as a queue is only woken in response to TX completions, the current
>> arrangement is fine.  However, sfc needs to tear down and restart hardware TX
>> queues as part of some reconfiguration, self-test and recovery code, and I
>> doubt it's the only such driver.
> 
> Mark the carrier off and packets will stop flowing to the driver.

Yep.

Though based on looking at a lot of existing driver code, I think this 
is sometimes unclear to driver writers -- when to manage carrier 
(netif_carrier_on/off) and when to manage queue flow 
(netif_queue_start/stop/wake).

	Jeff

Re: [PATCH 3/4] sfc: resolve tx multiqueue bug

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Tue, 22 Jul 2008 17:29:59 -0400

> David Miller wrote:
> > From: Ben Hutchings <bhutchings@solarflare.com>
> > Date: Tue, 22 Jul 2008 19:41:19 +0100
> > 
> >> So long as a queue is only woken in response to TX completions, the current
> >> arrangement is fine.  However, sfc needs to tear down and restart hardware TX
> >> queues as part of some reconfiguration, self-test and recovery code, and I
> >> doubt it's the only such driver.
> > 
> > Mark the carrier off and packets will stop flowing to the driver.
> 
> Yep.
> 
> Though based on looking at a lot of existing driver code, I think this 
> is sometimes unclear to driver writers -- when to manage carrier 
> (netif_carrier_on/off) and when to manage queue flow 
> (netif_queue_start/stop/wake).

This is true.

My current plan is to fix up as many drivers as possible, and
assuming the reality that we can't expect to fix them all
by 2.6.27 we will remove the WARN_ON_ONCE from __netif_schedule()
right before 2.6.27-final then reinstate it for the 2.6.28
merge window.

Re: [PATCH 1/4] sfc: I2C adapter initialisation fixes

[Jeff Garzik]

Ben Hutchings wrote:
> As recommended by Jean Delvare:
> - Increase timeout to 50 ms
> - Leave adapter class clear so that unwanted drivers do not probe our bus
> - Use strlcpy() for name initialisation
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> Jeff,
> 
> These are exactly the differences between net-2.6 and the version of sfc I
> intended to submit to you before today.  If this doesn't apply then I'm
> confused as to what you have.

applied patches 1, 2, 4