[PATCH] spi subsystem: destroy the spi_bitbang workqueue only after the spi master is unregistered

[PATCH] spi subsystem: destroy the spi_bitbang workqueue only after the spi master is unregistered

[Chris Lesiak]

From: Chris Lesiak <chris.lesiak@licor.com>

This patch fixes a bug in the cleanup of an spi_bitbang bus.  The
workqueue associated with the bus was destroyed before the call to
spi_unregister_master.  That meant that spi devices on that bus would be
unable to do IO in their remove method.  The shutdown flag should have
been able to prevent a segfault, but was never getting set.  By waiting
to destroy the workqueue until after the master is unregistered, devices
are able to do IO in their remove methods.  An added benefit is that
neither the shutdown flag nor a wait for the queue of messages to empty
is needed.

Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>

---

This patch is against 2.6.19.2, but should apply cleanly to 2.6.20

--- linux-2.6.19.2/include/linux/spi/spi_bitbang.h.orig	2007-03-07 15:08:13.000000000 -0600
+++ linux-2.6.19.2/include/linux/spi/spi_bitbang.h	2007-03-07 15:08:27.000000000 -0600
@@ -25,7 +25,6 @@ struct spi_bitbang {
 	spinlock_t		lock;
 	struct list_head	queue;
 	u8			busy;
-	u8			shutdown;
 	u8			use_dma;
 
 	struct spi_master	*master;
--- linux-2.6.19.2/drivers/spi/spi_bitbang.c.orig	2007-03-07 15:03:42.000000000 -0600
+++ linux-2.6.19.2/drivers/spi/spi_bitbang.c	2007-03-07 15:14:10.000000000 -0600
@@ -301,10 +301,6 @@ static void bitbang_work(void *_bitbang)
 		setup_transfer = NULL;
 
 		list_for_each_entry (t, &m->transfers, transfer_list) {
-			if (bitbang->shutdown) {
-				status = -ESHUTDOWN;
-				break;
-			}
 
 			/* override or restore speed and wordsize */
 			if (t->speed_hz || t->bits_per_word) {
@@ -409,8 +405,6 @@ int spi_bitbang_transfer(struct spi_devi
 	m->status = -EINPROGRESS;
 
 	bitbang = spi_master_get_devdata(spi->master);
-	if (bitbang->shutdown)
-		return -ESHUTDOWN;
 
 	spin_lock_irqsave(&bitbang->lock, flags);
 	if (!spi->max_speed_hz)
@@ -505,28 +499,10 @@ EXPORT_SYMBOL_GPL(spi_bitbang_start);
  */
 int spi_bitbang_stop(struct spi_bitbang *bitbang)
 {
-	unsigned	limit = 500;
-
-	spin_lock_irq(&bitbang->lock);
-	bitbang->shutdown = 0;
-	while (!list_empty(&bitbang->queue) && limit--) {
-		spin_unlock_irq(&bitbang->lock);
-
-		dev_dbg(bitbang->master->cdev.dev, "wait for queue\n");
-		msleep(10);
-
-		spin_lock_irq(&bitbang->lock);
-	}
-	spin_unlock_irq(&bitbang->lock);
-	if (!list_empty(&bitbang->queue)) {
-		dev_err(bitbang->master->cdev.dev, "queue didn't empty\n");
-		return -EBUSY;
-	}
+	spi_unregister_master(bitbang->master);
 
 	destroy_workqueue(bitbang->workqueue);
 
-	spi_unregister_master(bitbang->master);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_bitbang_stop);

Re: [PATCH] spi subsystem: destroy the spi_bitbang workqueue only after the spi master is unregistered

[David Brownell]

On Wednesday 07 March 2007 3:48 pm, Chris Lesiak wrote:
> From: Chris Lesiak <chris.lesiak@licor.com>
> 
> This patch fixes a bug in the cleanup of an spi_bitbang bus. 

It's nearly right, but see below.


> @@ -505,28 +499,10 @@ EXPORT_SYMBOL_GPL(spi_bitbang_start);
>   */
>  int spi_bitbang_stop(struct spi_bitbang *bitbang)
>  {
> -	unsigned	limit = 500;
> -
> -	spin_lock_irq(&bitbang->lock);
> -	bitbang->shutdown = 0;
> -	while (!list_empty(&bitbang->queue) && limit--) {
> -		spin_unlock_irq(&bitbang->lock);
> -
> -		dev_dbg(bitbang->master->cdev.dev, "wait for queue\n");
> -		msleep(10);
> -
> -		spin_lock_irq(&bitbang->lock);
> -	}

You completely removed an odious busy-wait, which is good ...

> -	spin_unlock_irq(&bitbang->lock);
> -	if (!list_empty(&bitbang->queue)) {
> -		dev_err(bitbang->master->cdev.dev, "queue didn't empty\n");
> -		return -EBUSY;
> -	}
> +	spi_unregister_master(bitbang->master);

... but right here there should be a WARN_ON(!list_empty(...)) to
flag the corresponding bogosity:  that somehow a request never
completed.

>  
>  	destroy_workqueue(bitbang->workqueue);
>  
> -	spi_unregister_master(bitbang->master);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(spi_bitbang_stop);
> 
> 

Re: [PATCH] spi subsystem: destroy the spi_bitbang workqueue only after the spi master is unregistered

[Chris Lesiak]

From: Chris Lesiak <chris.lesiak@licor.com>

This patch fixes a bug in the cleanup of an spi_bitbang bus.  The
workqueue associated with the bus was destroyed before the call to
spi_unregister_master.  That meant that spi devices on that bus would be
unable to do IO in their remove method.  The shutdown flag should have
been able to prevent a segfault, but was never getting set.  By waiting
to destroy the workqueue until after the master is unregistered, devices
are able to do IO in their remove methods.  An added benefit is that
neither the shutdown flag nor a wait for the queue of messages to empty
is needed.

Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>

---

diff -uprN -X linux-2.6.20-vanilla/Documentation/dontdiff linux-2.6.20-vanilla/drivers/spi/spi_bitbang.c linux-2.6.20/drivers/spi/spi_bitbang.c
--- linux-2.6.20-vanilla/drivers/spi/spi_bitbang.c	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/drivers/spi/spi_bitbang.c	2007-03-09 11:23:42.000000000 -0600
@@ -302,10 +302,6 @@ static void bitbang_work(struct work_str
 		setup_transfer = NULL;
 
 		list_for_each_entry (t, &m->transfers, transfer_list) {
-			if (bitbang->shutdown) {
-				status = -ESHUTDOWN;
-				break;
-			}
 
 			/* override or restore speed and wordsize */
 			if (t->speed_hz || t->bits_per_word) {
@@ -410,8 +406,6 @@ int spi_bitbang_transfer(struct spi_devi
 	m->status = -EINPROGRESS;
 
 	bitbang = spi_master_get_devdata(spi->master);
-	if (bitbang->shutdown)
-		return -ESHUTDOWN;
 
 	spin_lock_irqsave(&bitbang->lock, flags);
 	if (!spi->max_speed_hz)
@@ -506,28 +500,12 @@ EXPORT_SYMBOL_GPL(spi_bitbang_start);
  */
 int spi_bitbang_stop(struct spi_bitbang *bitbang)
 {
-	unsigned	limit = 500;
-
-	spin_lock_irq(&bitbang->lock);
-	bitbang->shutdown = 0;
-	while (!list_empty(&bitbang->queue) && limit--) {
-		spin_unlock_irq(&bitbang->lock);
+	spi_unregister_master(bitbang->master);
 
-		dev_dbg(bitbang->master->cdev.dev, "wait for queue\n");
-		msleep(10);
-
-		spin_lock_irq(&bitbang->lock);
-	}
-	spin_unlock_irq(&bitbang->lock);
-	if (!list_empty(&bitbang->queue)) {
-		dev_err(bitbang->master->cdev.dev, "queue didn't empty\n");
-		return -EBUSY;
-	}
+	WARN_ON(!list_empty(&bitbang->queue));
 
 	destroy_workqueue(bitbang->workqueue);
 
-	spi_unregister_master(bitbang->master);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_bitbang_stop);
diff -uprN -X linux-2.6.20-vanilla/Documentation/dontdiff linux-2.6.20-vanilla/include/linux/spi/spi_bitbang.h linux-2.6.20/include/linux/spi/spi_bitbang.h
--- linux-2.6.20-vanilla/include/linux/spi/spi_bitbang.h	2007-02-04 12:44:54.000000000 -0600
+++ linux-2.6.20/include/linux/spi/spi_bitbang.h	2007-03-09 11:23:42.000000000 -0600
@@ -25,7 +25,6 @@ struct spi_bitbang {
 	spinlock_t		lock;
 	struct list_head	queue;
 	u8			busy;
-	u8			shutdown;
 	u8			use_dma;
 
 	struct spi_master	*master;