* [PATCH v3 0/4] mtd: core: protect access to mtd devices while in suspend @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Follow-up on discussion in: https://lkml.org/lkml/2021/10/4/41 https://lkml.org/lkml/2021/10/11/435 Changes since v2: - added signoff's to patch from Boris - removed accidential line break - kept tests consistent: master->master.suspended == 0 -> !master->master.suspended - added comments to mtdconcat patch - moved mtdconcat before ('mtd: core: protect access to MTD devices while in suspend') Changes since v1: - removed __mtd_suspend/__mtd_resume functions as they are not used by mtdconcat anymore. - only master mtd_info is used for mtd_{start,end}_access(). Warn if we got mtd's. - added Boris patch for using uninitialized _suspend/_resume hooks when bbt scanning - mtdconcat uses device _suspend/_resume hooks - I don't really like the macro proposal from Boris mtd_no_suspend_void_call()/mtd_no_suspend_ret_call() I think they make the code complex to read and the macro's doesn't fit every where anyway... Changes since from rfc v1/v2: - added access protection for all device access hooks in mtd_info. - added Suggested-by to [1/3] patch. - removed refereces to commit ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op") from commit msg as commit 013e6292aaf5 ("mtd: rawnand: Simplify the locking") is to be blamed. - tested on a kernel with LOCKDEP enabled. Boris Brezillon (1): mtd: rawnand: nand_bbt: hide suspend/resume hooks while scanning bbt Sean Nyekjaer (3): mtd: mtdconcat: add suspend lock handling mtd: core: protect access to MTD devices while in suspend mtd: rawnand: remove suspended check drivers/mtd/mtdconcat.c | 15 +++- drivers/mtd/mtdcore.c | 124 +++++++++++++++++++++++++++---- drivers/mtd/nand/raw/nand_base.c | 52 ++++--------- drivers/mtd/nand/raw/nand_bbt.c | 28 ++++++- include/linux/mtd/mtd.h | 81 ++++++++++++++++---- include/linux/mtd/rawnand.h | 5 +- 6 files changed, 230 insertions(+), 75 deletions(-) -- 2.33.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 0/4] mtd: core: protect access to mtd devices while in suspend @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Follow-up on discussion in: https://lkml.org/lkml/2021/10/4/41 https://lkml.org/lkml/2021/10/11/435 Changes since v2: - added signoff's to patch from Boris - removed accidential line break - kept tests consistent: master->master.suspended == 0 -> !master->master.suspended - added comments to mtdconcat patch - moved mtdconcat before ('mtd: core: protect access to MTD devices while in suspend') Changes since v1: - removed __mtd_suspend/__mtd_resume functions as they are not used by mtdconcat anymore. - only master mtd_info is used for mtd_{start,end}_access(). Warn if we got mtd's. - added Boris patch for using uninitialized _suspend/_resume hooks when bbt scanning - mtdconcat uses device _suspend/_resume hooks - I don't really like the macro proposal from Boris mtd_no_suspend_void_call()/mtd_no_suspend_ret_call() I think they make the code complex to read and the macro's doesn't fit every where anyway... Changes since from rfc v1/v2: - added access protection for all device access hooks in mtd_info. - added Suggested-by to [1/3] patch. - removed refereces to commit ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op") from commit msg as commit 013e6292aaf5 ("mtd: rawnand: Simplify the locking") is to be blamed. - tested on a kernel with LOCKDEP enabled. Boris Brezillon (1): mtd: rawnand: nand_bbt: hide suspend/resume hooks while scanning bbt Sean Nyekjaer (3): mtd: mtdconcat: add suspend lock handling mtd: core: protect access to MTD devices while in suspend mtd: rawnand: remove suspended check drivers/mtd/mtdconcat.c | 15 +++- drivers/mtd/mtdcore.c | 124 +++++++++++++++++++++++++++---- drivers/mtd/nand/raw/nand_base.c | 52 ++++--------- drivers/mtd/nand/raw/nand_bbt.c | 28 ++++++- include/linux/mtd/mtd.h | 81 ++++++++++++++++---- include/linux/mtd/rawnand.h | 5 +- 6 files changed, 230 insertions(+), 75 deletions(-) -- 2.33.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/4] mtd: rawnand: nand_bbt: hide suspend/resume hooks while scanning bbt 2021-10-25 9:27 ` Sean Nyekjaer @ 2021-10-25 9:27 ` Sean Nyekjaer -1 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel From: Boris Brezillon <boris.brezillon@collabora.com> The BBT scan logic use the MTD helpers before the MTD layer had a chance to initialize the device, and that leads to issues when accessing the uninitialized suspend lock. Let's temporarily set the suspend/resume hooks to NULL to skip the lock acquire/release step. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Tested-by: Sean Nyekjaer <sean@geanix.com> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/nand/raw/nand_bbt.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c index b7ad030225f8..93d385703469 100644 --- a/drivers/mtd/nand/raw/nand_bbt.c +++ b/drivers/mtd/nand/raw/nand_bbt.c @@ -1397,8 +1397,28 @@ static int nand_create_badblock_pattern(struct nand_chip *this) */ int nand_create_bbt(struct nand_chip *this) { + struct mtd_info *mtd = nand_to_mtd(this); + int (*suspend) (struct mtd_info *) = mtd->_suspend; + void (*resume) (struct mtd_info *) = mtd->_resume; int ret; + /* + * The BBT scan logic use the MTD helpers before the MTD layer had a + * chance to initialize the device, and that leads to issues when + * accessing the uninitialized suspend lock. Let's temporarily set the + * suspend/resume hooks to NULL to skip the lock acquire/release step. + * + * FIXME: This is an ugly hack, so please don't copy this pattern to + * other MTD implementations. The proper fix would be to implement a + * generic BBT scan logic at the NAND level that's not using any of the + * MTD helpers to access pages. We also might consider doing a two + * step initialization at the MTD level (mtd_device_init() + + * mtd_device_register()) so some of the fields are initialized + * early. + */ + mtd->_suspend = NULL; + mtd->_resume = NULL; + /* Is a flash based bad block table requested? */ if (this->bbt_options & NAND_BBT_USE_FLASH) { /* Use the default pattern descriptors */ @@ -1422,7 +1442,13 @@ int nand_create_bbt(struct nand_chip *this) return ret; } - return nand_scan_bbt(this, this->badblock_pattern); + ret = nand_scan_bbt(this, this->badblock_pattern); + + /* Restore the suspend/resume hooks. */ + mtd->_suspend = suspend; + mtd->_resume = resume; + + return ret; } EXPORT_SYMBOL(nand_create_bbt); -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 1/4] mtd: rawnand: nand_bbt: hide suspend/resume hooks while scanning bbt @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel From: Boris Brezillon <boris.brezillon@collabora.com> The BBT scan logic use the MTD helpers before the MTD layer had a chance to initialize the device, and that leads to issues when accessing the uninitialized suspend lock. Let's temporarily set the suspend/resume hooks to NULL to skip the lock acquire/release step. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Tested-by: Sean Nyekjaer <sean@geanix.com> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/nand/raw/nand_bbt.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c index b7ad030225f8..93d385703469 100644 --- a/drivers/mtd/nand/raw/nand_bbt.c +++ b/drivers/mtd/nand/raw/nand_bbt.c @@ -1397,8 +1397,28 @@ static int nand_create_badblock_pattern(struct nand_chip *this) */ int nand_create_bbt(struct nand_chip *this) { + struct mtd_info *mtd = nand_to_mtd(this); + int (*suspend) (struct mtd_info *) = mtd->_suspend; + void (*resume) (struct mtd_info *) = mtd->_resume; int ret; + /* + * The BBT scan logic use the MTD helpers before the MTD layer had a + * chance to initialize the device, and that leads to issues when + * accessing the uninitialized suspend lock. Let's temporarily set the + * suspend/resume hooks to NULL to skip the lock acquire/release step. + * + * FIXME: This is an ugly hack, so please don't copy this pattern to + * other MTD implementations. The proper fix would be to implement a + * generic BBT scan logic at the NAND level that's not using any of the + * MTD helpers to access pages. We also might consider doing a two + * step initialization at the MTD level (mtd_device_init() + + * mtd_device_register()) so some of the fields are initialized + * early. + */ + mtd->_suspend = NULL; + mtd->_resume = NULL; + /* Is a flash based bad block table requested? */ if (this->bbt_options & NAND_BBT_USE_FLASH) { /* Use the default pattern descriptors */ @@ -1422,7 +1442,13 @@ int nand_create_bbt(struct nand_chip *this) return ret; } - return nand_scan_bbt(this, this->badblock_pattern); + ret = nand_scan_bbt(this, this->badblock_pattern); + + /* Restore the suspend/resume hooks. */ + mtd->_suspend = suspend; + mtd->_resume = resume; + + return ret; } EXPORT_SYMBOL(nand_create_bbt); -- 2.33.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/4] mtd: mtdconcat: add suspend lock handling 2021-10-25 9:27 ` Sean Nyekjaer @ 2021-10-25 9:27 ` Sean Nyekjaer -1 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Use MTD hooks to control suspend/resume of MTD devices. concat_{suspend,resume} will be called from mtd_{suspend,resume}, which already have taken the suspend/resume lock. It's safe to proceed with calling MTD device hooks directly from here. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/mtdconcat.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c index f685a581df48..37532f529820 100644 --- a/drivers/mtd/mtdconcat.c +++ b/drivers/mtd/mtdconcat.c @@ -566,9 +566,15 @@ static int concat_suspend(struct mtd_info *mtd) for (i = 0; i < concat->num_subdev; i++) { struct mtd_info *subdev = concat->subdev[i]; - if ((rc = mtd_suspend(subdev)) < 0) + /* + * Call MTD hook directly from here, + * mtd_suspend() have the suspend/resume lock. + */ + rc = subdev->_suspend ? subdev->_suspend(subdev) : 0; + if (rc < 0) return rc; } + return rc; } @@ -579,7 +585,12 @@ static void concat_resume(struct mtd_info *mtd) for (i = 0; i < concat->num_subdev; i++) { struct mtd_info *subdev = concat->subdev[i]; - mtd_resume(subdev); + /* + * Call MTD hook directly from here, + * mtd_resume() have the suspend/resume lock. + */ + if (subdev->_resume) + subdev->_resume(subdev); } } -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/4] mtd: mtdconcat: add suspend lock handling @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Use MTD hooks to control suspend/resume of MTD devices. concat_{suspend,resume} will be called from mtd_{suspend,resume}, which already have taken the suspend/resume lock. It's safe to proceed with calling MTD device hooks directly from here. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/mtdconcat.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c index f685a581df48..37532f529820 100644 --- a/drivers/mtd/mtdconcat.c +++ b/drivers/mtd/mtdconcat.c @@ -566,9 +566,15 @@ static int concat_suspend(struct mtd_info *mtd) for (i = 0; i < concat->num_subdev; i++) { struct mtd_info *subdev = concat->subdev[i]; - if ((rc = mtd_suspend(subdev)) < 0) + /* + * Call MTD hook directly from here, + * mtd_suspend() have the suspend/resume lock. + */ + rc = subdev->_suspend ? subdev->_suspend(subdev) : 0; + if (rc < 0) return rc; } + return rc; } @@ -579,7 +585,12 @@ static void concat_resume(struct mtd_info *mtd) for (i = 0; i < concat->num_subdev; i++) { struct mtd_info *subdev = concat->subdev[i]; - mtd_resume(subdev); + /* + * Call MTD hook directly from here, + * mtd_resume() have the suspend/resume lock. + */ + if (subdev->_resume) + subdev->_resume(subdev); } } -- 2.33.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] mtd: mtdconcat: add suspend lock handling 2021-10-25 9:27 ` Sean Nyekjaer @ 2021-10-25 14:59 ` Boris Brezillon -1 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2021-10-25 14:59 UTC (permalink / raw) To: Sean Nyekjaer Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Hello Sean, On Mon, 25 Oct 2021 11:27:50 +0200 Sean Nyekjaer <sean@geanix.com> wrote: The subject is misleading, how about 'mtd: mtdconcat: Don't use mtd_{suspend,resume}()' > Use MTD hooks to control suspend/resume of MTD devices. > concat_{suspend,resume} will be called from mtd_{suspend,resume}, > which already have taken the suspend/resume lock. > It's safe to proceed with calling MTD device hooks directly from here. " The MTD suspend logic will soon be adjusted to automatically wait for device wake-up before issuing IOs. In order to do that a new read-write lock will be added and taken in write-mode in the mtd_{suspend,resume}() path. Since mtdconcat.c itself is an MTD device, calling mtd_suspend/resume() on subdevices from the mtdconcat ->_{suspend,resume}() hook will lead to a nested lock, which lockdep will complain about if we don't add a proper annotation. Let's keep things simple and replace those mtd_{suspend,resume}(subdev) calls by subdev->_{suspend,resume}() ones to avoid this situation. " > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Again, this commit doesn't fix anything. > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > drivers/mtd/mtdconcat.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c > index f685a581df48..37532f529820 100644 > --- a/drivers/mtd/mtdconcat.c > +++ b/drivers/mtd/mtdconcat.c > @@ -566,9 +566,15 @@ static int concat_suspend(struct mtd_info *mtd) > > for (i = 0; i < concat->num_subdev; i++) { > struct mtd_info *subdev = concat->subdev[i]; > - if ((rc = mtd_suspend(subdev)) < 0) > + /* > + * Call MTD hook directly from here, > + * mtd_suspend() have the suspend/resume lock. /* * Call the MTD hook directly to avoid a nested lock * on ->suspend_lock. */ > + */ > + rc = subdev->_suspend ? subdev->_suspend(subdev) : 0; > + if (rc < 0) > return rc; > } > + > return rc; > } > > @@ -579,7 +585,12 @@ static void concat_resume(struct mtd_info *mtd) > > for (i = 0; i < concat->num_subdev; i++) { > struct mtd_info *subdev = concat->subdev[i]; > - mtd_resume(subdev); > + /* > + * Call MTD hook directly from here, > + * mtd_resume() have the suspend/resume lock. > + */ Ditto. > + if (subdev->_resume) > + subdev->_resume(subdev); > } > } > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/4] mtd: mtdconcat: add suspend lock handling @ 2021-10-25 14:59 ` Boris Brezillon 0 siblings, 0 replies; 12+ messages in thread From: Boris Brezillon @ 2021-10-25 14:59 UTC (permalink / raw) To: Sean Nyekjaer Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Hello Sean, On Mon, 25 Oct 2021 11:27:50 +0200 Sean Nyekjaer <sean@geanix.com> wrote: The subject is misleading, how about 'mtd: mtdconcat: Don't use mtd_{suspend,resume}()' > Use MTD hooks to control suspend/resume of MTD devices. > concat_{suspend,resume} will be called from mtd_{suspend,resume}, > which already have taken the suspend/resume lock. > It's safe to proceed with calling MTD device hooks directly from here. " The MTD suspend logic will soon be adjusted to automatically wait for device wake-up before issuing IOs. In order to do that a new read-write lock will be added and taken in write-mode in the mtd_{suspend,resume}() path. Since mtdconcat.c itself is an MTD device, calling mtd_suspend/resume() on subdevices from the mtdconcat ->_{suspend,resume}() hook will lead to a nested lock, which lockdep will complain about if we don't add a proper annotation. Let's keep things simple and replace those mtd_{suspend,resume}(subdev) calls by subdev->_{suspend,resume}() ones to avoid this situation. " > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Again, this commit doesn't fix anything. > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > drivers/mtd/mtdconcat.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c > index f685a581df48..37532f529820 100644 > --- a/drivers/mtd/mtdconcat.c > +++ b/drivers/mtd/mtdconcat.c > @@ -566,9 +566,15 @@ static int concat_suspend(struct mtd_info *mtd) > > for (i = 0; i < concat->num_subdev; i++) { > struct mtd_info *subdev = concat->subdev[i]; > - if ((rc = mtd_suspend(subdev)) < 0) > + /* > + * Call MTD hook directly from here, > + * mtd_suspend() have the suspend/resume lock. /* * Call the MTD hook directly to avoid a nested lock * on ->suspend_lock. */ > + */ > + rc = subdev->_suspend ? subdev->_suspend(subdev) : 0; > + if (rc < 0) > return rc; > } > + > return rc; > } > > @@ -579,7 +585,12 @@ static void concat_resume(struct mtd_info *mtd) > > for (i = 0; i < concat->num_subdev; i++) { > struct mtd_info *subdev = concat->subdev[i]; > - mtd_resume(subdev); > + /* > + * Call MTD hook directly from here, > + * mtd_resume() have the suspend/resume lock. > + */ Ditto. > + if (subdev->_resume) > + subdev->_resume(subdev); > } > } > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/4] mtd: core: protect access to MTD devices while in suspend 2021-10-25 9:27 ` Sean Nyekjaer @ 2021-10-25 9:27 ` Sean Nyekjaer -1 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Prevent MTD access while in a suspended state. Also prevent suspending a device which is still currently in use. Commit 013e6292aaf5 ("mtd: rawnand: Simplify the locking") allows the rawnand layer to return errors rather than waiting in a blocking wait. Tested on a iMX6ULL. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/mtdcore.c | 124 +++++++++++++++++++++++++++++++++++----- include/linux/mtd/mtd.h | 81 +++++++++++++++++++++----- 2 files changed, 176 insertions(+), 29 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 153229198947..8e6b7e858ada 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -777,6 +777,8 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd) INIT_LIST_HEAD(&mtd->partitions); mutex_init(&mtd->master.partitions_lock); mutex_init(&mtd->master.chrdev_lock); + init_waitqueue_head(&mtd->master.resume_wq); + init_rwsem(&mtd->master.suspend_lock); } static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user) @@ -1267,7 +1269,9 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) adjinstr.addr += mst_ofs; + mtd_start_access(master); ret = master->_erase(master, &adjinstr); + mtd_end_access(master); if (adjinstr.fail_addr != MTD_FAIL_ADDR_UNKNOWN) { instr->fail_addr = adjinstr.fail_addr - mst_ofs; @@ -1289,6 +1293,7 @@ int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, void **virt, resource_size_t *phys) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; *virt = NULL; @@ -1301,8 +1306,12 @@ int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, if (!len) return 0; + mtd_start_access(master); from = mtd_get_master_ofs(mtd, from); - return master->_point(master, from, len, retlen, virt, phys); + ret = master->_point(master, from, len, retlen, virt, phys); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_point); @@ -1310,6 +1319,7 @@ EXPORT_SYMBOL_GPL(mtd_point); int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_unpoint) return -EOPNOTSUPP; @@ -1317,7 +1327,12 @@ int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len) return -EINVAL; if (!len) return 0; - return master->_unpoint(master, mtd_get_master_ofs(mtd, from), len); + + mtd_start_access(master); + ret = master->_unpoint(master, mtd_get_master_ofs(mtd, from), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_unpoint); @@ -1372,6 +1387,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, }; int ret; + /* mtd_read_oob_std handles mtd access protection */ ret = mtd_read_oob(mtd, from, &ops); *retlen = ops.retlen; @@ -1388,6 +1404,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, }; int ret; + /* mtd_write_oob_std handles mtd access protection */ ret = mtd_write_oob(mtd, to, &ops); *retlen = ops.retlen; @@ -1406,6 +1423,7 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_panic_write) @@ -1419,8 +1437,12 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, if (!master->oops_panic_write) master->oops_panic_write = true; - return master->_panic_write(master, mtd_get_master_ofs(mtd, to), len, - retlen, buf); + mtd_start_access(master); + ret = master->_panic_write(master, mtd_get_master_ofs(mtd, to), len, + retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_panic_write); @@ -1464,11 +1486,13 @@ static int mtd_read_oob_std(struct mtd_info *mtd, loff_t from, int ret; from = mtd_get_master_ofs(mtd, from); + mtd_start_access(master); if (master->_read_oob) ret = master->_read_oob(master, from, ops); else ret = master->_read(master, from, ops->len, &ops->retlen, ops->datbuf); + mtd_end_access(master); return ret; } @@ -1480,11 +1504,13 @@ static int mtd_write_oob_std(struct mtd_info *mtd, loff_t to, int ret; to = mtd_get_master_ofs(mtd, to); + mtd_start_access(master); if (master->_write_oob) ret = master->_write_oob(master, to, ops); else ret = master->_write(master, to, ops->len, &ops->retlen, ops->datbuf); + mtd_end_access(master); return ret; } @@ -1992,12 +2018,18 @@ int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen, struct otp_info *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_get_fact_prot_info) return -EOPNOTSUPP; if (!len) return 0; - return master->_get_fact_prot_info(master, len, retlen, buf); + + mtd_start_access(master); + ret = master->_get_fact_prot_info(master, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_get_fact_prot_info); @@ -2005,13 +2037,19 @@ int mtd_read_fact_prot_reg(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_read_fact_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_read_fact_prot_reg(master, from, len, retlen, buf); + + mtd_start_access(master); + ret = master->_read_fact_prot_reg(master, from, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg); @@ -2019,12 +2057,18 @@ int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen, struct otp_info *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_get_user_prot_info) return -EOPNOTSUPP; if (!len) return 0; - return master->_get_user_prot_info(master, len, retlen, buf); + + mtd_start_access(master); + ret = master->_get_user_prot_info(master, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_get_user_prot_info); @@ -2032,13 +2076,19 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_read_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_read_user_prot_reg(master, from, len, retlen, buf); + + mtd_start_access(master); + ret = master->_read_user_prot_reg(master, from, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_read_user_prot_reg); @@ -2053,7 +2103,11 @@ int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len, return -EOPNOTSUPP; if (!len) return 0; + + mtd_start_access(master); ret = master->_write_user_prot_reg(master, to, len, retlen, buf); + mtd_end_access(master); + if (ret) return ret; @@ -2068,24 +2122,36 @@ EXPORT_SYMBOL_GPL(mtd_write_user_prot_reg); int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_lock_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_lock_user_prot_reg(master, from, len); + + mtd_start_access(master); + ret = master->_lock_user_prot_reg(master, from, len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg); int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_erase_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_erase_user_prot_reg(master, from, len); + + mtd_start_access(master); + ret = master->_erase_user_prot_reg(master, from, len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg); @@ -2093,6 +2159,7 @@ EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg); int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_lock) return -EOPNOTSUPP; @@ -2106,13 +2173,18 @@ int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_lock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_lock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_lock); int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_unlock) return -EOPNOTSUPP; @@ -2126,13 +2198,18 @@ int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_unlock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_unlock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_unlock); int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_is_locked) return -EOPNOTSUPP; @@ -2146,13 +2223,18 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_is_locked(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_is_locked(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_is_locked); int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (ofs < 0 || ofs >= mtd->size) return -EINVAL; @@ -2162,13 +2244,18 @@ int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; - return master->_block_isreserved(master, mtd_get_master_ofs(mtd, ofs)); + mtd_start_access(master); + ret = master->_block_isreserved(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_block_isreserved); int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (ofs < 0 || ofs >= mtd->size) return -EINVAL; @@ -2178,7 +2265,11 @@ int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; - return master->_block_isbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_start_access(master); + ret = master->_block_isbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_block_isbad); @@ -2197,7 +2288,10 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; + mtd_start_access(master); ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + if (ret) return ret; diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 88227044fc86..b074106e2d8e 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -231,6 +231,8 @@ struct mtd_master { struct mutex partitions_lock; struct mutex chrdev_lock; unsigned int suspended : 1; + wait_queue_head_t resume_wq; + struct rw_semaphore suspend_lock; }; struct mtd_info { @@ -476,10 +478,47 @@ static inline u32 mtd_oobavail(struct mtd_info *mtd, struct mtd_oob_ops *ops) return ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; } +static inline void mtd_start_access(struct mtd_info *master) +{ + WARN_ON_ONCE(master != mtd_get_master(master)); + + /* + * Don't take the suspend_lock on devices that don't + * implement the suspend hook. Otherwise, lockdep will + * complain about nested locks when trying to suspend MTD + * partitions or MTD devices created by gluebi which are + * backed by real devices. + */ + if (!master->_suspend) + return; + + /* + * Wait until the device is resumed. Should we have a + * non-blocking mode here? + */ + while (1) { + down_read(&master->master.suspend_lock); + if (!master->master.suspended) + return; + + up_read(&master->master.suspend_lock); + wait_event(master->master.resume_wq, !master->master.suspended); + } +} + +static inline void mtd_end_access(struct mtd_info *master) +{ + if (!master->_suspend) + return; + + up_read(&master->master.suspend_lock); +} + static inline int mtd_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_max_bad_blocks) return -ENOTSUPP; @@ -487,8 +526,12 @@ static inline int mtd_max_bad_blocks(struct mtd_info *mtd, if (mtd->size < (len + ofs) || ofs < 0) return -EINVAL; - return master->_max_bad_blocks(master, mtd_get_master_ofs(mtd, ofs), - len); + mtd_start_access(master); + ret = master->_max_bad_blocks(master, mtd_get_master_ofs(mtd, ofs), + len); + mtd_end_access(master); + + return ret; } int mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, @@ -546,30 +589,40 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs); static inline int mtd_suspend(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - int ret; - - if (master->master.suspended) - return 0; + int ret = 0; - ret = master->_suspend ? master->_suspend(master) : 0; - if (ret) + if (!master->_suspend) return ret; - master->master.suspended = 1; - return 0; + down_write(&master->master.suspend_lock); + if (!master->master.suspended) { + ret = master->_suspend(master); + if (!ret) + master->master.suspended = 1; + } + up_write(&master->master.suspend_lock); + + return ret; } static inline void mtd_resume(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - if (!master->master.suspended) + if (!master->_suspend) return; - if (master->_resume) - master->_resume(master); + down_write(&master->master.suspend_lock); + if (master->master.suspended) { + if (master->_resume) + master->_resume(master); + + master->master.suspended = 0; - master->master.suspended = 0; + /* The MTD dev has been resumed, wake up all waiters. */ + wake_up_all(&master->master.resume_wq); + } + up_write(&master->master.suspend_lock); } static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd) -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/4] mtd: core: protect access to MTD devices while in suspend @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Prevent MTD access while in a suspended state. Also prevent suspending a device which is still currently in use. Commit 013e6292aaf5 ("mtd: rawnand: Simplify the locking") allows the rawnand layer to return errors rather than waiting in a blocking wait. Tested on a iMX6ULL. Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/mtdcore.c | 124 +++++++++++++++++++++++++++++++++++----- include/linux/mtd/mtd.h | 81 +++++++++++++++++++++----- 2 files changed, 176 insertions(+), 29 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 153229198947..8e6b7e858ada 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -777,6 +777,8 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd) INIT_LIST_HEAD(&mtd->partitions); mutex_init(&mtd->master.partitions_lock); mutex_init(&mtd->master.chrdev_lock); + init_waitqueue_head(&mtd->master.resume_wq); + init_rwsem(&mtd->master.suspend_lock); } static ssize_t mtd_otp_size(struct mtd_info *mtd, bool is_user) @@ -1267,7 +1269,9 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr) adjinstr.addr += mst_ofs; + mtd_start_access(master); ret = master->_erase(master, &adjinstr); + mtd_end_access(master); if (adjinstr.fail_addr != MTD_FAIL_ADDR_UNKNOWN) { instr->fail_addr = adjinstr.fail_addr - mst_ofs; @@ -1289,6 +1293,7 @@ int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, void **virt, resource_size_t *phys) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; *virt = NULL; @@ -1301,8 +1306,12 @@ int mtd_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, if (!len) return 0; + mtd_start_access(master); from = mtd_get_master_ofs(mtd, from); - return master->_point(master, from, len, retlen, virt, phys); + ret = master->_point(master, from, len, retlen, virt, phys); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_point); @@ -1310,6 +1319,7 @@ EXPORT_SYMBOL_GPL(mtd_point); int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_unpoint) return -EOPNOTSUPP; @@ -1317,7 +1327,12 @@ int mtd_unpoint(struct mtd_info *mtd, loff_t from, size_t len) return -EINVAL; if (!len) return 0; - return master->_unpoint(master, mtd_get_master_ofs(mtd, from), len); + + mtd_start_access(master); + ret = master->_unpoint(master, mtd_get_master_ofs(mtd, from), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_unpoint); @@ -1372,6 +1387,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, }; int ret; + /* mtd_read_oob_std handles mtd access protection */ ret = mtd_read_oob(mtd, from, &ops); *retlen = ops.retlen; @@ -1388,6 +1404,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, }; int ret; + /* mtd_write_oob_std handles mtd access protection */ ret = mtd_write_oob(mtd, to, &ops); *retlen = ops.retlen; @@ -1406,6 +1423,7 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_panic_write) @@ -1419,8 +1437,12 @@ int mtd_panic_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, if (!master->oops_panic_write) master->oops_panic_write = true; - return master->_panic_write(master, mtd_get_master_ofs(mtd, to), len, - retlen, buf); + mtd_start_access(master); + ret = master->_panic_write(master, mtd_get_master_ofs(mtd, to), len, + retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_panic_write); @@ -1464,11 +1486,13 @@ static int mtd_read_oob_std(struct mtd_info *mtd, loff_t from, int ret; from = mtd_get_master_ofs(mtd, from); + mtd_start_access(master); if (master->_read_oob) ret = master->_read_oob(master, from, ops); else ret = master->_read(master, from, ops->len, &ops->retlen, ops->datbuf); + mtd_end_access(master); return ret; } @@ -1480,11 +1504,13 @@ static int mtd_write_oob_std(struct mtd_info *mtd, loff_t to, int ret; to = mtd_get_master_ofs(mtd, to); + mtd_start_access(master); if (master->_write_oob) ret = master->_write_oob(master, to, ops); else ret = master->_write(master, to, ops->len, &ops->retlen, ops->datbuf); + mtd_end_access(master); return ret; } @@ -1992,12 +2018,18 @@ int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen, struct otp_info *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_get_fact_prot_info) return -EOPNOTSUPP; if (!len) return 0; - return master->_get_fact_prot_info(master, len, retlen, buf); + + mtd_start_access(master); + ret = master->_get_fact_prot_info(master, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_get_fact_prot_info); @@ -2005,13 +2037,19 @@ int mtd_read_fact_prot_reg(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_read_fact_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_read_fact_prot_reg(master, from, len, retlen, buf); + + mtd_start_access(master); + ret = master->_read_fact_prot_reg(master, from, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_read_fact_prot_reg); @@ -2019,12 +2057,18 @@ int mtd_get_user_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen, struct otp_info *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_get_user_prot_info) return -EOPNOTSUPP; if (!len) return 0; - return master->_get_user_prot_info(master, len, retlen, buf); + + mtd_start_access(master); + ret = master->_get_user_prot_info(master, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_get_user_prot_info); @@ -2032,13 +2076,19 @@ int mtd_read_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf) { struct mtd_info *master = mtd_get_master(mtd); + int ret; *retlen = 0; if (!master->_read_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_read_user_prot_reg(master, from, len, retlen, buf); + + mtd_start_access(master); + ret = master->_read_user_prot_reg(master, from, len, retlen, buf); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_read_user_prot_reg); @@ -2053,7 +2103,11 @@ int mtd_write_user_prot_reg(struct mtd_info *mtd, loff_t to, size_t len, return -EOPNOTSUPP; if (!len) return 0; + + mtd_start_access(master); ret = master->_write_user_prot_reg(master, to, len, retlen, buf); + mtd_end_access(master); + if (ret) return ret; @@ -2068,24 +2122,36 @@ EXPORT_SYMBOL_GPL(mtd_write_user_prot_reg); int mtd_lock_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_lock_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_lock_user_prot_reg(master, from, len); + + mtd_start_access(master); + ret = master->_lock_user_prot_reg(master, from, len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_lock_user_prot_reg); int mtd_erase_user_prot_reg(struct mtd_info *mtd, loff_t from, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_erase_user_prot_reg) return -EOPNOTSUPP; if (!len) return 0; - return master->_erase_user_prot_reg(master, from, len); + + mtd_start_access(master); + ret = master->_erase_user_prot_reg(master, from, len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg); @@ -2093,6 +2159,7 @@ EXPORT_SYMBOL_GPL(mtd_erase_user_prot_reg); int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_lock) return -EOPNOTSUPP; @@ -2106,13 +2173,18 @@ int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_lock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_lock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_lock); int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_unlock) return -EOPNOTSUPP; @@ -2126,13 +2198,18 @@ int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_unlock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_unlock(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_unlock); int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_is_locked) return -EOPNOTSUPP; @@ -2146,13 +2223,18 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len) len = (u64)mtd_div_by_eb(len, mtd) * master->erasesize; } - return master->_is_locked(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_start_access(master); + ret = master->_is_locked(master, mtd_get_master_ofs(mtd, ofs), len); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_is_locked); int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (ofs < 0 || ofs >= mtd->size) return -EINVAL; @@ -2162,13 +2244,18 @@ int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; - return master->_block_isreserved(master, mtd_get_master_ofs(mtd, ofs)); + mtd_start_access(master); + ret = master->_block_isreserved(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_block_isreserved); int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (ofs < 0 || ofs >= mtd->size) return -EINVAL; @@ -2178,7 +2265,11 @@ int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; - return master->_block_isbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_start_access(master); + ret = master->_block_isbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + + return ret; } EXPORT_SYMBOL_GPL(mtd_block_isbad); @@ -2197,7 +2288,10 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs) if (mtd->flags & MTD_SLC_ON_MLC_EMULATION) ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize; + mtd_start_access(master); ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs)); + mtd_end_access(master); + if (ret) return ret; diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 88227044fc86..b074106e2d8e 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -231,6 +231,8 @@ struct mtd_master { struct mutex partitions_lock; struct mutex chrdev_lock; unsigned int suspended : 1; + wait_queue_head_t resume_wq; + struct rw_semaphore suspend_lock; }; struct mtd_info { @@ -476,10 +478,47 @@ static inline u32 mtd_oobavail(struct mtd_info *mtd, struct mtd_oob_ops *ops) return ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; } +static inline void mtd_start_access(struct mtd_info *master) +{ + WARN_ON_ONCE(master != mtd_get_master(master)); + + /* + * Don't take the suspend_lock on devices that don't + * implement the suspend hook. Otherwise, lockdep will + * complain about nested locks when trying to suspend MTD + * partitions or MTD devices created by gluebi which are + * backed by real devices. + */ + if (!master->_suspend) + return; + + /* + * Wait until the device is resumed. Should we have a + * non-blocking mode here? + */ + while (1) { + down_read(&master->master.suspend_lock); + if (!master->master.suspended) + return; + + up_read(&master->master.suspend_lock); + wait_event(master->master.resume_wq, !master->master.suspended); + } +} + +static inline void mtd_end_access(struct mtd_info *master) +{ + if (!master->_suspend) + return; + + up_read(&master->master.suspend_lock); +} + static inline int mtd_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len) { struct mtd_info *master = mtd_get_master(mtd); + int ret; if (!master->_max_bad_blocks) return -ENOTSUPP; @@ -487,8 +526,12 @@ static inline int mtd_max_bad_blocks(struct mtd_info *mtd, if (mtd->size < (len + ofs) || ofs < 0) return -EINVAL; - return master->_max_bad_blocks(master, mtd_get_master_ofs(mtd, ofs), - len); + mtd_start_access(master); + ret = master->_max_bad_blocks(master, mtd_get_master_ofs(mtd, ofs), + len); + mtd_end_access(master); + + return ret; } int mtd_wunit_to_pairing_info(struct mtd_info *mtd, int wunit, @@ -546,30 +589,40 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs); static inline int mtd_suspend(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - int ret; - - if (master->master.suspended) - return 0; + int ret = 0; - ret = master->_suspend ? master->_suspend(master) : 0; - if (ret) + if (!master->_suspend) return ret; - master->master.suspended = 1; - return 0; + down_write(&master->master.suspend_lock); + if (!master->master.suspended) { + ret = master->_suspend(master); + if (!ret) + master->master.suspended = 1; + } + up_write(&master->master.suspend_lock); + + return ret; } static inline void mtd_resume(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - if (!master->master.suspended) + if (!master->_suspend) return; - if (master->_resume) - master->_resume(master); + down_write(&master->master.suspend_lock); + if (master->master.suspended) { + if (master->_resume) + master->_resume(master); + + master->master.suspended = 0; - master->master.suspended = 0; + /* The MTD dev has been resumed, wake up all waiters. */ + wake_up_all(&master->master.resume_wq); + } + up_write(&master->master.suspend_lock); } static inline uint32_t mtd_div_by_eb(uint64_t sz, struct mtd_info *mtd) -- 2.33.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/4] mtd: rawnand: remove suspended check 2021-10-25 9:27 ` Sean Nyekjaer @ 2021-10-25 9:27 ` Sean Nyekjaer -1 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Access is protected in upper MTD layer when MTD devices are suspended. Commit ("mtd: core: protect access to MTD devices while in suspend") introduces access protection, so it's safe to remove suspended checks. Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/nand/raw/nand_base.c | 52 ++++++++------------------------ include/linux/mtd/rawnand.h | 5 +-- 2 files changed, 14 insertions(+), 43 deletions(-) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 3d6c6e880520..aa2874ae3c4a 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -332,19 +332,11 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) * @chip: NAND chip structure * * Lock the device and its controller for exclusive access - * - * Return: -EBUSY if the chip has been suspended, 0 otherwise */ -static int nand_get_device(struct nand_chip *chip) +static void nand_get_device(struct nand_chip *chip) { mutex_lock(&chip->lock); - if (chip->suspended) { - mutex_unlock(&chip->lock); - return -EBUSY; - } mutex_lock(&chip->controller->lock); - - return 0; } /** @@ -573,10 +565,7 @@ static int nand_block_markbad_lowlevel(struct nand_chip *chip, loff_t ofs) nand_erase_nand(chip, &einfo, 0); /* Write bad block marker to OOB */ - ret = nand_get_device(chip); - if (ret) - return ret; - + nand_get_device(chip); ret = nand_markbad_bbm(chip, ofs); nand_release_device(chip); } @@ -3756,9 +3745,7 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from, ops->mode != MTD_OPS_RAW) return -ENOTSUPP; - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); if (!ops->datbuf) ret = nand_do_read_oob(chip, from, ops); @@ -4345,13 +4332,11 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops) { struct nand_chip *chip = mtd_to_nand(mtd); - int ret; + int ret = 0; ops->retlen = 0; - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); switch (ops->mode) { case MTD_OPS_PLACE_OOB: @@ -4410,10 +4395,8 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr, if (nand_region_is_secured(chip, instr->addr, instr->len)) return -EIO; - /* Grab the lock and see if the device is available */ - ret = nand_get_device(chip); - if (ret) - return ret; + /* Grab the lock */ + nand_get_device(chip); /* Shift to get first page */ page = (int)(instr->addr >> chip->page_shift); @@ -4499,8 +4482,8 @@ static void nand_sync(struct mtd_info *mtd) pr_debug("%s: called\n", __func__); - /* Grab the lock and see if the device is available */ - WARN_ON(nand_get_device(chip)); + /* Grab the lock */ + nand_get_device(chip); /* Release it and go back */ nand_release_device(chip); } @@ -4517,9 +4500,7 @@ static int nand_block_isbad(struct mtd_info *mtd, loff_t offs) int ret; /* Select the NAND device */ - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); nand_select_target(chip, chipnr); @@ -4565,8 +4546,6 @@ static int nand_suspend(struct mtd_info *mtd) mutex_lock(&chip->lock); if (chip->ops.suspend) ret = chip->ops.suspend(chip); - if (!ret) - chip->suspended = 1; mutex_unlock(&chip->lock); return ret; @@ -4580,15 +4559,10 @@ static void nand_resume(struct mtd_info *mtd) { struct nand_chip *chip = mtd_to_nand(mtd); + mutex_lock(&chip->lock); - if (chip->suspended) { - if (chip->ops.resume) - chip->ops.resume(chip); - chip->suspended = 0; - } else { - pr_err("%s called for a chip which is not in suspended state\n", - __func__); - } + if (chip->ops.resume) + chip->ops.resume(chip); mutex_unlock(&chip->lock); } diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h index b2f9dd3cbd69..1198a6548912 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -1237,9 +1237,7 @@ struct nand_secure_region { * @pagecache.page: Page number currently in the cache. -1 means no page is * currently cached * @buf_align: Minimum buffer alignment required by a platform - * @lock: Lock protecting the suspended field. Also used to serialize accesses - * to the NAND device - * @suspended: Set to 1 when the device is suspended, 0 when it's not + * @lock: Lock to serialize accesses to the NAND device * @cur_cs: Currently selected target. -1 means no target selected, otherwise we * should always have cur_cs >= 0 && cur_cs < nanddev_ntargets(). * NAND Controller drivers should not modify this value, but they're @@ -1293,7 +1291,6 @@ struct nand_chip { /* Internals */ struct mutex lock; - unsigned int suspended : 1; int cur_cs; int read_retries; struct nand_secure_region *secure_regions; -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 4/4] mtd: rawnand: remove suspended check @ 2021-10-25 9:27 ` Sean Nyekjaer 0 siblings, 0 replies; 12+ messages in thread From: Sean Nyekjaer @ 2021-10-25 9:27 UTC (permalink / raw) To: Boris Brezillon Cc: Sean Nyekjaer, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, Boris Brezillon, linux-mtd, linux-kernel Access is protected in upper MTD layer when MTD devices are suspended. Commit ("mtd: core: protect access to MTD devices while in suspend") introduces access protection, so it's safe to remove suspended checks. Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- drivers/mtd/nand/raw/nand_base.c | 52 ++++++++------------------------ include/linux/mtd/rawnand.h | 5 +-- 2 files changed, 14 insertions(+), 43 deletions(-) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 3d6c6e880520..aa2874ae3c4a 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -332,19 +332,11 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) * @chip: NAND chip structure * * Lock the device and its controller for exclusive access - * - * Return: -EBUSY if the chip has been suspended, 0 otherwise */ -static int nand_get_device(struct nand_chip *chip) +static void nand_get_device(struct nand_chip *chip) { mutex_lock(&chip->lock); - if (chip->suspended) { - mutex_unlock(&chip->lock); - return -EBUSY; - } mutex_lock(&chip->controller->lock); - - return 0; } /** @@ -573,10 +565,7 @@ static int nand_block_markbad_lowlevel(struct nand_chip *chip, loff_t ofs) nand_erase_nand(chip, &einfo, 0); /* Write bad block marker to OOB */ - ret = nand_get_device(chip); - if (ret) - return ret; - + nand_get_device(chip); ret = nand_markbad_bbm(chip, ofs); nand_release_device(chip); } @@ -3756,9 +3745,7 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from, ops->mode != MTD_OPS_RAW) return -ENOTSUPP; - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); if (!ops->datbuf) ret = nand_do_read_oob(chip, from, ops); @@ -4345,13 +4332,11 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to, struct mtd_oob_ops *ops) { struct nand_chip *chip = mtd_to_nand(mtd); - int ret; + int ret = 0; ops->retlen = 0; - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); switch (ops->mode) { case MTD_OPS_PLACE_OOB: @@ -4410,10 +4395,8 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr, if (nand_region_is_secured(chip, instr->addr, instr->len)) return -EIO; - /* Grab the lock and see if the device is available */ - ret = nand_get_device(chip); - if (ret) - return ret; + /* Grab the lock */ + nand_get_device(chip); /* Shift to get first page */ page = (int)(instr->addr >> chip->page_shift); @@ -4499,8 +4482,8 @@ static void nand_sync(struct mtd_info *mtd) pr_debug("%s: called\n", __func__); - /* Grab the lock and see if the device is available */ - WARN_ON(nand_get_device(chip)); + /* Grab the lock */ + nand_get_device(chip); /* Release it and go back */ nand_release_device(chip); } @@ -4517,9 +4500,7 @@ static int nand_block_isbad(struct mtd_info *mtd, loff_t offs) int ret; /* Select the NAND device */ - ret = nand_get_device(chip); - if (ret) - return ret; + nand_get_device(chip); nand_select_target(chip, chipnr); @@ -4565,8 +4546,6 @@ static int nand_suspend(struct mtd_info *mtd) mutex_lock(&chip->lock); if (chip->ops.suspend) ret = chip->ops.suspend(chip); - if (!ret) - chip->suspended = 1; mutex_unlock(&chip->lock); return ret; @@ -4580,15 +4559,10 @@ static void nand_resume(struct mtd_info *mtd) { struct nand_chip *chip = mtd_to_nand(mtd); + mutex_lock(&chip->lock); - if (chip->suspended) { - if (chip->ops.resume) - chip->ops.resume(chip); - chip->suspended = 0; - } else { - pr_err("%s called for a chip which is not in suspended state\n", - __func__); - } + if (chip->ops.resume) + chip->ops.resume(chip); mutex_unlock(&chip->lock); } diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h index b2f9dd3cbd69..1198a6548912 100644 --- a/include/linux/mtd/rawnand.h +++ b/include/linux/mtd/rawnand.h @@ -1237,9 +1237,7 @@ struct nand_secure_region { * @pagecache.page: Page number currently in the cache. -1 means no page is * currently cached * @buf_align: Minimum buffer alignment required by a platform - * @lock: Lock protecting the suspended field. Also used to serialize accesses - * to the NAND device - * @suspended: Set to 1 when the device is suspended, 0 when it's not + * @lock: Lock to serialize accesses to the NAND device * @cur_cs: Currently selected target. -1 means no target selected, otherwise we * should always have cur_cs >= 0 && cur_cs < nanddev_ntargets(). * NAND Controller drivers should not modify this value, but they're @@ -1293,7 +1291,6 @@ struct nand_chip { /* Internals */ struct mutex lock; - unsigned int suspended : 1; int cur_cs; int read_retries; struct nand_secure_region *secure_regions; -- 2.33.0 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-25 15:00 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-25 9:27 [PATCH v3 0/4] mtd: core: protect access to mtd devices while in suspend Sean Nyekjaer 2021-10-25 9:27 ` Sean Nyekjaer 2021-10-25 9:27 ` [PATCH v3 1/4] mtd: rawnand: nand_bbt: hide suspend/resume hooks while scanning bbt Sean Nyekjaer 2021-10-25 9:27 ` Sean Nyekjaer 2021-10-25 9:27 ` [PATCH v3 2/4] mtd: mtdconcat: add suspend lock handling Sean Nyekjaer 2021-10-25 9:27 ` Sean Nyekjaer 2021-10-25 14:59 ` Boris Brezillon 2021-10-25 14:59 ` Boris Brezillon 2021-10-25 9:27 ` [PATCH v3 3/4] mtd: core: protect access to MTD devices while in suspend Sean Nyekjaer 2021-10-25 9:27 ` Sean Nyekjaer 2021-10-25 9:27 ` [PATCH v3 4/4] mtd: rawnand: remove suspended check Sean Nyekjaer 2021-10-25 9:27 ` Sean Nyekjaer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.