Messages in this thread Patch in this message | | | Date | Tue, 23 Nov 2021 13:03:52 +0100 | Subject | Re: [PATCH v5 3/4] mtd: core: protect access to MTD devices while in suspend | From | Marek Szyprowski <> |
| |
Hi,
On 02.11.2021 12:02, Sean Nyekjaer wrote: > 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. > > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
This patch landed recently in linux-next as commit 9d6abd489e70 ("mtd: core: protect access to MTD devices while in suspend"). I found that it triggers the following warning on my test systems:
INFO: trying to register non-static key. The code is fine but needs lockdep annotation, or maybe you didn't initialize this object before use? turning off the locking correctness validator. CPU: 1 PID: 1606 Comm: reboot Not tainted 5.16.0-rc1+ #4165 Hardware name: linux,dummy-virt (DT) Call trace: dump_backtrace+0x0/0x1ac show_stack+0x18/0x24 dump_stack_lvl+0x8c/0xb8 dump_stack+0x18/0x34 register_lock_class+0x4a0/0x4cc __lock_acquire+0x78/0x20cc lock_acquire.part.0+0xe0/0x230 lock_acquire+0x68/0x84 down_write+0x64/0xe0 physmap_flash_shutdown+0x60/0x140 platform_shutdown+0x28/0x40 device_shutdown+0x160/0x340 __do_sys_reboot+0x1f8/0x2a0 __arm64_sys_reboot+0x28/0x34 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x4c/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x1a4/0x1a8 Flash device refused suspend due to active operation (state 20) ------------[ cut here ]------------ DEBUG_RWSEMS_WARN_ON(sem->magic != sem): count = 0x1, magic = 0x0, owner = 0xffff588b4547c740, curr 0xffff588b4547c740, list not empty WARNING: CPU: 1 PID: 1606 at kernel/locking/rwsem.c:1322 up_write+0x144/0x1a0 Modules linked in: bluetooth ecdh_generic ecc rfkill ipv6 CPU: 1 PID: 1606 Comm: reboot Not tainted 5.16.0-rc1+ #4165 Hardware name: linux,dummy-virt (DT) pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : up_write+0x144/0x1a0 lr : up_write+0x144/0x1a0 sp : ffff8000109ebbd0 x29: ffff8000109ebbd0 x28: ffff588b4547c740 x27: 0000000000000000 x26: ffffce0238d56470 x25: 0000000000000008 x24: ffffce0239bba030 x23: ffff588b451938b0 x22: 0000000000000000 x21: ffff588b44046c80 x20: ffffce02397a2000 x19: ffff588b451938b0 x18: 0000000000000030 x17: 0000000000000000 x16: 0000000000000000 x15: ffffffffffffffff x14: 0000000000000005 x13: ffffce02397c5198 x12: 0000000000000390 x11: 0000000000000130 x10: ffffce023981d198 x9 : 00000000fffff000 x8 : ffffce02397c5198 x7 : ffffce023981d198 x6 : 0000000000000000 x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000 x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff588b4547c740 Call trace: up_write+0x144/0x1a0 physmap_flash_shutdown+0x13c/0x140 platform_shutdown+0x28/0x40 device_shutdown+0x160/0x340 __do_sys_reboot+0x1f8/0x2a0 __arm64_sys_reboot+0x28/0x34 invoke_syscall+0x48/0x114 el0_svc_common.constprop.0+0x60/0x11c do_el0_svc_compat+0x1c/0x50 el0_svc_compat+0x4c/0x100 el0t_32_sync_handler+0x90/0x140 el0t_32_sync+0x1a4/0x1a8 irq event stamp: 2541 hardirqs last enabled at (2541): [<ffffce02382d94c8>] _raw_spin_unlock_irq+0x44/0x90 hardirqs last disabled at (2540): [<ffffce02382d98cc>] _raw_spin_lock_irq+0xac/0xb0 softirqs last enabled at (2278): [<ffffce0237210470>] _stext+0x470/0x5e8 softirqs last disabled at (2273): [<ffffce023729d904>] __irq_exit_rcu+0x180/0x1ac ---[ end trace d06160a193b668c2 ]--- Flash device refused suspend due to active operation (state 20)
Reverting $subject patch on top of linux-next hides the warning. The above log has been gathered on QEMU/arm64 'virt' machine, during the reboot operation. If you need more information how to reproduce it, let me know.
> --- > drivers/mtd/mtdcore.c | 115 +++++++++++++++++++++++++++++++++++----- > include/linux/mtd/mtd.h | 81 +++++++++++++++++++++++----- > 2 files changed, 169 insertions(+), 27 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 153229198947..f02b602b3fa9 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; > > @@ -1464,11 +1481,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 +1499,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 +2013,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 +2032,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 +2052,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 +2071,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 +2098,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 +2117,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 +2154,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 +2168,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 +2193,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 +2218,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 +2239,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 +2260,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 +2283,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)
Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
| |