Skip to content

Commit 013e629

Browse files
Boris Brezillonmiquelraynal
authored andcommitted
mtd: rawnand: Simplify the locking
nand_get_device() was complex for apparently no good reason. Let's replace this locking scheme with 2 mutexes: one attached to the controller and another one attached to the chip. Every time the core calls nand_get_device(), it will first lock the chip and if the chip is not suspended, will then lock the controller. nand_release_device() will release both lock in the reverse order. nand_get_device() can sleep, just like the previous implementation, which means you should never call that from an atomic context. We also get rid of - the chip->state field, since all it was used for was flagging the chip as suspended. We replace it by a field called chip->suspended and directly set it from nand_suspend/resume() - the controller->wq and controller->active fields which are no longer needed since the new controller->lock (now a mutex) guarantees that all operations are serialized at the controller level - panic_nand_get_device() which would anyway be a no-op. Talking about panic write, I keep thinking the rawnand implementation is unsafe because there's not negotiation with the controller to know when it's actually done with it's previous operation. I don't intend to fix that here, but that's probably something we should look at, or maybe we should consider dropping the ->_panic_write() implementation Last important change to mention: we now return -EBUSY when someone tries to access a device that as been suspended, and propagate this error to the upper layer. Signed-off-by: Boris Brezillon <[email protected]> Signed-off-by: Miquel Raynal <[email protected]>
1 parent 661803b commit 013e629

File tree

2 files changed

+54
-81
lines changed

2 files changed

+54
-81
lines changed

drivers/mtd/nand/raw/nand_base.c

Lines changed: 45 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -278,11 +278,8 @@ EXPORT_SYMBOL_GPL(nand_deselect_target);
278278
static void nand_release_device(struct nand_chip *chip)
279279
{
280280
/* Release the controller and the chip */
281-
spin_lock(&chip->controller->lock);
282-
chip->controller->active = NULL;
283-
chip->state = FL_READY;
284-
wake_up(&chip->controller->wq);
285-
spin_unlock(&chip->controller->lock);
281+
mutex_unlock(&chip->controller->lock);
282+
mutex_unlock(&chip->lock);
286283
}
287284

288285
/**
@@ -330,58 +327,24 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
330327
return nand_block_bad(chip, ofs);
331328
}
332329

333-
/**
334-
* panic_nand_get_device - [GENERIC] Get chip for selected access
335-
* @chip: the nand chip descriptor
336-
* @new_state: the state which is requested
337-
*
338-
* Used when in panic, no locks are taken.
339-
*/
340-
static void panic_nand_get_device(struct nand_chip *chip, int new_state)
341-
{
342-
/* Hardware controller shared among independent devices */
343-
chip->controller->active = chip;
344-
chip->state = new_state;
345-
}
346-
347330
/**
348331
* nand_get_device - [GENERIC] Get chip for selected access
349332
* @chip: NAND chip structure
350-
* @new_state: the state which is requested
351333
*
352-
* Get the device and lock it for exclusive access
334+
* Lock the device and its controller for exclusive access
335+
*
336+
* Return: -EBUSY if the chip has been suspended, 0 otherwise
353337
*/
354-
static int
355-
nand_get_device(struct nand_chip *chip, int new_state)
338+
static int nand_get_device(struct nand_chip *chip)
356339
{
357-
spinlock_t *lock = &chip->controller->lock;
358-
wait_queue_head_t *wq = &chip->controller->wq;
359-
DECLARE_WAITQUEUE(wait, current);
360-
retry:
361-
spin_lock(lock);
362-
363-
/* Hardware controller shared among independent devices */
364-
if (!chip->controller->active)
365-
chip->controller->active = chip;
366-
367-
if (chip->controller->active == chip && chip->state == FL_READY) {
368-
chip->state = new_state;
369-
spin_unlock(lock);
370-
return 0;
371-
}
372-
if (new_state == FL_PM_SUSPENDED) {
373-
if (chip->controller->active->state == FL_PM_SUSPENDED) {
374-
chip->state = FL_PM_SUSPENDED;
375-
spin_unlock(lock);
376-
return 0;
377-
}
340+
mutex_lock(&chip->lock);
341+
if (chip->suspended) {
342+
mutex_unlock(&chip->lock);
343+
return -EBUSY;
378344
}
379-
set_current_state(TASK_UNINTERRUPTIBLE);
380-
add_wait_queue(wq, &wait);
381-
spin_unlock(lock);
382-
schedule();
383-
remove_wait_queue(wq, &wait);
384-
goto retry;
345+
mutex_lock(&chip->controller->lock);
346+
347+
return 0;
385348
}
386349

387350
/**
@@ -602,7 +565,10 @@ static int nand_block_markbad_lowlevel(struct nand_chip *chip, loff_t ofs)
602565
nand_erase_nand(chip, &einfo, 0);
603566

604567
/* Write bad block marker to OOB */
605-
nand_get_device(chip, FL_WRITING);
568+
ret = nand_get_device(chip);
569+
if (ret)
570+
return ret;
571+
606572
ret = nand_markbad_bbm(chip, ofs);
607573
nand_release_device(chip);
608574
}
@@ -3580,7 +3546,9 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
35803546
ops->mode != MTD_OPS_RAW)
35813547
return -ENOTSUPP;
35823548

