Skip to contentSkip to conversation

pxa2xx_spi: Fix race condition in stop_queue()

PATCH New Vasily Khoruzhick 5 participants 14 messages Mar 13, 2011 – Apr 7, 2011 1 message not in the archive SPI

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

There's a race condition in stop_queue(),
if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

---
 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/spi/pxa2xx_spi.c +1 -1
index a429b01..3aa7820 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
 	drv_data->run = QUEUE_STOPPED;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
Signature · 1 line
1.7.4.1
#1 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 5 lines · On Monday 14 March 2011 00:27:10 Vasily Khoruzhick wrote:

There's a race condition in stop_queue(),
if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

Ping?

Quoted 18 lines
---
 drivers/spi/pxa2xx_spi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/spi/pxa2xx_spi.c +1 -1
index a429b01..3aa7820 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
 	drv_data->run = QUEUE_STOPPED;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
#2 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

In reply to Vasily Khoruzhick
Quoted 5 lines · Vasily Khoruzhick

There's a race condition in stop_queue(),
if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

I think this is a good catch if the queue could be stopped only when
1) queue is empty AND 2) transfer is not in progress.

There are several other places you might need to change accordingly, e.g. spi_bfin5xx.c

Quoted 23 lines

---
?drivers/spi/pxa2xx_spi.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/spi/pxa2xx_spi.c +0 -0
index a429b01..3aa7820 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)

? ? ? ? * execution path (pump_messages) would be required to call wake_up or ? ? ? ? * friends on every SPI message. Do this instead */
? ? ? ?drv_data->run = QUEUE_STOPPED;
- ? ? ? while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+ ? ? ? while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) { ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&drv_data->lock, flags);
? ? ? ? ? ? ? ?msleep(10);
? ? ? ? ? ? ? ?spin_lock_irqsave(&drv_data->lock, flags);
--
1.7.4.1

#3 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 1 line · On Friday 01 April 2011 10:26:37 Eric Miao wrote:

On Mon, Mar 14, 2011 at 6:27 AM, Vasily Khoruzhick <[email protected]>

Quoted 8 lines · wrote:
Quoted 5 lines

There's a race condition in stop_queue(),
if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

I think this is a good catch if the queue could be stopped only when
1) queue is empty AND 2) transfer is not in progress.

It has check later (under while):

if (!list_empty(&drv_data->queue) || drv_data->busy)

	status = -EBUSY;

And I'm hitting it on Z2 when I'm trying to enter suspend - it fails, because spi driver refuses to enter suspend.

Quoted 2 lines

There are several other places you might need to change accordingly, e.g. spi_bfin5xx.c

I can make a patch for other drivers, but I can't test it - I have no appropriate boards, and even toolchain.

Regards
Vasily

#4 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 16 lines · Vasily Khoruzhick
Quoted 9 lines · On Friday 01 April 2011 10:26:37 Eric Miao wrote:
Quoted 5 lines · Vasily Khoruzhick

There's a race condition in stop_queue(),
if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

I think this is a good catch if the queue could be stopped only when
1) queue is empty AND 2) transfer is not in progress.

It has check later (under while):

if (!list_empty(&drv_data->queue) || drv_data->busy) ? ? ? ?status = -EBUSY;

Quoted 8 lines

And I'm hitting it on Z2 when I'm trying to enter suspend - it fails, because spi driver refuses to enter suspend.

Quoted 2 lines

There are several other places you might need to change accordingly, e.g. spi_bfin5xx.c

I can make a patch for other drivers, but I can't test it - I have no appropriate boards, and even toolchain.

that's fine, i do. was this patch actually merged ? or is there a new version on the way ?
-mike

#5 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 3 lines · On Monday 04 April 2011 20:35:14 Mike Frysinger wrote:

that's fine, i do. was this patch actually merged ? or is there a new version on the way ?
-mike

I suspect it's not merged yet. Btw, should I make a patch which fixes all drivers, or it's prefferable to make one patch per driver?

Regards
Vasily

#6 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 9 lines · Vasily Khoruzhick
Quoted 3 lines · On Monday 04 April 2011 20:35:14 Mike Frysinger wrote:

that's fine, i do. ?was this patch actually merged ? ?or is there a new version on the way ?
-mike

