From: Boris Brezillon <boris.brezillon@collabora.com> To: Sean Nyekjaer <sean@geanix.com> Cc: Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, Boris Brezillon <bbrezillon@kernel.org>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 1/2] mtd: core: protect access to mtd devices while in suspend Date: Fri, 8 Oct 2021 17:30:43 +0200 [thread overview] Message-ID: <20211008173043.6263ba80@collabora.com> (raw) In-Reply-To: <20211008143825.3717116-1-sean@geanix.com> 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. 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. > --- > > 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. BTW, did you test this series with lockdep enabled to make sure we don't introduce a deadlock? > + /* > + * 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); > +} > +
WARNING: multiple messages have this Message-ID (diff)
From: Boris Brezillon <boris.brezillon@collabora.com> To: Sean Nyekjaer <sean@geanix.com> Cc: Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, Boris Brezillon <bbrezillon@kernel.org>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 1/2] mtd: core: protect access to mtd devices while in suspend Date: Fri, 8 Oct 2021 17:30:43 +0200 [thread overview] Message-ID: <20211008173043.6263ba80@collabora.com> (raw) In-Reply-To: <20211008143825.3717116-1-sean@geanix.com> 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. 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. > --- > > 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. BTW, did you test this series with lockdep enabled to make sure we don't introduce a deadlock? > + /* > + * 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); > +} > + ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2021-10-08 15:31 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-04 6:56 [RFC PATCH] mtd: rawnand: use mutex to protect access while in suspend Sean Nyekjaer 2021-10-04 6:56 ` Sean Nyekjaer 2021-10-04 8:41 ` Boris Brezillon 2021-10-04 8:41 ` Boris Brezillon 2021-10-04 8:55 ` Sean Nyekjaer 2021-10-04 8:55 ` Sean Nyekjaer 2021-10-04 9:58 ` Boris Brezillon 2021-10-04 9:58 ` Boris Brezillon 2021-10-04 10:12 ` Sean Nyekjaer 2021-10-04 10:12 ` Sean Nyekjaer 2021-10-04 11:47 ` Boris Brezillon 2021-10-04 11:47 ` Boris Brezillon 2021-10-05 7:09 ` Sean Nyekjaer 2021-10-05 7:09 ` Sean Nyekjaer 2021-10-05 8:23 ` Boris Brezillon 2021-10-05 8:23 ` Boris Brezillon 2021-10-05 8:49 ` Sean Nyekjaer 2021-10-05 8:49 ` Sean Nyekjaer 2021-10-05 8:58 ` Boris Brezillon 2021-10-05 8:58 ` Boris Brezillon 2021-10-07 11:43 ` Sean Nyekjaer 2021-10-07 11:43 ` Sean Nyekjaer 2021-10-07 12:18 ` Boris Brezillon 2021-10-07 12:18 ` Boris Brezillon 2021-10-07 12:39 ` Sean Nyekjaer 2021-10-07 12:39 ` Sean Nyekjaer 2021-10-07 13:14 ` Boris Brezillon 2021-10-07 13:14 ` Boris Brezillon 2021-10-08 10:04 ` Sean Nyekjaer 2021-10-08 10:04 ` Sean Nyekjaer 2021-10-08 11:20 ` Boris Brezillon 2021-10-08 11:20 ` Boris Brezillon 2021-10-08 11:54 ` Sean Nyekjaer 2021-10-08 11:54 ` Sean Nyekjaer 2021-10-08 12:15 ` Boris Brezillon 2021-10-08 12:15 ` Boris Brezillon 2021-10-08 14:38 ` [RFC PATCH 1/2] mtd: core: protect access to mtd devices " Sean Nyekjaer 2021-10-08 14:38 ` Sean Nyekjaer 2021-10-08 15:30 ` Boris Brezillon [this message] 2021-10-08 15:30 ` Boris Brezillon 2021-10-08 17:31 ` Sean Nyekjaer 2021-10-08 17:31 ` Sean Nyekjaer 2021-10-08 15:35 ` Miquel Raynal 2021-10-08 15:35 ` Miquel Raynal 2021-10-08 16:08 ` Boris Brezillon 2021-10-08 16:08 ` Boris Brezillon 2021-10-08 17:50 ` Sean Nyekjaer 2021-10-08 17:50 ` Sean Nyekjaer 2021-10-08 14:38 ` [RFC PATCH 2/2] mtd: rawnand: remove suspended check Sean Nyekjaer 2021-10-08 14:38 ` Sean Nyekjaer 2021-10-08 22:05 ` kernel test robot 2021-10-08 22:05 ` kernel test robot 2021-10-08 22:47 ` kernel test robot 2021-10-08 22:47 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211008173043.6263ba80@collabora.com \ --to=boris.brezillon@collabora.com \ --cc=bbrezillon@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=miquel.raynal@bootlin.com \ --cc=richard@nod.at \ --cc=sean@geanix.com \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.