3583-
nand_get_device(chip, FL_READING);
3549+
ret = nand_get_device(chip);
3550+
if (ret)
3551+
return ret;
35843552

35853553
if (!ops->datbuf)
35863554
ret = nand_do_read_oob(chip, from, ops);
@@ -4099,9 +4067,6 @@ static int panic_nand_write(struct mtd_info *mtd, loff_t to, size_t len,
40994067
struct mtd_oob_ops ops;
41004068
int ret;
41014069

4102-
/* Grab the device */
4103-
panic_nand_get_device(chip, FL_WRITING);
4104-
41054070
nand_select_target(chip, chipnr);
41064071

41074072
/* Wait for the device to get ready */
@@ -4132,7 +4097,9 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
41324097

41334098
ops->retlen = 0;
41344099

4135-
nand_get_device(chip, FL_WRITING);
4100+
ret = nand_get_device(chip);
4101+
if (ret)
4102+
return ret;
41364103

41374104
switch (ops->mode) {
41384105
case MTD_OPS_PLACE_OOB:
@@ -4205,7 +4172,9 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
42054172
return -EINVAL;
42064173

42074174
/* Grab the lock and see if the device is available */
4208-
nand_get_device(chip, FL_ERASING);
4175+
ret = nand_get_device(chip);
4176+
if (ret)
4177+
return ret;
42094178

42104179
/* Shift to get first page */
42114180
page = (int)(instr->addr >> chip->page_shift);
@@ -4298,7 +4267,7 @@ static void nand_sync(struct mtd_info *mtd)
42984267
pr_debug("%s: called\n", __func__);
42994268

43004269
/* Grab the lock and see if the device is available */
4301-
nand_get_device(chip, FL_SYNCING);
4270+
WARN_ON(nand_get_device(chip));
43024271
/* Release it and go back */
43034272
nand_release_device(chip);
43044273
}
@@ -4315,7 +4284,10 @@ static int nand_block_isbad(struct mtd_info *mtd, loff_t offs)
43154284
int ret;
43164285

43174286
/* Select the NAND device */
4318-
nand_get_device(chip, FL_READING);
4287+
ret = nand_get_device(chip);
4288+
if (ret)
4289+
return ret;
4290+
43194291
nand_select_target(chip, chipnr);
43204292

43214293
ret = nand_block_checkbad(chip, offs, 0);
@@ -4388,7 +4360,13 @@ static int nand_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
43884360
*/
43894361
static int nand_suspend(struct mtd_info *mtd)
43904362
{
4391-
return nand_get_device(mtd_to_nand(mtd), FL_PM_SUSPENDED);
4363+
struct nand_chip *chip = mtd_to_nand(mtd);
4364+
4365+
mutex_lock(&chip->lock);
4366+
chip->suspended = 1;
4367+
mutex_unlock(&chip->lock);
4368+
4369+
return 0;
43924370
}
43934371

43944372
/**
@@ -4399,11 +4377,13 @@ static void nand_resume(struct mtd_info *mtd)
43994377
{
44004378
struct nand_chip *chip = mtd_to_nand(mtd);
44014379

4402-
if (chip->state == FL_PM_SUSPENDED)
4403-
nand_release_device(chip);
4380+
mutex_lock(&chip->lock);
4381+
if (chip->suspended)
4382+
chip->suspended = 0;
44044383
else
44054384
pr_err("%s called for a chip which is not in suspended state\n",
44064385
__func__);
4386+
mutex_unlock(&chip->lock);
44074387
}
44084388

44094389
/**
@@ -4413,7 +4393,7 @@ static void nand_resume(struct mtd_info *mtd)
44134393
*/
44144394
static void nand_shutdown(struct mtd_info *mtd)
44154395
{
4416-
nand_get_device(mtd_to_nand(mtd), FL_PM_SUSPENDED);
4396+
nand_suspend(mtd);
44174397
}
44184398

44194399
/* Set default functions */
@@ -5018,6 +4998,8 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
50184998
/* Assume all dies are deselected when we enter nand_scan_ident(). */
50194999
chip->cur_cs = -1;
50205000

5001+
mutex_init(&chip->lock);
5002+
50215003
/* Enforce the right timings for reset/detection */
50225004
onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0);
50235005

@@ -5717,9 +5699,6 @@ static int nand_scan_tail(struct nand_chip *chip)
57175699
}
57185700
chip->subpagesize = mtd->writesize >> mtd->subpage_sft;
57195701

5720-
/* Initialize state */
5721-
chip->state = FL_READY;
5722-
57235702
/* Invalidate the pagebuffer reference */
57245703
chip->pagebuf = -1;
57255704

include/linux/mtd/rawnand.h

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,12 @@
1616
#ifndef __LINUX_MTD_RAWNAND_H
1717
#define __LINUX_MTD_RAWNAND_H
1818

19-
#include <linux/wait.h>
20-
#include <linux/spinlock.h>
2119
#include <linux/mtd/mtd.h>
2220
#include <linux/mtd/flashchip.h>
2321
#include <linux/mtd/bbm.h>
2422
#include <linux/mtd/jedec.h>
2523
#include <linux/mtd/onfi.h>
24+
#include <linux/mutex.h>
2625
#include <linux/of.h>
2726
#include <linux/types.h>
2827

@@ -897,25 +896,17 @@ struct nand_controller_ops {
897896
/**
898897
* struct nand_controller - Structure used to describe a NAND controller
899898
*
900-
* @lock: protection lock
901-
* @active: the mtd device which holds the controller currently
902-
* @wq: wait queue to sleep on if a NAND operation is in
903-
* progress used instead of the per chip wait queue
904-
* when a hw controller is available.
899+
* @lock: lock used to serialize accesses to the NAND controller
905900
* @ops: NAND controller operations.
906901
*/
907902
struct nand_controller {
908-
spinlock_t lock;
909-
struct nand_chip *active;
910-
wait_queue_head_t wq;
903+
struct mutex lock;
911904
const struct nand_controller_ops *ops;
912905
};
913906

914907
static inline void nand_controller_init(struct nand_controller *nfc)
915908
{
916-
nfc->active = NULL;
917-
spin_lock_init(&nfc->lock);
918-
init_waitqueue_head(&nfc->wq);
909+
mutex_init(&nfc->lock);
919910
}
920911

921912
/**
@@ -983,7 +974,6 @@ struct nand_legacy {
983974
* setting the read-retry mode. Mostly needed for MLC NAND.
984975
* @ecc: [BOARDSPECIFIC] ECC control structure
985976
* @buf_align: minimum buffer alignment required by a platform
986-
* @state: [INTERN] the current state of the NAND device
987977
* @oob_poi: "poison value buffer," used for laying out OOB data
988978
* before writing
989979
* @page_shift: [INTERN] number of address bits in a page (column
@@ -1034,6 +1024,9 @@ struct nand_legacy {
10341024
* cur_cs < numchips. NAND Controller drivers should not
10351025
* modify this value, but they're allowed to read it.
10361026
* @read_retries: [INTERN] the number of read retry modes supported
1027+
* @lock: lock protecting the suspended field. Also used to
1028+
* serialize accesses to the NAND device.
1029+
* @suspended: set to 1 when the device is suspended, 0 when it's not.
10371030
* @bbt: [INTERN] bad block table pointer
10381031
* @bbt_td: [REPLACEABLE] bad block table descriptor for flash
10391032
* lookup.
@@ -1088,7 +1081,8 @@ struct nand_chip {
10881081

10891082
int read_retries;
10901083

1091-
flstate_t state;
1084+
struct mutex lock;
1085+
unsigned int suspended : 1;
10921086

10931087
uint8_t *oob_poi;
10941088
struct nand_controller *controller;

0 commit comments

Comments
 (0)