lkml.org 
[lkml]   [2021]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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);
> > +}
> > +

\
 
 \ /
  Last update: 2021-10-08 19:31    [W:0.066 / U:1.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site