Commit c4e77376 authored by Stefani Seibold's avatar Stefani Seibold Committed by David Woodhouse

mtd: fix a huge latency problem in the MTD CFI and LPDDR flash drivers.

The use of a memcpy() during a spinlock operation will cause very long
thread context switch delays if the flash chip bandwidth is low and the
data to be copied large, because a spinlock will disable preemption.

For example: A flash with 6,5 MB/s bandwidth will cause under ubifs,
which request sometimes 128 KiB (the flash erase size), a preemption delay of
20 milliseconds. High priority threads will not be served during this
time, regardless whether this threads access the flash or not. This behavior
breaks real time.

The patch changes all the use of spin_lock operations for xxxx->mutex
into mutex operations, which is exact what the name says and means.

I have checked the code of the drivers and there is no use of atomic
pathes like interrupt or timers. The mtdoops facility will also not be used
by this drivers. So it is dave to replace the spin_lock against mutex.

There is no performance regression since the mutex is normally not
acquired.

Changelog:
 06.03.2010 First release
 26.03.2010 Fix mutex[1] issue and tested it for compile failure
Signed-off-by: default avatarStefani Seibold <stefani@seibold.net>
Signed-off-by: default avatarArtem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: default avatarDavid Woodhouse <David.Woodhouse@intel.com>
parent 67026418
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
...@@ -58,10 +58,10 @@ static int fwh_xxlock_oneblock(struct map_info *map, struct flchip *chip, ...@@ -58,10 +58,10 @@ static int fwh_xxlock_oneblock(struct map_info *map, struct flchip *chip,
* to flash memory - that means that we don't have to check status * to flash memory - that means that we don't have to check status
* and timeout. * and timeout.
*/ */
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, adr, FL_LOCKING); ret = get_chip(map, chip, adr, FL_LOCKING);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -72,7 +72,7 @@ static int fwh_xxlock_oneblock(struct map_info *map, struct flchip *chip, ...@@ -72,7 +72,7 @@ static int fwh_xxlock_oneblock(struct map_info *map, struct flchip *chip,
/* Done and happy. */ /* Done and happy. */
chip->state = chip->oldstate; chip->state = chip->oldstate;
put_chip(map, chip, adr); put_chip(map, chip, adr);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return 0; return 0;
} }
......
...@@ -155,8 +155,7 @@ static struct cfi_private *genprobe_ident_chips(struct map_info *map, struct chi ...@@ -155,8 +155,7 @@ static struct cfi_private *genprobe_ident_chips(struct map_info *map, struct chi
pchip->start = (i << cfi.chipshift); pchip->start = (i << cfi.chipshift);
pchip->state = FL_READY; pchip->state = FL_READY;
init_waitqueue_head(&pchip->wq); init_waitqueue_head(&pchip->wq);
spin_lock_init(&pchip->_spinlock); mutex_init(&pchip->mutex);
pchip->mutex = &pchip->_spinlock;
} }
} }
......
...@@ -106,8 +106,7 @@ struct mtd_info *lpddr_cmdset(struct map_info *map) ...@@ -106,8 +106,7 @@ struct mtd_info *lpddr_cmdset(struct map_info *map)
/* those should be reset too since /* those should be reset too since
they create memory references. */ they create memory references. */
init_waitqueue_head(&chip->wq); init_waitqueue_head(&chip->wq);
spin_lock_init(&chip->_spinlock); mutex_init(&chip->mutex);
chip->mutex = &chip->_spinlock;
chip++; chip++;
} }
} }
...@@ -143,7 +142,7 @@ static int wait_for_ready(struct map_info *map, struct flchip *chip, ...@@ -143,7 +142,7 @@ static int wait_for_ready(struct map_info *map, struct flchip *chip,
} }
/* OK Still waiting. Drop the lock, wait a while and retry. */ /* OK Still waiting. Drop the lock, wait a while and retry. */
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
if (sleep_time >= 1000000/HZ) { if (sleep_time >= 1000000/HZ) {
/* /*
* Half of the normal delay still remaining * Half of the normal delay still remaining
...@@ -158,17 +157,17 @@ static int wait_for_ready(struct map_info *map, struct flchip *chip, ...@@ -158,17 +157,17 @@ static int wait_for_ready(struct map_info *map, struct flchip *chip,
cond_resched(); cond_resched();
timeo--; timeo--;
} }
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
while (chip->state != chip_state) { while (chip->state != chip_state) {
/* Someone's suspended the operation: sleep */ /* Someone's suspended the operation: sleep */
DECLARE_WAITQUEUE(wait, current); DECLARE_WAITQUEUE(wait, current);
set_current_state(TASK_UNINTERRUPTIBLE); set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue(&chip->wq, &wait); add_wait_queue(&chip->wq, &wait);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
schedule(); schedule();
remove_wait_queue(&chip->wq, &wait); remove_wait_queue(&chip->wq, &wait);
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
} }
if (chip->erase_suspended || chip->write_suspended) { if (chip->erase_suspended || chip->write_suspended) {
/* Suspend has occured while sleep: reset timeout */ /* Suspend has occured while sleep: reset timeout */
...@@ -229,20 +228,20 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode) ...@@ -229,20 +228,20 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode)
* it'll happily send us to sleep. In any case, when * it'll happily send us to sleep. In any case, when
* get_chip returns success we're clear to go ahead. * get_chip returns success we're clear to go ahead.
*/ */
ret = spin_trylock(contender->mutex); ret = mutex_trylock(&contender->mutex);
spin_unlock(&shared->lock); spin_unlock(&shared->lock);
if (!ret) if (!ret)
goto retry; goto retry;
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
ret = chip_ready(map, contender, mode); ret = chip_ready(map, contender, mode);
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
if (ret == -EAGAIN) { if (ret == -EAGAIN) {
spin_unlock(contender->mutex); mutex_unlock(&contender->mutex);
goto retry; goto retry;
} }
if (ret) { if (ret) {
spin_unlock(contender->mutex); mutex_unlock(&contender->mutex);
return ret; return ret;
} }
spin_lock(&shared->lock); spin_lock(&shared->lock);
...@@ -251,10 +250,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode) ...@@ -251,10 +250,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode)
* state. Put contender and retry. */ * state. Put contender and retry. */
if (chip->state == FL_SYNCING) { if (chip->state == FL_SYNCING) {
put_chip(map, contender); put_chip(map, contender);
spin_unlock(contender->mutex); mutex_unlock(&contender->mutex);
goto retry; goto retry;
} }
spin_unlock(contender->mutex); mutex_unlock(&contender->mutex);
} }
/* Check if we have suspended erase on this chip. /* Check if we have suspended erase on this chip.
...@@ -264,10 +263,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode) ...@@ -264,10 +263,10 @@ static int get_chip(struct map_info *map, struct flchip *chip, int mode)
spin_unlock(&shared->lock); spin_unlock(&shared->lock);
set_current_state(TASK_UNINTERRUPTIBLE); set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue(&chip->wq, &wait); add_wait_queue(&chip->wq, &wait);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
schedule(); schedule();
remove_wait_queue(&chip->wq, &wait); remove_wait_queue(&chip->wq, &wait);
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
goto retry; goto retry;
} }
...@@ -336,10 +335,10 @@ static int chip_ready(struct map_info *map, struct flchip *chip, int mode) ...@@ -336,10 +335,10 @@ static int chip_ready(struct map_info *map, struct flchip *chip, int mode)
sleep: sleep:
set_current_state(TASK_UNINTERRUPTIBLE); set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue(&chip->wq, &wait); add_wait_queue(&chip->wq, &wait);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
schedule(); schedule();
remove_wait_queue(&chip->wq, &wait); remove_wait_queue(&chip->wq, &wait);
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
return -EAGAIN; return -EAGAIN;
} }
} }
...@@ -355,12 +354,12 @@ static void put_chip(struct map_info *map, struct flchip *chip) ...@@ -355,12 +354,12 @@ static void put_chip(struct map_info *map, struct flchip *chip)
if (shared->writing && shared->writing != chip) { if (shared->writing && shared->writing != chip) {
/* give back the ownership */ /* give back the ownership */
struct flchip *loaner = shared->writing; struct flchip *loaner = shared->writing;
spin_lock(loaner->mutex); mutex_lock(&loaner->mutex);
spin_unlock(&shared->lock); spin_unlock(&shared->lock);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
put_chip(map, loaner); put_chip(map, loaner);
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
spin_unlock(loaner->mutex); mutex_unlock(&loaner->mutex);
wake_up(&chip->wq); wake_up(&chip->wq);
return; return;
} }
...@@ -413,10 +412,10 @@ int do_write_buffer(struct map_info *map, struct flchip *chip, ...@@ -413,10 +412,10 @@ int do_write_buffer(struct map_info *map, struct flchip *chip,
wbufsize = 1 << lpddr->qinfo->BufSizeShift; wbufsize = 1 << lpddr->qinfo->BufSizeShift;
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_WRITING); ret = get_chip(map, chip, FL_WRITING);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
/* Figure out the number of words to write */ /* Figure out the number of words to write */
...@@ -477,7 +476,7 @@ int do_write_buffer(struct map_info *map, struct flchip *chip, ...@@ -477,7 +476,7 @@ int do_write_buffer(struct map_info *map, struct flchip *chip,
} }
out: put_chip(map, chip); out: put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -489,10 +488,10 @@ int do_erase_oneblock(struct mtd_info *mtd, loff_t adr) ...@@ -489,10 +488,10 @@ int do_erase_oneblock(struct mtd_info *mtd, loff_t adr)
struct flchip *chip = &lpddr->chips[chipnum]; struct flchip *chip = &lpddr->chips[chipnum];
int ret; int ret;
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_ERASING); ret = get_chip(map, chip, FL_ERASING);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
send_pfow_command(map, LPDDR_BLOCK_ERASE, adr, 0, NULL); send_pfow_command(map, LPDDR_BLOCK_ERASE, adr, 0, NULL);
...@@ -504,7 +503,7 @@ int do_erase_oneblock(struct mtd_info *mtd, loff_t adr) ...@@ -504,7 +503,7 @@ int do_erase_oneblock(struct mtd_info *mtd, loff_t adr)
goto out; goto out;
} }
out: put_chip(map, chip); out: put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -517,10 +516,10 @@ static int lpddr_read(struct mtd_info *mtd, loff_t adr, size_t len, ...@@ -517,10 +516,10 @@ static int lpddr_read(struct mtd_info *mtd, loff_t adr, size_t len,
struct flchip *chip = &lpddr->chips[chipnum]; struct flchip *chip = &lpddr->chips[chipnum];
int ret = 0; int ret = 0;
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_READY); ret = get_chip(map, chip, FL_READY);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -528,7 +527,7 @@ static int lpddr_read(struct mtd_info *mtd, loff_t adr, size_t len, ...@@ -528,7 +527,7 @@ static int lpddr_read(struct mtd_info *mtd, loff_t adr, size_t len,
*retlen = len; *retlen = len;
put_chip(map, chip); put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -568,9 +567,9 @@ static int lpddr_point(struct mtd_info *mtd, loff_t adr, size_t len, ...@@ -568,9 +567,9 @@ static int lpddr_point(struct mtd_info *mtd, loff_t adr, size_t len,
else else
thislen = len; thislen = len;
/* get the chip */ /* get the chip */
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_POINT); ret = get_chip(map, chip, FL_POINT);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
if (ret) if (ret)
break; break;
...@@ -610,7 +609,7 @@ static void lpddr_unpoint (struct mtd_info *mtd, loff_t adr, size_t len) ...@@ -610,7 +609,7 @@ static void lpddr_unpoint (struct mtd_info *mtd, loff_t adr, size_t len)
else else
thislen = len; thislen = len;
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
if (chip->state == FL_POINT) { if (chip->state == FL_POINT) {
chip->ref_point_counter--; chip->ref_point_counter--;
if (chip->ref_point_counter == 0) if (chip->ref_point_counter == 0)
...@@ -620,7 +619,7 @@ static void lpddr_unpoint (struct mtd_info *mtd, loff_t adr, size_t len) ...@@ -620,7 +619,7 @@ static void lpddr_unpoint (struct mtd_info *mtd, loff_t adr, size_t len)
"pointed region\n", map->name); "pointed region\n", map->name);
put_chip(map, chip); put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
len -= thislen; len -= thislen;
ofs = 0; ofs = 0;
...@@ -726,10 +725,10 @@ int do_xxlock(struct mtd_info *mtd, loff_t adr, uint32_t len, int thunk) ...@@ -726,10 +725,10 @@ int do_xxlock(struct mtd_info *mtd, loff_t adr, uint32_t len, int thunk)
int chipnum = adr >> lpddr->chipshift; int chipnum = adr >> lpddr->chipshift;
struct flchip *chip = &lpddr->chips[chipnum]; struct flchip *chip = &lpddr->chips[chipnum];
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_LOCKING); ret = get_chip(map, chip, FL_LOCKING);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -749,7 +748,7 @@ int do_xxlock(struct mtd_info *mtd, loff_t adr, uint32_t len, int thunk) ...@@ -749,7 +748,7 @@ int do_xxlock(struct mtd_info *mtd, loff_t adr, uint32_t len, int thunk)
goto out; goto out;
} }
out: put_chip(map, chip); out: put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -770,10 +769,10 @@ int word_program(struct map_info *map, loff_t adr, uint32_t curval) ...@@ -770,10 +769,10 @@ int word_program(struct map_info *map, loff_t adr, uint32_t curval)
int chipnum = adr >> lpddr->chipshift; int chipnum = adr >> lpddr->chipshift;
struct flchip *chip = &lpddr->chips[chipnum]; struct flchip *chip = &lpddr->chips[chipnum];
spin_lock(chip->mutex); mutex_lock(&chip->mutex);
ret = get_chip(map, chip, FL_WRITING); ret = get_chip(map, chip, FL_WRITING);
if (ret) { if (ret) {
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
...@@ -787,7 +786,7 @@ int word_program(struct map_info *map, loff_t adr, uint32_t curval) ...@@ -787,7 +786,7 @@ int word_program(struct map_info *map, loff_t adr, uint32_t curval)
} }
out: put_chip(map, chip); out: put_chip(map, chip);
spin_unlock(chip->mutex); mutex_unlock(&chip->mutex);
return ret; return ret;
} }
......
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
* has asm/spinlock.h, or 2.4, which has linux/spinlock.h * has asm/spinlock.h, or 2.4, which has linux/spinlock.h
*/ */
#include <linux/sched.h> #include <linux/sched.h>
#include <linux/mutex.h>
typedef enum { typedef enum {
FL_READY, FL_READY,
...@@ -74,8 +75,7 @@ struct flchip { ...@@ -74,8 +75,7 @@ struct flchip {
unsigned int erase_suspended:1; unsigned int erase_suspended:1;
unsigned long in_progress_block_addr; unsigned long in_progress_block_addr;
spinlock_t *mutex; struct mutex mutex;
spinlock_t _spinlock; /* We do it like this because sometimes they'll be shared. */
wait_queue_head_t wq; /* Wait on here when we're waiting for the chip wait_queue_head_t wq; /* Wait on here when we're waiting for the chip
to be ready */ to be ready */
int word_write_time; int word_write_time;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment