Messages in this thread | | | Date | Fri, 8 Oct 2021 19:31:14 +0200 | From | Sean Nyekjaer <> | Subject | Re: [RFC PATCH 1/2] mtd: core: protect access to mtd devices while in suspend |
| |
On Fri, Oct 08, 2021 at 05:30:43PM +0200, Boris Brezillon wrote: > Hi Sean, > > Can you please submit that as a separate thread, ideally with an > incremented version number, a changelog and a reference to all your > previous attempts.
Yes, I'll do that with the next one.
> > On Fri, 8 Oct 2021 16:38:24 +0200 > Sean Nyekjaer <sean@geanix.com> wrote: > > > This will prevent reading/writing/erasing to a suspended mtd device. > > It will force mtd_write()/mtd_read()/mtd_erase() to wait for > > mtd_resume() to unlock access to mtd devices. > > I think this has to be done for all the hooks except ->_reboot(), > ->_get_device() and ->_put_device(). > > > > > Exec_op[0] speed things up, so we see this race when rawnand devices going > > Mention the commit directly: > > Commit ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op") speed > things up, so we see this race when rawnand devices going ... > > > into suspend. But it's actually "mtd: rawnand: Simplify the locking" that > > But it's actually commit 013e6292aaf5 ("mtd: rawnand: Simplify the > locking") that ... > > > allows it to return errors rather than locking, before that commit it would > > have waited for the rawnand device to resume. > > > > Tested on a iMX6ULL. > > > > [0]: > > ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op") > > > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking") > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > > You flagged yourself as the author even though you didn't really write > that code. I guess I'm fine with that, but I'd appreciate a > > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> > > here, at least. >
Of course, of course I forgot it... Still an RFC after all :)
> > --- > > > > Hope I got it all :) > > > > drivers/mtd/mtdcore.c | 57 ++++++++++++++++++++++++++++++++++++++++- > > include/linux/mtd/mtd.h | 36 ++++++++++++++++++-------- > > 2 files changed, 81 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index c8fd7f758938..3c93202e6cbb 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -36,6 +36,44 @@ > > > > struct backing_dev_info *mtd_bdi; > > > > +static void mtd_start_access(struct mtd_info *mtd) > > +{ > > + struct mtd_info *master = mtd_get_master(mtd); > > + > > + /* > > + * 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; > > + > > You need to remove the ->_suspend()/->_resume() implementation in > mtd_concat.c, otherwise you'll hit the case described in the comment.
Do you mean to remove concat_suspend() and concat_resume() together with the references to them?
> > BTW, did you test this series with lockdep enabled to make sure we > don't introduce a deadlock? >
Good you mentioned it... I thought the kernel had LOCKDEP enabled, but I guess it at some point got removed.
It reveals that mtd_read_oob() -> mtd_start_access() is using the suspend_lock rw_semaphore before it's initialized... But it's not complaining when going suspend and resuming, will continue to test with LOCKDEP enabled.
/Sean
> > + /* > > + * 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 == 0); > > + } > > +} > > + > > +static void mtd_end_access(struct mtd_info *mtd) > > +{ > > + struct mtd_info *master = mtd_get_master(mtd); > > + > > + if (!master->_suspend) > > + return; > > + > > + up_read(&master->master.suspend_lock); > > +} > > +
| |