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

PATCH In review Chris Lesiak 2 participants 3 messages Mar 8, 2007 – Mar 9, 2007 2 messages not in the archive SPI

lore

Share & tools

lore

Chris Lesiak

author

2007-03-08 00:03 UTC #2 lore raw

PATCH 1/1 spi subsystem: destroy the spi_bitbang workqueue only after the spi master is unregistered

In reply to a message not in the archive

From: Chris Lesiak <[email protected]>

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 <[email protected]>

---

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

include/linux/spi/spi_bitbang.h renamed from include/linux/spi/spi_bitbang.h.orig +0 -1

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

drivers/spi/spi_bitbang.c renamed from drivers/spi/spi_bitbang.c.orig +1 ~~-25~~

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

David Brownell

2007-03-09 06:43 UTC #3 lore raw

Quoted 3 lines · Chris Lesiak

From: Chris Lesiak <[email protected]>
This patch fixes a bug in the cleanup of an spi_bitbang bus.

It's nearly right, but see below.

Quoted 6 lines

drivers/spi/spi_bitbang.c renamed from drivers/spi/spi_bitbang.c.orig +0 -2

@@ -505,28 +499,10 @@ EXPORT_SYMBOL_GPL(spi_bitbang_start);
  */
 int spi_bitbang_stop(struct spi_bitbang *bitbang)
 {
-	unsigned	limit = 500;
-

drivers/spi/spi_bitbang.c EXPORT_SYMBOL_GPL(spi_bitbang_start); · line 519

Quoted 10 lines

drivers/spi/spi_bitbang.c renamed from drivers/spi/spi_bitbang.c.orig +0 ~~-10~~

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

drivers/spi/spi_bitbang.c EXPORT_SYMBOL_GPL(spi_bitbang_start); · line 502

Quoted 6 lines

drivers/spi/spi_bitbang.c renamed from drivers/spi/spi_bitbang.c.orig +1 -5

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

Quoted 10 lines

drivers/spi/spi_bitbang.c renamed from drivers/spi/spi_bitbang.c.orig +0 -2

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

Chris Lesiak

author

2007-03-09 17:32 UTC #4 lore raw

From: Chris Lesiak <[email protected]>

Signed-off-by Chris Lesiak <[email protected]>

---

drivers/spi/spi_bitbang.c +2 ~~-24~~

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

include/linux/spi/spi_bitbang.h +0 -1

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