I suspect it's not merged yet. Btw, should I make a patch which fixes all drivers, or it's prefferable to make one patch per driver?

Please come up with one patch, Mike can help add a Tested-by then.

#7 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

Quoted 1 line · On Tuesday 05 April 2011 06:21:27 Eric Miao wrote:

On Tue, Apr 5, 2011 at 1:47 AM, Vasily Khoruzhick <[email protected]>

Quoted 9 lines · wrote:
Quoted 7 lines · On Monday 04 April 2011 20:35:14 Mike Frysinger wrote:
Quoted 3 lines

that's fine, i do. was this patch actually merged ? or is there a new version on the way ?
-mike

I suspect it's not merged yet. Btw, should I make a patch which fixes all drivers, or it's prefferable to make one patch per driver?

Please come up with one patch, Mike can help add a Tested-by then.

Sorry, but I did not get if you want splitted patch or not :) Anyway, 4 on- line-patches does not look reasonable anyway, so I'm sending not-splitted patch in few minutes.

#8 lore raw

PATCH 1/1 spi: Fix race condition in stop_queue()

There's a race condition in stop_queue() in some drivers - if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

This patch affects following drivers:
pxa2xx_spi
spi_bfin5xx
amba-pl022
dw_spi

---
 drivers/spi/amba-pl022.c  |    2 +-
 drivers/spi/dw_spi.c      |    2 +-
 drivers/spi/pxa2xx_spi.c  |    2 +-
 drivers/spi/spi_bfin5xx.c |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/spi/amba-pl022.c +1 -1
