mtd/chips: add missing set_current_state() to cfi_{amdstd,staa}_sync()

PATCH In review Dmitry Adamushko 2 participants 3 messages Apr 3, 2008 – Apr 8, 2008 Memory technology devices (MTD)

Share & tools

Dmitry Adamushko

author

2008-04-03 19:38 UTC #0 lore raw

PATCH 1/1 mtd/chips: add missing set_current_state() to cfi_{amdstd,staa}_sync()

From: Dmitry Adamushko <[email protected]>
Subject: [mtd/chips] add missing set_current_state() to cfi_{amdstd,staa}_sync()

cfi_amdstd_sync() and cfi_staa_sync() call schedule() without changing task's state appropriately.

In case of e.g. chip->state == FL_ERASING, cfi_*_sync() will be busy-looping either redundantly for a fixed interval of time (for SCHED_NORMAL tasks) or possibly endlessly (for RT tasks and UP).

Signed-off-by Dmitry Adamushko <[email protected]>

---

drivers/mtd/chips/cfi_cmdset_0002.c +1 -0

index d072e87..458d477 100644
@@ -1763,6 +1763,7 @@ static void cfi_amdstd_sync (struct mtd_info *mtd)
 
 		default:
 			/* Not an idle state */
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			add_wait_queue(&chip->wq, &wait);
 
 			spin_unlock(chip->mutex);

drivers/mtd/chips/cfi_cmdset_0020.c +1 -0

index b344ff8..492e2ab 100644
@@ -1015,6 +1015,7 @@ static void cfi_staa_sync (struct mtd_info *mtd)
 
 		default:
 			/* Not an idle state */
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			add_wait_queue(&chip->wq, &wait);
 
 			spin_unlock_bh(chip->mutex);

Andrew Morton

2008-04-07 23:16 UTC #1 lore raw

Quoted 31 lines · On Thu, 03 Apr 2008 21:38:23 +0200 Dmitry Adamushko…

From: Dmitry Adamushko <[email protected]>
Subject: [mtd/chips] add missing set_current_state() to cfi_{amdstd,staa}_sync()
cfi_amdstd_sync() and cfi_staa_sync() call schedule() without changing task's state appropriately.
In case of e.g. chip->state == FL_ERASING, cfi_*_sync() will be busy-looping either redundantly for a fixed interval of time (for SCHED_NORMAL tasks) or possibly endlessly (for RT tasks and UP).
Signed-off-by Dmitry Adamushko <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
index d072e87..458d477 100644
@@ -1763,6 +1763,7 @@ static void cfi_amdstd_sync (struct mtd_info *mtd)
 
 		default:
 			/* Not an idle state */
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			add_wait_queue(&chip->wq, &wait);
 
 			spin_unlock(chip->mutex);
drivers/mtd/chips/cfi_cmdset_0020.c +0 -0
index b344ff8..492e2ab 100644

drivers/mtd/chips/cfi_cmdset_0020.c static void cfi_staa_sync (struct mtd_info *mtd) · line 1021

Quoted 7 lines

drivers/mtd/chips/cfi_cmdset_0020.c +1 -0

@@ -1015,6 +1015,7 @@ static void cfi_staa_sync (struct mtd_info *mtd)
 
 		default:
 			/* Not an idle state */
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			add_wait_queue(&chip->wq, &wait);
 
 			spin_unlock_bh(chip->mutex);

The change certainly looks correct. Has it been runtime tested?

Thanks.

Dmitry Adamushko

author

2008-04-08 08:21 UTC #2 lore raw

Quoted 40 lines · Andrew Morton

Quoted 37 lines · On Thu, 03 Apr 2008 21:38:23 +0200 Dmitry Adamushko…
From: Dmitry Adamushko <[email protected]>
Subject: [mtd/chips] add missing set_current_state() to cfi_{amdstd,staa}_sync()
cfi_amdstd_sync() and cfi_staa_sync() call schedule() without changing task's state appropriately.
In case of e.g. chip->state == FL_ERASING, cfi_*_sync() will be busy-looping either redundantly for a fixed interval of time (for SCHED_NORMAL tasks) or possibly endlessly (for RT tasks and UP).
Signed-off-by Dmitry Adamushko <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
index d072e87..458d477 100644
@@ -1763,6 +1763,7 @@ static void cfi_amdstd_sync (struct mtd_info *mtd)
 
              default:
                      /* Not an idle state */
+                     set_current_state(TASK_UNINTERRUPTIBLE);
                      add_wait_queue(&chip->wq, &wait);
 
                      spin_unlock(chip->mutex);
drivers/mtd/chips/cfi_cmdset_0020.c +1 -0
index b344ff8..492e2ab 100644
@@ -1015,6 +1015,7 @@ static void cfi_staa_sync (struct mtd_info *mtd)
 
              default:
                      /* Not an idle state */
+                     set_current_state(TASK_UNINTERRUPTIBLE);
                      add_wait_queue(&chip->wq, &wait);
 

Quoted 4 lines

drivers/mtd/chips/cfi_cmdset_0020.c static void cfi_staa_sync (struct mtd_info *mtd) · line 1021
Quoted 1 line
drivers/mtd/chips/cfi_cmdset_0020.c +0 -0
                      spin_unlock_bh(chip->mutex);
The change certainly looks correct. Has it been runtime tested?

It has been tested with an oldish 2.6.8.1 where the problem initially occured. It was a RT task that happened to close an mtd fd (and resulting in ->sync() being called) and ran in the middle of the ->erase op... resulting in a "nice" endless loop.

The versions of cfi_{amdstd,staa}_sync() in the mainline look not that much different from the respective versions in 2.6.8.1.

I guess, it went unnoticed for so long time due to :

(1) apps. don't often directly open/close() fd for mtd partitions;
(2) a race against ->erase() (or smth else) is rare;
(3) if (1) is not true, then an app. is still unlikely to be RT (in which case, a task just loops until its timeslice is gone).

Quoted 3 lines

Thanks.

Signature · 2 lines

Best regards,
Dmitry Adamushko