index 71a1219..23aaaa9 100644
@@ -1553,7 +1553,7 @@ static int stop_queue(struct pl022 *pl022)
 	 * A wait_queue on the pl022->busy could be used, but then the common
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
-	while (!list_empty(&pl022->queue) && pl022->busy && limit--) {
+	while ((!list_empty(&pl022->queue) || pl022->busy) && limit--) {
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&pl022->queue_lock, flags);
drivers/spi/dw_spi.c +1 -1
index 22af77f..b53be4d 100644
@@ -821,7 +821,7 @@ static int stop_queue(struct dw_spi *dws)
 
 	spin_lock_irqsave(&dws->lock, flags);
 	dws->run = QUEUE_STOPPED;
-	while (!list_empty(&dws->queue) && dws->busy && limit--) {
+	while ((!list_empty(&dws->queue) || dws->busy) && limit--) {
 		spin_unlock_irqrestore(&dws->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&dws->lock, flags);
drivers/spi/pxa2xx_spi.c +1 -1
index 9592883..39cc10f 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
 	drv_data->run = QUEUE_STOPPED;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
drivers/spi/spi_bfin5xx.c +1 -1
index 3f22351..0e14f40 100644
@@ -1244,7 +1244,7 @@ static inline int bfin_spi_stop_queue(struct bfin_spi_master_data *drv_data)
 	 * friends on every SPI message. Do this instead
 	 */
 	drv_data->running = false;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
Signature · 1 line
1.7.4.1
#9 lore raw

PATCH 1/1 spi: Fix race condition in stop_queue()

Quoted 6 lines · Vasily Khoruzhick

There's a race condition in stop_queue() in some drivers - if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

Quoted 7 lines

This patch affects following drivers:
pxa2xx_spi
spi_bfin5xx
amba-pl022
dw_spi

Quoted 63 lines

---

?drivers/spi/amba-pl022.c ?| ? ?2 +-
?drivers/spi/dw_spi.c ? ? ?| ? ?2 +-
?drivers/spi/pxa2xx_spi.c ?| ? ?2 +-
?drivers/spi/spi_bfin5xx.c | ? ?2 +-

?4 files changed, 4 insertions(+), 4 deletions(-)

drivers/spi/amba-pl022.c +0 -0
index 71a1219..23aaaa9 100644
@@ -1553,7 +1553,7 @@ static int stop_queue(struct pl022 *pl022)

? ? ? ? * A wait_queue on the pl022->busy could be used, but then the common ? ? ? ? * execution path (pump_messages) would be required to call wake_up or ? ? ? ? * friends on every SPI message. Do this instead */
- ? ? ? while (!list_empty(&pl022->queue) && pl022->busy && limit--) {
+ ? ? ? while ((!list_empty(&pl022->queue) || pl022->busy) && limit--) { ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&pl022->queue_lock, flags); ? ? ? ? ? ? ? ?msleep(10);
? ? ? ? ? ? ? ?spin_lock_irqsave(&pl022->queue_lock, flags);

drivers/spi/dw_spi.c +1 -1
index 22af77f..b53be4d 100644
@@ -821,7 +821,7 @@ static int stop_queue(struct dw_spi *dws)
 
 ? ? ? ?spin_lock_irqsave(&dws->lock, flags);
 ? ? ? ?dws->run = QUEUE_STOPPED;
- ? ? ? while (!list_empty(&dws->queue) && dws->busy && limit--) {
+ ? ? ? while ((!list_empty(&dws->queue) || dws->busy) && limit--) {
 ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&dws->lock, flags);
 ? ? ? ? ? ? ? ?msleep(10);
 ? ? ? ? ? ? ? ?spin_lock_irqsave(&dws->lock, flags);
drivers/spi/pxa2xx_spi.c +0 -0
index 9592883..39cc10f 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)

? ? ? ? * execution path (pump_messages) would be required to call wake_up or ? ? ? ? * friends on every SPI message. Do this instead */
? ? ? ?drv_data->run = QUEUE_STOPPED;
- ? ? ? while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+ ? ? ? while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) { ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&drv_data->lock, flags);
? ? ? ? ? ? ? ?msleep(10);
? ? ? ? ? ? ? ?spin_lock_irqsave(&drv_data->lock, flags);

drivers/spi/spi_bfin5xx.c +0 -0
index 3f22351..0e14f40 100644
@@ -1244,7 +1244,7 @@ static inline int bfin_spi_stop_queue(struct bfin_spi_master_data *drv_data)

? ? ? ? * friends on every SPI message. Do this instead
? ? ? ? */
? ? ? ?drv_data->running = false;
- ? ? ? while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+ ? ? ? while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) { ? ? ? ? ? ? ? ?spin_unlock_irqrestore(&drv_data->lock, flags);
? ? ? ? ? ? ? ?msleep(10);
? ? ? ? ? ? ? ?spin_lock_irqsave(&drv_data->lock, flags);
--
1.7.4.1

Applied
#11 lore raw

PATCH 1/1 spi: Fix race condition in stop_queue()

In reply to Vasily Khoruzhick
Quoted 6 lines · Vasily Khoruzhick

There's a race condition in stop_queue() in some drivers - if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

Quoted 7 lines

This patch affects following drivers:
pxa2xx_spi
spi_bfin5xx
amba-pl022
dw_spi

Applied, thanks.

g.

Quoted 67 lines
---
 drivers/spi/amba-pl022.c  |    2 +-
 drivers/spi/dw_spi.c      |    2 +-
 drivers/spi/pxa2xx_spi.c  |    2 +-
 drivers/spi/spi_bfin5xx.c |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/spi/amba-pl022.c +1 -1
index 71a1219..23aaaa9 100644
@@ -1553,7 +1553,7 @@ static int stop_queue(struct pl022 *pl022)
 	 * A wait_queue on the pl022->busy could be used, but then the common
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
-	while (!list_empty(&pl022->queue) && pl022->busy && limit--) {
+	while ((!list_empty(&pl022->queue) || pl022->busy) && limit--) {
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&pl022->queue_lock, flags);
drivers/spi/dw_spi.c +1 -1
index 22af77f..b53be4d 100644
@@ -821,7 +821,7 @@ static int stop_queue(struct dw_spi *dws)
 
 	spin_lock_irqsave(&dws->lock, flags);
 	dws->run = QUEUE_STOPPED;
-	while (!list_empty(&dws->queue) && dws->busy && limit--) {
+	while ((!list_empty(&dws->queue) || dws->busy) && limit--) {
 		spin_unlock_irqrestore(&dws->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&dws->lock, flags);
drivers/spi/pxa2xx_spi.c +1 -1
index 9592883..39cc10f 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
 	drv_data->run = QUEUE_STOPPED;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
drivers/spi/spi_bfin5xx.c +1 -1
index 3f22351..0e14f40 100644
@@ -1244,7 +1244,7 @@ static inline int bfin_spi_stop_queue(struct bfin_spi_master_data *drv_data)
 	 * friends on every SPI message. Do this instead
 	 */
 	drv_data->running = false;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
Signature · 7 lines
1.7.4.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

A message that is not in the archive (sent off-list or not yet received): 201104061746.44134.anarsoul-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#13 lore raw

PATCH 1/1 spi: Fix race condition in stop_queue()

There's a race condition in stop_queue() in some drivers - if drv_data->queue is empty, but drv_data->busy is still set (or opposite situation) stop_queue will return -EBUSY. So fix loop condition to check that both drv_data->queue is empty and drv_data->busy is not set.

This patch affects following drivers:
pxa2xx_spi
spi_bfin5xx
amba-pl022
dw_spi

---
 drivers/spi/amba-pl022.c  |    2 +-
 drivers/spi/dw_spi.c      |    2 +-
 drivers/spi/pxa2xx_spi.c  |    2 +-
 drivers/spi/spi_bfin5xx.c |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
drivers/spi/amba-pl022.c +1 -1
index 71a1219..23aaaa9 100644
@@ -1553,7 +1553,7 @@ static int stop_queue(struct pl022 *pl022)
 	 * A wait_queue on the pl022->busy could be used, but then the common
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
-	while (!list_empty(&pl022->queue) && pl022->busy && limit--) {
+	while ((!list_empty(&pl022->queue) || pl022->busy) && limit--) {
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&pl022->queue_lock, flags);
drivers/spi/dw_spi.c +1 -1
index 22af77f..b53be4d 100644
@@ -821,7 +821,7 @@ static int stop_queue(struct dw_spi *dws)
 
 	spin_lock_irqsave(&dws->lock, flags);
 	dws->run = QUEUE_STOPPED;
-	while (!list_empty(&dws->queue) && dws->busy && limit--) {
+	while ((!list_empty(&dws->queue) || dws->busy) && limit--) {
 		spin_unlock_irqrestore(&dws->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&dws->lock, flags);
drivers/spi/pxa2xx_spi.c +1 -1
index 9592883..39cc10f 100644
@@ -1493,7 +1493,7 @@ static int stop_queue(struct driver_data *drv_data)
 	 * execution path (pump_messages) would be required to call wake_up or
 	 * friends on every SPI message. Do this instead */
 	drv_data->run = QUEUE_STOPPED;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
drivers/spi/spi_bfin5xx.c +1 -1
index 3f22351..0e14f40 100644
@@ -1244,7 +1244,7 @@ static inline int bfin_spi_stop_queue(struct bfin_spi_master_data *drv_data)
 	 * friends on every SPI message. Do this instead
 	 */
 	drv_data->running = false;
-	while (!list_empty(&drv_data->queue) && drv_data->busy && limit--) {
+	while ((!list_empty(&drv_data->queue) || drv_data->busy) && limit--) {
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 		msleep(10);
 		spin_lock_irqsave(&drv_data->lock, flags);
Signature · 9 lines
1.7.4.1


------------------------------------------------------------------------------
Xperia(TM) PLAY
It's a major breakthrough. An authentic gaming
smartphone on the nation's most reliable network.
And it wants your games.
http://p.sf.net/sfu/verizon-sfdev
#14 lore raw

PATCH 1/1 pxa2xx_spi: Fix race condition in stop_queue()

In reply to Vasily Khoruzhick
Quoted 7 lines · Vasily Khoruzhick
Quoted 6 lines · On Tuesday 05 April 2011 06:21:27 Eric Miao wrote:
Quoted 5 lines · Vasily Khoruzhick
Quoted 3 lines · On Monday 04 April 2011 20:35:14 Mike Frysinger wrote:

that's fine, i do. ?was this patch actually merged ? ?or is there a new version on the way ?
-mike

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

I suspect it's not merged yet. Btw, should I make a patch which fixes all drivers, or it's prefferable to make one patch per driver?

Please come up with one patch, Mike can help add a Tested-by then.

Sorry, but I did not get if you want splitted patch or not :) Anyway, 4 on- line-patches does not look reasonable anyway, so I'm sending not-splitted patch in few minutes.

yes, that is fine. thanks :).
-mike