This will prevent nand_get_device() from returning -EBUSY.
It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
access to the mtd device.
Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
that will in turn hard error on every error returened.
We have seen during ubifs tries to call mtd_write before the mtd device
is resumed.
Exec_op[0] speed things up, so we see this race when the device is
resuming. But it's actually "mtd: rawnand: Simplify the locking" that
allows it to return -EBUSY, before that commit it would have waited for
the mtd 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 <[email protected]>
---
I did this a RFC as we probably will need to remove the suspended
variable as it's kinda made obsolute by this change.
Should we introduce a new mutex? Or maybe a spin_lock?
drivers/mtd/nand/raw/nand_base.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..0ea343404cac 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4567,7 +4567,6 @@ static int nand_suspend(struct mtd_info *mtd)
ret = chip->ops.suspend(chip);
if (!ret)
chip->suspended = 1;
- mutex_unlock(&chip->lock);
return ret;
}
@@ -4580,7 +4579,6 @@ 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);
--
2.33.0
On Mon, 4 Oct 2021 08:56:09 +0200
Sean Nyekjaer <[email protected]> wrote:
> This will prevent nand_get_device() from returning -EBUSY.
> It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> access to the mtd device.
>
> Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> that will in turn hard error on every error returened.
> We have seen during ubifs tries to call mtd_write before the mtd device
> is resumed.
I think the problem is here. Why would UBIFS/UBI try to write something
to a device that's not resumed yet (or has been suspended already, if
you hit this in the suspend path).
>
> Exec_op[0] speed things up, so we see this race when the device is
> resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> allows it to return -EBUSY, before that commit it would have waited for
> the mtd device to resume.
Uh, wait. If nand_resume() was called before any writes/reads this
wouldn't happen. IMHO, the problem is not that we return -EBUSY without
blocking, the problem is that someone issues a write/read before calling
mtd_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 <[email protected]>
> ---
>
> I did this a RFC as we probably will need to remove the suspended
> variable as it's kinda made obsolute by this change.
> Should we introduce a new mutex? Or maybe a spin_lock?
>
> drivers/mtd/nand/raw/nand_base.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3d6c6e880520..0ea343404cac 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4567,7 +4567,6 @@ static int nand_suspend(struct mtd_info *mtd)
> ret = chip->ops.suspend(chip);
> if (!ret)
> chip->suspended = 1;
> - mutex_unlock(&chip->lock);
Hm, I'm not sure keeping the lock when you're in a suspended state
is a good idea. It just papers over another bug IMO (see above).
>
> return ret;
> }
> @@ -4580,7 +4579,6 @@ 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);
On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> On Mon, 4 Oct 2021 08:56:09 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > This will prevent nand_get_device() from returning -EBUSY.
> > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > access to the mtd device.
> >
> > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > that will in turn hard error on every error returened.
> > We have seen during ubifs tries to call mtd_write before the mtd device
> > is resumed.
>
> I think the problem is here. Why would UBIFS/UBI try to write something
> to a device that's not resumed yet (or has been suspended already, if
> you hit this in the suspend path).
>
> >
> > Exec_op[0] speed things up, so we see this race when the device is
> > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > allows it to return -EBUSY, before that commit it would have waited for
> > the mtd device to resume.
>
> Uh, wait. If nand_resume() was called before any writes/reads this
> wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> blocking, the problem is that someone issues a write/read before calling
> mtd_resume().
>
The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
"""
Last important change to mention: we now return -EBUSY when someone
tries to access a device that as been suspended, and propagate this
error to the upper layer.
"""
IMHO "mtd: rawnand: Simplify the locking" should never had been merged
before the upper layers was fixed to handle -EBUSY. ;)
Which they still not are...
Yes, guess there is data in the ubifs queue when going into suspend,
then the ubifs kthread is starting writing when the cpu resumes.
Before mtd_resume() and other pm_resume() handles are called.
How would you have ubifs to wait for mtd_resume()? If you don't like
this mutex solution?
> >
> > Tested on a iMX6ULL.
> >
> > [0]:
> > ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op")
> >
> > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> > Signed-off-by: Sean Nyekjaer <[email protected]>
> > ---
> >
> > I did this a RFC as we probably will need to remove the suspended
> > variable as it's kinda made obsolute by this change.
> > Should we introduce a new mutex? Or maybe a spin_lock?
> >
> > drivers/mtd/nand/raw/nand_base.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 3d6c6e880520..0ea343404cac 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -4567,7 +4567,6 @@ static int nand_suspend(struct mtd_info *mtd)
> > ret = chip->ops.suspend(chip);
> > if (!ret)
> > chip->suspended = 1;
> > - mutex_unlock(&chip->lock);
>
> Hm, I'm not sure keeping the lock when you're in a suspended state
> is a good idea. It just papers over another bug IMO (see above).
>
> >
> > return ret;
> > }
> > @@ -4580,7 +4579,6 @@ 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);
>
On Mon, 4 Oct 2021 10:55:09 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> > On Mon, 4 Oct 2021 08:56:09 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > This will prevent nand_get_device() from returning -EBUSY.
> > > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > > access to the mtd device.
> > >
> > > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > > that will in turn hard error on every error returened.
> > > We have seen during ubifs tries to call mtd_write before the mtd device
> > > is resumed.
> >
> > I think the problem is here. Why would UBIFS/UBI try to write something
> > to a device that's not resumed yet (or has been suspended already, if
> > you hit this in the suspend path).
> >
> > >
> > > Exec_op[0] speed things up, so we see this race when the device is
> > > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > > allows it to return -EBUSY, before that commit it would have waited for
> > > the mtd device to resume.
> >
> > Uh, wait. If nand_resume() was called before any writes/reads this
> > wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> > blocking, the problem is that someone issues a write/read before calling
> > mtd_resume().
> >
>
> The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
>
> """
> Last important change to mention: we now return -EBUSY when someone
> tries to access a device that as been suspended, and propagate this
> error to the upper layer.
> """
>
> IMHO "mtd: rawnand: Simplify the locking" should never had been merged
> before the upper layers was fixed to handle -EBUSY. ;)
> Which they still not are...
That's not really the problem here. Upper layers should never get
-EBUSY in the first place if the MTD device was resumed before the UBI
device. Looks like we have a missing UBI -> MTD parenting link, which
would explain why things don't get resumed in the right order. Can you
try with the following diff applied?
---
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index f399edc82191..1981ce8f3a26 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -905,6 +905,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
ubi_num, ubi->dev.release = dev_release;
ubi->dev.class = &ubi_class;
ubi->dev.groups = ubi_dev_groups;
+ ubi->dev.parent = &mtd->dev;
ubi->mtd = mtd;
ubi->ubi_num = ubi_num;
On Mon, Oct 04, 2021 at 11:58:17AM +0200, Boris Brezillon wrote:
> On Mon, 4 Oct 2021 10:55:09 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> > > On Mon, 4 Oct 2021 08:56:09 +0200
> > > Sean Nyekjaer <[email protected]> wrote:
> > >
> > > > This will prevent nand_get_device() from returning -EBUSY.
> > > > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > > > access to the mtd device.
> > > >
> > > > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > > > that will in turn hard error on every error returened.
> > > > We have seen during ubifs tries to call mtd_write before the mtd device
> > > > is resumed.
> > >
> > > I think the problem is here. Why would UBIFS/UBI try to write something
> > > to a device that's not resumed yet (or has been suspended already, if
> > > you hit this in the suspend path).
> > >
> > > >
> > > > Exec_op[0] speed things up, so we see this race when the device is
> > > > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > > > allows it to return -EBUSY, before that commit it would have waited for
> > > > the mtd device to resume.
> > >
> > > Uh, wait. If nand_resume() was called before any writes/reads this
> > > wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> > > blocking, the problem is that someone issues a write/read before calling
> > > mtd_resume().
> > >
> >
> > The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
> >
> > """
> > Last important change to mention: we now return -EBUSY when someone
> > tries to access a device that as been suspended, and propagate this
> > error to the upper layer.
> > """
> >
> > IMHO "mtd: rawnand: Simplify the locking" should never had been merged
> > before the upper layers was fixed to handle -EBUSY. ;)
> > Which they still not are...
>
> That's not really the problem here. Upper layers should never get
> -EBUSY in the first place if the MTD device was resumed before the UBI
> device. Looks like we have a missing UBI -> MTD parenting link, which
> would explain why things don't get resumed in the right order. Can you
> try with the following diff applied?
>
> ---
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index f399edc82191..1981ce8f3a26 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -905,6 +905,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
> ubi_num, ubi->dev.release = dev_release;
> ubi->dev.class = &ubi_class;
> ubi->dev.groups = ubi_dev_groups;
> + ubi->dev.parent = &mtd->dev;
>
> ubi->mtd = mtd;
> ubi->ubi_num = ubi_num;
>
No change:
[ 71.739193] Filesystems sync: 34.212 seconds
[ 71.755044] Freezing user space processes ... (elapsed 0.004 seconds) done.
[ 71.767289] OOM killer disabled.
[ 71.770552] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
[ 71.782182] printk: Suspending console(s) (use no_console_suspend to debug)
[ 71.824391] nand_suspend
[ 71.825177] gpmi_pm_suspend
[ 71.825676] PM: suspend devices took 0.040 seconds
[ 71.825971] nand_write_oob - nand_get_device() returned -EBUSY
[ 71.825985] ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 986:65536, written 0 bytes
[ 71.826029] CPU: 0 PID: 7 Comm: kworker/u2:0 Not tainted 5.15.0-rc3-dirty #43
[ 71.826043] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[ 71.826054] Workqueue: writeback wb_workfn (flush-ubifs_0_8)
[ 71.826094] [<c010da84>] (unwind_backtrace) from [<c010a1b4>] (show_stack+0x10/0x14)
[ 71.826122] [<c010a1b4>] (show_stack) from [<c0989c30>] (dump_stack_lvl+0x40/0x4c)
[ 71.826151] [<c0989c30>] (dump_stack_lvl) from [<c05ed690>] (ubi_io_write+0x510/0x6b0)
[ 71.826178] [<c05ed690>] (ubi_io_write) from [<c05ea2f0>] (ubi_eba_write_leb+0xd0/0x968)
[ 71.826204] [<c05ea2f0>] (ubi_eba_write_leb) from [<c05e8754>] (ubi_leb_write+0xd0/0xe8)
[ 71.826232] [<c05e8754>] (ubi_leb_write) from [<c03d67bc>] (ubifs_leb_write+0x68/0x104)
[ 71.826263] [<c03d67bc>] (ubifs_leb_write) from [<c03d79e8>] (ubifs_wbuf_write_nolock+0x28c/0x74c)
[ 71.826291] [<c03d79e8>] (ubifs_wbuf_write_nolock) from [<c03ca18c>] (ubifs_jnl_write_data+0x1b8/0x2b4)
[ 71.826319] [<c03ca18c>] (ubifs_jnl_write_data) from [<c03cd184>] (do_writepage+0x190/0x284)
[ 71.826342] [<c03cd184>] (do_writepage) from [<c023083c>] (__writepage+0x14/0x68)
[ 71.826367] [<c023083c>] (__writepage) from [<c0231748>] (write_cache_pages+0x1c8/0x3f0)
[ 71.826390] [<c0231748>] (write_cache_pages) from [<c0233854>] (do_writepages+0xcc/0x1f4)
[ 71.826413] [<c0233854>] (do_writepages) from [<c02d03dc>] (__writeback_single_inode+0x2c/0x1b4)
[ 71.826440] [<c02d03dc>] (__writeback_single_inode) from [<c02d0a64>] (writeback_sb_inodes+0x200/0x470)
[ 71.826466] [<c02d0a64>] (writeback_sb_inodes) from [<c02d0d10>] (__writeback_inodes_wb+0x3c/0xf4)
[ 71.826493] [<c02d0d10>] (__writeback_inodes_wb) from [<c02d0f58>] (wb_writeback+0x190/0x1f0)
[ 71.826520] [<c02d0f58>] (wb_writeback) from [<c02d21d8>] (wb_workfn+0x2c0/0x3d4)
[ 71.826545] [<c02d21d8>] (wb_workfn) from [<c013ac04>] (process_one_work+0x1e0/0x440)
[ 71.826574] [<c013ac04>] (process_one_work) from [<c013aeac>] (worker_thread+0x48/0x594)
[ 71.826600] [<c013aeac>] (worker_thread) from [<c0142364>] (kthread+0x134/0x15c)
[ 71.826625] [<c0142364>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
[...]
[ 71.921673] gpmi_pm_resume
[ 71.923319] nand_resume
[ 71.936120] PM: resume devices took 0.100 seconds
[ 72.314551] ci_hdrc ci_hdrc.0: freeing queued request
[ 72.521656] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
[ 75.006404] OOM killer enabled.
[ 75.009562] Restarting tasks ...
[ 75.074123] done.
[ 75.095540] PM: suspend exit
With the RFC PATCH:
[ 3702.682122] Filesystems sync: 33.416 seconds
[ 3702.695350] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 3702.704218] OOM killer disabled.
[ 3702.707559] Freezing remaining freezable tasks ... (elapsed 0.003 seconds) done.
[ 3702.718696] printk: Suspending console(s) (use no_console_suspend to debug)
[ 3702.757660] nand_suspend
[ 3702.758577] gpmi_pm_suspend
[ 3702.759072] PM: suspend devices took 0.040 seconds
[ 3702.761618] Disabling non-boot CPUs ...
[ 3702.854985] gpmi_pm_resume
[ 3702.856623] nand_resume
[ 3702.867796] PM: resume devices took 0.110 seconds
[ 3702.895019] OOM killer enabled.
[ 3702.898291] Restarting tasks ... done.
[ 3702.950723] PM: suspend exit
On Mon, 4 Oct 2021 12:12:46 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Mon, Oct 04, 2021 at 11:58:17AM +0200, Boris Brezillon wrote:
> > On Mon, 4 Oct 2021 10:55:09 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> > > > On Mon, 4 Oct 2021 08:56:09 +0200
> > > > Sean Nyekjaer <[email protected]> wrote:
> > > >
> > > > > This will prevent nand_get_device() from returning -EBUSY.
> > > > > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > > > > access to the mtd device.
> > > > >
> > > > > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > > > > that will in turn hard error on every error returened.
> > > > > We have seen during ubifs tries to call mtd_write before the mtd device
> > > > > is resumed.
> > > >
> > > > I think the problem is here. Why would UBIFS/UBI try to write something
> > > > to a device that's not resumed yet (or has been suspended already, if
> > > > you hit this in the suspend path).
> > > >
> > > > >
> > > > > Exec_op[0] speed things up, so we see this race when the device is
> > > > > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > > > > allows it to return -EBUSY, before that commit it would have waited for
> > > > > the mtd device to resume.
> > > >
> > > > Uh, wait. If nand_resume() was called before any writes/reads this
> > > > wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> > > > blocking, the problem is that someone issues a write/read before calling
> > > > mtd_resume().
> > > >
> > >
> > > The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
> > >
> > > """
> > > Last important change to mention: we now return -EBUSY when someone
> > > tries to access a device that as been suspended, and propagate this
> > > error to the upper layer.
> > > """
> > >
> > > IMHO "mtd: rawnand: Simplify the locking" should never had been merged
> > > before the upper layers was fixed to handle -EBUSY. ;)
> > > Which they still not are...
> >
> > That's not really the problem here. Upper layers should never get
> > -EBUSY in the first place if the MTD device was resumed before the UBI
> > device. Looks like we have a missing UBI -> MTD parenting link, which
> > would explain why things don't get resumed in the right order. Can you
> > try with the following diff applied?
> >
> > ---
> > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > index f399edc82191..1981ce8f3a26 100644
> > --- a/drivers/mtd/ubi/build.c
> > +++ b/drivers/mtd/ubi/build.c
> > @@ -905,6 +905,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
> > ubi_num, ubi->dev.release = dev_release;
> > ubi->dev.class = &ubi_class;
> > ubi->dev.groups = ubi_dev_groups;
> > + ubi->dev.parent = &mtd->dev;
> >
> > ubi->mtd = mtd;
> > ubi->ubi_num = ubi_num;
> >
>
> No change:
> [ 71.739193] Filesystems sync: 34.212 seconds
> [ 71.755044] Freezing user space processes ... (elapsed 0.004 seconds) done.
> [ 71.767289] OOM killer disabled.
> [ 71.770552] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
> [ 71.782182] printk: Suspending console(s) (use no_console_suspend to debug)
> [ 71.824391] nand_suspend
> [ 71.825177] gpmi_pm_suspend
> [ 71.825676] PM: suspend devices took 0.040 seconds
> [ 71.825971] nand_write_oob - nand_get_device() returned -EBUSY
> [ 71.825985] ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 986:65536, written 0 bytes
> [ 71.826029] CPU: 0 PID: 7 Comm: kworker/u2:0 Not tainted 5.15.0-rc3-dirty #43
> [ 71.826043] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [ 71.826054] Workqueue: writeback wb_workfn (flush-ubifs_0_8)
> [ 71.826094] [<c010da84>] (unwind_backtrace) from [<c010a1b4>] (show_stack+0x10/0x14)
> [ 71.826122] [<c010a1b4>] (show_stack) from [<c0989c30>] (dump_stack_lvl+0x40/0x4c)
> [ 71.826151] [<c0989c30>] (dump_stack_lvl) from [<c05ed690>] (ubi_io_write+0x510/0x6b0)
> [ 71.826178] [<c05ed690>] (ubi_io_write) from [<c05ea2f0>] (ubi_eba_write_leb+0xd0/0x968)
> [ 71.826204] [<c05ea2f0>] (ubi_eba_write_leb) from [<c05e8754>] (ubi_leb_write+0xd0/0xe8)
> [ 71.826232] [<c05e8754>] (ubi_leb_write) from [<c03d67bc>] (ubifs_leb_write+0x68/0x104)
> [ 71.826263] [<c03d67bc>] (ubifs_leb_write) from [<c03d79e8>] (ubifs_wbuf_write_nolock+0x28c/0x74c)
> [ 71.826291] [<c03d79e8>] (ubifs_wbuf_write_nolock) from [<c03ca18c>] (ubifs_jnl_write_data+0x1b8/0x2b4)
> [ 71.826319] [<c03ca18c>] (ubifs_jnl_write_data) from [<c03cd184>] (do_writepage+0x190/0x284)
> [ 71.826342] [<c03cd184>] (do_writepage) from [<c023083c>] (__writepage+0x14/0x68)
> [ 71.826367] [<c023083c>] (__writepage) from [<c0231748>] (write_cache_pages+0x1c8/0x3f0)
> [ 71.826390] [<c0231748>] (write_cache_pages) from [<c0233854>] (do_writepages+0xcc/0x1f4)
> [ 71.826413] [<c0233854>] (do_writepages) from [<c02d03dc>] (__writeback_single_inode+0x2c/0x1b4)
> [ 71.826440] [<c02d03dc>] (__writeback_single_inode) from [<c02d0a64>] (writeback_sb_inodes+0x200/0x470)
> [ 71.826466] [<c02d0a64>] (writeback_sb_inodes) from [<c02d0d10>] (__writeback_inodes_wb+0x3c/0xf4)
> [ 71.826493] [<c02d0d10>] (__writeback_inodes_wb) from [<c02d0f58>] (wb_writeback+0x190/0x1f0)
> [ 71.826520] [<c02d0f58>] (wb_writeback) from [<c02d21d8>] (wb_workfn+0x2c0/0x3d4)
> [ 71.826545] [<c02d21d8>] (wb_workfn) from [<c013ac04>] (process_one_work+0x1e0/0x440)
> [ 71.826574] [<c013ac04>] (process_one_work) from [<c013aeac>] (worker_thread+0x48/0x594)
> [ 71.826600] [<c013aeac>] (worker_thread) from [<c0142364>] (kthread+0x134/0x15c)
> [ 71.826625] [<c0142364>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
I'm not entirely sure, but given the timing, it looks like this
actually happens in the suspend path, not it the resume path. What I
don't get is why we still have a kernel thread running at that point.
>
> [...]
>
> [ 71.921673] gpmi_pm_resume
> [ 71.923319] nand_resume
> [ 71.936120] PM: resume devices took 0.100 seconds
> [ 72.314551] ci_hdrc ci_hdrc.0: freeing queued request
> [ 72.521656] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
> [ 75.006404] OOM killer enabled.
> [ 75.009562] Restarting tasks ...
> [ 75.074123] done.
> [ 75.095540] PM: suspend exit
>
> With the RFC PATCH:
> [ 3702.682122] Filesystems sync: 33.416 seconds
> [ 3702.695350] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [ 3702.704218] OOM killer disabled.
> [ 3702.707559] Freezing remaining freezable tasks ... (elapsed 0.003 seconds) done.
> [ 3702.718696] printk: Suspending console(s) (use no_console_suspend to debug)
> [ 3702.757660] nand_suspend
> [ 3702.758577] gpmi_pm_suspend
> [ 3702.759072] PM: suspend devices took 0.040 seconds
> [ 3702.761618] Disabling non-boot CPUs ...
> [ 3702.854985] gpmi_pm_resume
> [ 3702.856623] nand_resume
> [ 3702.867796] PM: resume devices took 0.110 seconds
> [ 3702.895019] OOM killer enabled.
> [ 3702.898291] Restarting tasks ... done.
> [ 3702.950723] PM: suspend exit
On Mon, Oct 04, 2021 at 01:47:00PM +0200, Boris Brezillon wrote:
> On Mon, 4 Oct 2021 12:12:46 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > On Mon, Oct 04, 2021 at 11:58:17AM +0200, Boris Brezillon wrote:
> > > On Mon, 4 Oct 2021 10:55:09 +0200
> > > Sean Nyekjaer <[email protected]> wrote:
> > >
> > > > On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> > > > > On Mon, 4 Oct 2021 08:56:09 +0200
> > > > > Sean Nyekjaer <[email protected]> wrote:
> > > > >
> > > > > > This will prevent nand_get_device() from returning -EBUSY.
> > > > > > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > > > > > access to the mtd device.
> > > > > >
> > > > > > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > > > > > that will in turn hard error on every error returened.
> > > > > > We have seen during ubifs tries to call mtd_write before the mtd device
> > > > > > is resumed.
> > > > >
> > > > > I think the problem is here. Why would UBIFS/UBI try to write something
> > > > > to a device that's not resumed yet (or has been suspended already, if
> > > > > you hit this in the suspend path).
> > > > >
> > > > > >
> > > > > > Exec_op[0] speed things up, so we see this race when the device is
> > > > > > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > > > > > allows it to return -EBUSY, before that commit it would have waited for
> > > > > > the mtd device to resume.
> > > > >
> > > > > Uh, wait. If nand_resume() was called before any writes/reads this
> > > > > wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> > > > > blocking, the problem is that someone issues a write/read before calling
> > > > > mtd_resume().
> > > > >
> > > >
> > > > The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
> > > >
> > > > """
> > > > Last important change to mention: we now return -EBUSY when someone
> > > > tries to access a device that as been suspended, and propagate this
> > > > error to the upper layer.
> > > > """
> > > >
> > > > IMHO "mtd: rawnand: Simplify the locking" should never had been merged
> > > > before the upper layers was fixed to handle -EBUSY. ;)
> > > > Which they still not are...
> > >
> > > That's not really the problem here. Upper layers should never get
> > > -EBUSY in the first place if the MTD device was resumed before the UBI
> > > device. Looks like we have a missing UBI -> MTD parenting link, which
> > > would explain why things don't get resumed in the right order. Can you
> > > try with the following diff applied?
> > >
> > > ---
> > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > > index f399edc82191..1981ce8f3a26 100644
> > > --- a/drivers/mtd/ubi/build.c
> > > +++ b/drivers/mtd/ubi/build.c
> > > @@ -905,6 +905,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
> > > ubi_num, ubi->dev.release = dev_release;
> > > ubi->dev.class = &ubi_class;
> > > ubi->dev.groups = ubi_dev_groups;
> > > + ubi->dev.parent = &mtd->dev;
> > >
> > > ubi->mtd = mtd;
> > > ubi->ubi_num = ubi_num;
> > >
> >
> > No change:
> > [ 71.739193] Filesystems sync: 34.212 seconds
> > [ 71.755044] Freezing user space processes ... (elapsed 0.004 seconds) done.
> > [ 71.767289] OOM killer disabled.
> > [ 71.770552] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
> > [ 71.782182] printk: Suspending console(s) (use no_console_suspend to debug)
> > [ 71.824391] nand_suspend
> > [ 71.825177] gpmi_pm_suspend
> > [ 71.825676] PM: suspend devices took 0.040 seconds
> > [ 71.825971] nand_write_oob - nand_get_device() returned -EBUSY
> > [ 71.825985] ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 986:65536, written 0 bytes
> > [ 71.826029] CPU: 0 PID: 7 Comm: kworker/u2:0 Not tainted 5.15.0-rc3-dirty #43
> > [ 71.826043] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> > [ 71.826054] Workqueue: writeback wb_workfn (flush-ubifs_0_8)
> > [ 71.826094] [<c010da84>] (unwind_backtrace) from [<c010a1b4>] (show_stack+0x10/0x14)
> > [ 71.826122] [<c010a1b4>] (show_stack) from [<c0989c30>] (dump_stack_lvl+0x40/0x4c)
> > [ 71.826151] [<c0989c30>] (dump_stack_lvl) from [<c05ed690>] (ubi_io_write+0x510/0x6b0)
> > [ 71.826178] [<c05ed690>] (ubi_io_write) from [<c05ea2f0>] (ubi_eba_write_leb+0xd0/0x968)
> > [ 71.826204] [<c05ea2f0>] (ubi_eba_write_leb) from [<c05e8754>] (ubi_leb_write+0xd0/0xe8)
> > [ 71.826232] [<c05e8754>] (ubi_leb_write) from [<c03d67bc>] (ubifs_leb_write+0x68/0x104)
> > [ 71.826263] [<c03d67bc>] (ubifs_leb_write) from [<c03d79e8>] (ubifs_wbuf_write_nolock+0x28c/0x74c)
> > [ 71.826291] [<c03d79e8>] (ubifs_wbuf_write_nolock) from [<c03ca18c>] (ubifs_jnl_write_data+0x1b8/0x2b4)
> > [ 71.826319] [<c03ca18c>] (ubifs_jnl_write_data) from [<c03cd184>] (do_writepage+0x190/0x284)
> > [ 71.826342] [<c03cd184>] (do_writepage) from [<c023083c>] (__writepage+0x14/0x68)
> > [ 71.826367] [<c023083c>] (__writepage) from [<c0231748>] (write_cache_pages+0x1c8/0x3f0)
> > [ 71.826390] [<c0231748>] (write_cache_pages) from [<c0233854>] (do_writepages+0xcc/0x1f4)
> > [ 71.826413] [<c0233854>] (do_writepages) from [<c02d03dc>] (__writeback_single_inode+0x2c/0x1b4)
> > [ 71.826440] [<c02d03dc>] (__writeback_single_inode) from [<c02d0a64>] (writeback_sb_inodes+0x200/0x470)
> > [ 71.826466] [<c02d0a64>] (writeback_sb_inodes) from [<c02d0d10>] (__writeback_inodes_wb+0x3c/0xf4)
> > [ 71.826493] [<c02d0d10>] (__writeback_inodes_wb) from [<c02d0f58>] (wb_writeback+0x190/0x1f0)
> > [ 71.826520] [<c02d0f58>] (wb_writeback) from [<c02d21d8>] (wb_workfn+0x2c0/0x3d4)
> > [ 71.826545] [<c02d21d8>] (wb_workfn) from [<c013ac04>] (process_one_work+0x1e0/0x440)
> > [ 71.826574] [<c013ac04>] (process_one_work) from [<c013aeac>] (worker_thread+0x48/0x594)
> > [ 71.826600] [<c013aeac>] (worker_thread) from [<c0142364>] (kthread+0x134/0x15c)
> > [ 71.826625] [<c0142364>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
>
> I'm not entirely sure, but given the timing, it looks like this
> actually happens in the suspend path, not it the resume path. What I
> don't get is why we still have a kernel thread running at that point.
Have you seen the reproducer script?
---
root@iwg26-v1:/data/root# cat /data/crash.sh
#!/bin/sh -x
echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
rm /data/test50M
dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
cp /tmp/test50M /data/ &
sleep 1
echo mem > /sys/power/state
---
As seen in the log above disk is synced before suspend.
cp is continuing to copy data to ubifs.
And then user space processes are frozen.
At this point the kernel thread would have unwritten data.
We tried to solve this with:
https://lkml.org/lkml/2021/9/1/280
/Sean
On Tue, 5 Oct 2021 09:09:30 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Mon, Oct 04, 2021 at 01:47:00PM +0200, Boris Brezillon wrote:
> > On Mon, 4 Oct 2021 12:12:46 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > On Mon, Oct 04, 2021 at 11:58:17AM +0200, Boris Brezillon wrote:
> > > > On Mon, 4 Oct 2021 10:55:09 +0200
> > > > Sean Nyekjaer <[email protected]> wrote:
> > > >
> > > > > On Mon, Oct 04, 2021 at 10:41:47AM +0200, Boris Brezillon wrote:
> > > > > > On Mon, 4 Oct 2021 08:56:09 +0200
> > > > > > Sean Nyekjaer <[email protected]> wrote:
> > > > > >
> > > > > > > This will prevent nand_get_device() from returning -EBUSY.
> > > > > > > It will force mtd_write()/mtd_read() to wait for the nand_resume() to unlock
> > > > > > > access to the mtd device.
> > > > > > >
> > > > > > > Then we avoid -EBUSY is returned to ubifsi via mtd_write()/mtd_read(),
> > > > > > > that will in turn hard error on every error returened.
> > > > > > > We have seen during ubifs tries to call mtd_write before the mtd device
> > > > > > > is resumed.
> > > > > >
> > > > > > I think the problem is here. Why would UBIFS/UBI try to write something
> > > > > > to a device that's not resumed yet (or has been suspended already, if
> > > > > > you hit this in the suspend path).
> > > > > >
> > > > > > >
> > > > > > > Exec_op[0] speed things up, so we see this race when the device is
> > > > > > > resuming. But it's actually "mtd: rawnand: Simplify the locking" that
> > > > > > > allows it to return -EBUSY, before that commit it would have waited for
> > > > > > > the mtd device to resume.
> > > > > >
> > > > > > Uh, wait. If nand_resume() was called before any writes/reads this
> > > > > > wouldn't happen. IMHO, the problem is not that we return -EBUSY without
> > > > > > blocking, the problem is that someone issues a write/read before calling
> > > > > > mtd_resume().
> > > > > >
> > > > >
> > > > > The commit msg from "mtd: rawnand: Simplify the locking" states this clearly.
> > > > >
> > > > > """
> > > > > Last important change to mention: we now return -EBUSY when someone
> > > > > tries to access a device that as been suspended, and propagate this
> > > > > error to the upper layer.
> > > > > """
> > > > >
> > > > > IMHO "mtd: rawnand: Simplify the locking" should never had been merged
> > > > > before the upper layers was fixed to handle -EBUSY. ;)
> > > > > Which they still not are...
> > > >
> > > > That's not really the problem here. Upper layers should never get
> > > > -EBUSY in the first place if the MTD device was resumed before the UBI
> > > > device. Looks like we have a missing UBI -> MTD parenting link, which
> > > > would explain why things don't get resumed in the right order. Can you
> > > > try with the following diff applied?
> > > >
> > > > ---
> > > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > > > index f399edc82191..1981ce8f3a26 100644
> > > > --- a/drivers/mtd/ubi/build.c
> > > > +++ b/drivers/mtd/ubi/build.c
> > > > @@ -905,6 +905,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int
> > > > ubi_num, ubi->dev.release = dev_release;
> > > > ubi->dev.class = &ubi_class;
> > > > ubi->dev.groups = ubi_dev_groups;
> > > > + ubi->dev.parent = &mtd->dev;
> > > >
> > > > ubi->mtd = mtd;
> > > > ubi->ubi_num = ubi_num;
> > > >
> > >
> > > No change:
> > > [ 71.739193] Filesystems sync: 34.212 seconds
> > > [ 71.755044] Freezing user space processes ... (elapsed 0.004 seconds) done.
> > > [ 71.767289] OOM killer disabled.
> > > [ 71.770552] Freezing remaining freezable tasks ... (elapsed 0.004 seconds) done.
> > > [ 71.782182] printk: Suspending console(s) (use no_console_suspend to debug)
> > > [ 71.824391] nand_suspend
> > > [ 71.825177] gpmi_pm_suspend
> > > [ 71.825676] PM: suspend devices took 0.040 seconds
> > > [ 71.825971] nand_write_oob - nand_get_device() returned -EBUSY
> > > [ 71.825985] ubi0 error: ubi_io_write: error -16 while writing 4096 bytes to PEB 986:65536, written 0 bytes
> > > [ 71.826029] CPU: 0 PID: 7 Comm: kworker/u2:0 Not tainted 5.15.0-rc3-dirty #43
> > > [ 71.826043] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> > > [ 71.826054] Workqueue: writeback wb_workfn (flush-ubifs_0_8)
> > > [ 71.826094] [<c010da84>] (unwind_backtrace) from [<c010a1b4>] (show_stack+0x10/0x14)
> > > [ 71.826122] [<c010a1b4>] (show_stack) from [<c0989c30>] (dump_stack_lvl+0x40/0x4c)
> > > [ 71.826151] [<c0989c30>] (dump_stack_lvl) from [<c05ed690>] (ubi_io_write+0x510/0x6b0)
> > > [ 71.826178] [<c05ed690>] (ubi_io_write) from [<c05ea2f0>] (ubi_eba_write_leb+0xd0/0x968)
> > > [ 71.826204] [<c05ea2f0>] (ubi_eba_write_leb) from [<c05e8754>] (ubi_leb_write+0xd0/0xe8)
> > > [ 71.826232] [<c05e8754>] (ubi_leb_write) from [<c03d67bc>] (ubifs_leb_write+0x68/0x104)
> > > [ 71.826263] [<c03d67bc>] (ubifs_leb_write) from [<c03d79e8>] (ubifs_wbuf_write_nolock+0x28c/0x74c)
> > > [ 71.826291] [<c03d79e8>] (ubifs_wbuf_write_nolock) from [<c03ca18c>] (ubifs_jnl_write_data+0x1b8/0x2b4)
> > > [ 71.826319] [<c03ca18c>] (ubifs_jnl_write_data) from [<c03cd184>] (do_writepage+0x190/0x284)
> > > [ 71.826342] [<c03cd184>] (do_writepage) from [<c023083c>] (__writepage+0x14/0x68)
> > > [ 71.826367] [<c023083c>] (__writepage) from [<c0231748>] (write_cache_pages+0x1c8/0x3f0)
> > > [ 71.826390] [<c0231748>] (write_cache_pages) from [<c0233854>] (do_writepages+0xcc/0x1f4)
> > > [ 71.826413] [<c0233854>] (do_writepages) from [<c02d03dc>] (__writeback_single_inode+0x2c/0x1b4)
> > > [ 71.826440] [<c02d03dc>] (__writeback_single_inode) from [<c02d0a64>] (writeback_sb_inodes+0x200/0x470)
> > > [ 71.826466] [<c02d0a64>] (writeback_sb_inodes) from [<c02d0d10>] (__writeback_inodes_wb+0x3c/0xf4)
> > > [ 71.826493] [<c02d0d10>] (__writeback_inodes_wb) from [<c02d0f58>] (wb_writeback+0x190/0x1f0)
> > > [ 71.826520] [<c02d0f58>] (wb_writeback) from [<c02d21d8>] (wb_workfn+0x2c0/0x3d4)
> > > [ 71.826545] [<c02d21d8>] (wb_workfn) from [<c013ac04>] (process_one_work+0x1e0/0x440)
> > > [ 71.826574] [<c013ac04>] (process_one_work) from [<c013aeac>] (worker_thread+0x48/0x594)
> > > [ 71.826600] [<c013aeac>] (worker_thread) from [<c0142364>] (kthread+0x134/0x15c)
> > > [ 71.826625] [<c0142364>] (kthread) from [<c0100150>] (ret_from_fork+0x14/0x24)
> >
> > I'm not entirely sure, but given the timing, it looks like this
> > actually happens in the suspend path, not it the resume path. What I
> > don't get is why we still have a kernel thread running at that point.
>
> Have you seen the reproducer script?
How would I know about this script or your previous attempt (mentioned
at the end of this email) given I was not Cc-ed on the previous
discussion, and nothing mentions it in this RFC...
> ---
> root@iwg26-v1:/data/root# cat /data/crash.sh
> #!/bin/sh -x
>
> echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
>
> rm /data/test50M
> dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
> cp /tmp/test50M /data/ &
> sleep 1
> echo mem > /sys/power/state
> ---
>
> As seen in the log above disk is synced before suspend.
> cp is continuing to copy data to ubifs.
> And then user space processes are frozen.
> At this point the kernel thread would have unwritten data.
>
> We tried to solve this with:
> https://lkml.org/lkml/2021/9/1/280
I see. It's still unclear to me when the write happens. Is it in the
suspend path (before the system is actually suspended), or in the
resume path (when the system is being resumed).
Anyway, let's admit writing to a storage device while it's suspended is
a valid use case and requires the storage layer to put this request on
old. This wait should not, IMHO, be handled at the NAND level, but at
the MTD level (using a waitqueue, and an atomic to make
suspended/resumed transitions safe). And abusing a mutex to implement
that is certainly not a good idea.
On Tue, Oct 05, 2021 at 10:23:00AM +0200, Boris Brezillon wrote:
> On Tue, 5 Oct 2021 09:09:30 +0200
> Sean Nyekjaer <[email protected]> wrote:
[ ... ]
> >
> > Have you seen the reproducer script?
>
> How would I know about this script or your previous attempt (mentioned
> at the end of this email) given I was not Cc-ed on the previous
> discussion, and nothing mentions it in this RFC...
>
That's why I shared it here ;)
Initially I thought this was a bug introduced by exec_op.
> > ---
> > root@iwg26-v1:/data/root# cat /data/crash.sh
> > #!/bin/sh -x
> >
> > echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
> >
> > rm /data/test50M
> > dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
> > cp /tmp/test50M /data/ &
> > sleep 1
> > echo mem > /sys/power/state
> > ---
> >
> > As seen in the log above disk is synced before suspend.
> > cp is continuing to copy data to ubifs.
> > And then user space processes are frozen.
> > At this point the kernel thread would have unwritten data.
> >
> > We tried to solve this with:
> > https://lkml.org/lkml/2021/9/1/280
>
> I see. It's still unclear to me when the write happens. Is it in the
> suspend path (before the system is actually suspended), or in the
> resume path (when the system is being resumed).
>
> Anyway, let's admit writing to a storage device while it's suspended is
> a valid use case and requires the storage layer to put this request on
> old. This wait should not, IMHO, be handled at the NAND level, but at
> the MTD level (using a waitqueue, and an atomic to make
> suspended/resumed transitions safe). And abusing a mutex to implement
> that is certainly not a good idea.
I did't say this was the right solution ;) I actually asked in the RFC:
"Should we introduce a new mutex? Or maybe a spin_lock?"
What are you proposing, a waitqueue in mtd_info? That gets checked in
mtd_write()/mtd_read()?
/Sean
On Tue, 5 Oct 2021 10:49:38 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Tue, Oct 05, 2021 at 10:23:00AM +0200, Boris Brezillon wrote:
> > On Tue, 5 Oct 2021 09:09:30 +0200
> > Sean Nyekjaer <[email protected]> wrote:
>
> [ ... ]
>
> > >
> > > Have you seen the reproducer script?
> >
> > How would I know about this script or your previous attempt (mentioned
> > at the end of this email) given I was not Cc-ed on the previous
> > discussion, and nothing mentions it in this RFC...
> >
>
> That's why I shared it here ;)
> Initially I thought this was a bug introduced by exec_op.
>
> > > ---
> > > root@iwg26-v1:/data/root# cat /data/crash.sh
> > > #!/bin/sh -x
> > >
> > > echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
> > >
> > > rm /data/test50M
> > > dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
> > > cp /tmp/test50M /data/ &
> > > sleep 1
> > > echo mem > /sys/power/state
> > > ---
> > >
> > > As seen in the log above disk is synced before suspend.
> > > cp is continuing to copy data to ubifs.
> > > And then user space processes are frozen.
> > > At this point the kernel thread would have unwritten data.
> > >
> > > We tried to solve this with:
> > > https://lkml.org/lkml/2021/9/1/280
> >
> > I see. It's still unclear to me when the write happens. Is it in the
> > suspend path (before the system is actually suspended), or in the
> > resume path (when the system is being resumed).
> >
> > Anyway, let's admit writing to a storage device while it's suspended is
> > a valid use case and requires the storage layer to put this request on
> > old. This wait should not, IMHO, be handled at the NAND level, but at
> > the MTD level (using a waitqueue, and an atomic to make
> > suspended/resumed transitions safe). And abusing a mutex to implement
> > that is certainly not a good idea.
>
> I did't say this was the right solution ;) I actually asked in the RFC:
> "Should we introduce a new mutex? Or maybe a spin_lock?"
>
> What are you proposing, a waitqueue in mtd_info? That gets checked in
> mtd_write()/mtd_read()?
Yes, and replacing the suspended state by an atomic, and providing a
helper to wait on the device readiness. Helper you will call in every
path involving a communication with the HW, not just mtd_read/write()
(you're missing erase at least, and I fear there are other hooks that
might lead to commands being issued to the device). But before we get
there, I think it's important to understand what the kernel expects.
IOW, if and when threads can do a request on a suspended device, and
when it's acceptable to wait (vs returning -EBUSY), otherwise I fear
we'll end up with deadlocks in the suspend/resume path.
On Tue, Oct 05, 2021 at 10:58:36AM +0200, Boris Brezillon wrote:
> On Tue, 5 Oct 2021 10:49:38 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > On Tue, Oct 05, 2021 at 10:23:00AM +0200, Boris Brezillon wrote:
> > > On Tue, 5 Oct 2021 09:09:30 +0200
> > > Sean Nyekjaer <[email protected]> wrote:
> >
> > [ ... ]
> >
> > > >
> > > > Have you seen the reproducer script?
> > >
> > > How would I know about this script or your previous attempt (mentioned
> > > at the end of this email) given I was not Cc-ed on the previous
> > > discussion, and nothing mentions it in this RFC...
> > >
> >
> > That's why I shared it here ;)
> > Initially I thought this was a bug introduced by exec_op.
> >
> > > > ---
> > > > root@iwg26-v1:/data/root# cat /data/crash.sh
> > > > #!/bin/sh -x
> > > >
> > > > echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
> > > >
> > > > rm /data/test50M
> > > > dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
> > > > cp /tmp/test50M /data/ &
> > > > sleep 1
> > > > echo mem > /sys/power/state
> > > > ---
> > > >
> > > > As seen in the log above disk is synced before suspend.
> > > > cp is continuing to copy data to ubifs.
> > > > And then user space processes are frozen.
> > > > At this point the kernel thread would have unwritten data.
> > > >
> > > > We tried to solve this with:
> > > > https://lkml.org/lkml/2021/9/1/280
> > >
> > > I see. It's still unclear to me when the write happens. Is it in the
> > > suspend path (before the system is actually suspended), or in the
> > > resume path (when the system is being resumed).
> > >
> > > Anyway, let's admit writing to a storage device while it's suspended is
> > > a valid use case and requires the storage layer to put this request on
> > > old. This wait should not, IMHO, be handled at the NAND level, but at
> > > the MTD level (using a waitqueue, and an atomic to make
> > > suspended/resumed transitions safe). And abusing a mutex to implement
> > > that is certainly not a good idea.
> >
> > I did't say this was the right solution ;) I actually asked in the RFC:
> > "Should we introduce a new mutex? Or maybe a spin_lock?"
> >
> > What are you proposing, a waitqueue in mtd_info? That gets checked in
> > mtd_write()/mtd_read()?
>
> Yes, and replacing the suspended state by an atomic, and providing a
> helper to wait on the device readiness. Helper you will call in every
> path involving a communication with the HW, not just mtd_read/write()
> (you're missing erase at least, and I fear there are other hooks that
> might lead to commands being issued to the device). But before we get
> there, I think it's important to understand what the kernel expects.
> IOW, if and when threads can do a request on a suspended device, and
> when it's acceptable to wait (vs returning -EBUSY), otherwise I fear
> we'll end up with deadlocks in the suspend/resume path.
I have a proposal [0] and yes I have ended up in many deadlocks during
testing. The hardest part is the locking when going into suspend.
I'm not sure the wait_queue is initialized the right place :)
And I'm kinda abusing the nand_get_device() for this...
Who do you think we should add to the discussion?
/Sean
[0]:
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..735dfff18143 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -337,11 +337,10 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
*/
static int nand_get_device(struct nand_chip *chip)
{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+
+ wait_event(mtd->wait_queue, atomic_read(&chip->suspended) == 0);
mutex_lock(&chip->lock);
- if (chip->suspended) {
- mutex_unlock(&chip->lock);
- return -EBUSY;
- }
mutex_lock(&chip->controller->lock);
return 0;
@@ -4562,11 +4561,15 @@ static int nand_suspend(struct mtd_info *mtd)
struct nand_chip *chip = mtd_to_nand(mtd);
int ret = 0;
+ atomic_inc(&chip->suspended);
mutex_lock(&chip->lock);
if (chip->ops.suspend)
ret = chip->ops.suspend(chip);
- if (!ret)
- chip->suspended = 1;
+ if (ret) {
+ /* Wake things up again if suspend fails */
+ atomic_dec(&chip->suspended);
+ wake_up(&mtd->wait_queue);
+ }
mutex_unlock(&chip->lock);
return ret;
@@ -4581,10 +4584,12 @@ static void nand_resume(struct mtd_info *mtd)
struct nand_chip *chip = mtd_to_nand(mtd);
mutex_lock(&chip->lock);
- if (chip->suspended) {
+ if (atomic_read(&chip->suspended)) {
if (chip->ops.resume)
chip->ops.resume(chip);
- chip->suspended = 0;
+
+ atomic_dec(&chip->suspended);
+ wake_up(&mtd->wait_queue);
} else {
pr_err("%s called for a chip which is not in suspended state\n",
__func__);
@@ -5099,6 +5104,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
(int)(targetsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
+
+ init_waitqueue_head(&mtd->wait_queue);
+
return 0;
free_detect_allocation:
@@ -6264,6 +6272,8 @@ static int nand_scan_tail(struct nand_chip *chip)
if (chip->options & NAND_SKIP_BBTSCAN)
return 0;
+ atomic_set(&chip->suspended, 0);
+
/* Build bad block table */
ret = nand_create_bbt(chip);
if (ret)
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 88227044fc86..f7dcbc336170 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -360,6 +360,8 @@ struct mtd_info {
int (*_get_device) (struct mtd_info *mtd);
void (*_put_device) (struct mtd_info *mtd);
+ wait_queue_head_t wait_queue;
+
/*
* flag indicates a panic write, low level drivers can take appropriate
* action if required to ensure writes go through
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index b2f9dd3cbd69..c25c0749f8d0 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1293,7 +1293,7 @@ struct nand_chip {
/* Internals */
struct mutex lock;
- unsigned int suspended : 1;
+ atomic_t suspended;
int cur_cs;
int read_retries;
struct nand_secure_region *secure_regions;
On Thu, 7 Oct 2021 13:43:51 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Tue, Oct 05, 2021 at 10:58:36AM +0200, Boris Brezillon wrote:
> > On Tue, 5 Oct 2021 10:49:38 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > On Tue, Oct 05, 2021 at 10:23:00AM +0200, Boris Brezillon wrote:
> > > > On Tue, 5 Oct 2021 09:09:30 +0200
> > > > Sean Nyekjaer <[email protected]> wrote:
> > >
> > > [ ... ]
> > >
> > > > >
> > > > > Have you seen the reproducer script?
> > > >
> > > > How would I know about this script or your previous attempt (mentioned
> > > > at the end of this email) given I was not Cc-ed on the previous
> > > > discussion, and nothing mentions it in this RFC...
> > > >
> > >
> > > That's why I shared it here ;)
> > > Initially I thought this was a bug introduced by exec_op.
> > >
> > > > > ---
> > > > > root@iwg26-v1:/data/root# cat /data/crash.sh
> > > > > #!/bin/sh -x
> > > > >
> > > > > echo enabled > /sys/devices/platform/soc/2100000.bus/21f4000.serial/tty/ttymxc4/power/wakeup
> > > > >
> > > > > rm /data/test50M
> > > > > dd if=/dev/urandom of=/tmp/test50M bs=1M count=50
> > > > > cp /tmp/test50M /data/ &
> > > > > sleep 1
> > > > > echo mem > /sys/power/state
> > > > > ---
> > > > >
> > > > > As seen in the log above disk is synced before suspend.
> > > > > cp is continuing to copy data to ubifs.
> > > > > And then user space processes are frozen.
> > > > > At this point the kernel thread would have unwritten data.
> > > > >
> > > > > We tried to solve this with:
> > > > > https://lkml.org/lkml/2021/9/1/280
> > > >
> > > > I see. It's still unclear to me when the write happens. Is it in the
> > > > suspend path (before the system is actually suspended), or in the
> > > > resume path (when the system is being resumed).
> > > >
> > > > Anyway, let's admit writing to a storage device while it's suspended is
> > > > a valid use case and requires the storage layer to put this request on
> > > > old. This wait should not, IMHO, be handled at the NAND level, but at
> > > > the MTD level (using a waitqueue, and an atomic to make
> > > > suspended/resumed transitions safe). And abusing a mutex to implement
> > > > that is certainly not a good idea.
> > >
> > > I did't say this was the right solution ;) I actually asked in the RFC:
> > > "Should we introduce a new mutex? Or maybe a spin_lock?"
> > >
> > > What are you proposing, a waitqueue in mtd_info? That gets checked in
> > > mtd_write()/mtd_read()?
> >
> > Yes, and replacing the suspended state by an atomic, and providing a
> > helper to wait on the device readiness. Helper you will call in every
> > path involving a communication with the HW, not just mtd_read/write()
> > (you're missing erase at least, and I fear there are other hooks that
> > might lead to commands being issued to the device). But before we get
> > there, I think it's important to understand what the kernel expects.
> > IOW, if and when threads can do a request on a suspended device, and
> > when it's acceptable to wait (vs returning -EBUSY), otherwise I fear
> > we'll end up with deadlocks in the suspend/resume path.
>
> I have a proposal [0] and yes I have ended up in many deadlocks during
> testing. The hardest part is the locking when going into suspend.
> I'm not sure the wait_queue is initialized the right place :)
> And I'm kinda abusing the nand_get_device() for this...
>
> Who do you think we should add to the discussion?
>
> /Sean
>
> [0]:
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3d6c6e880520..735dfff18143 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
As I said previously, I think this should be handled MTD level
(drivers/mtd/mtdcore.c) not in the raw NAND framework.
> @@ -337,11 +337,10 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
> */
> static int nand_get_device(struct nand_chip *chip)
> {
> + struct mtd_info *mtd = nand_to_mtd(chip);
> +
> + wait_event(mtd->wait_queue, atomic_read(&chip->suspended) == 0);
> mutex_lock(&chip->lock);
> - if (chip->suspended) {
> - mutex_unlock(&chip->lock);
> - return -EBUSY;
> - }
There's a race here: the device might enter suspend again before you're
able to acquire the lock.
> mutex_lock(&chip->controller->lock);
>
> return 0;
> @@ -4562,11 +4561,15 @@ static int nand_suspend(struct mtd_info *mtd)
> struct nand_chip *chip = mtd_to_nand(mtd);
> int ret = 0;
>
> + atomic_inc(&chip->suspended);
> mutex_lock(&chip->lock);
And it's racy here as well: you mark the device as suspended before you
even acquired the lock.
> if (chip->ops.suspend)
> ret = chip->ops.suspend(chip);
> - if (!ret)
> - chip->suspended = 1;
> + if (ret) {
> + /* Wake things up again if suspend fails */
> + atomic_dec(&chip->suspended);
> + wake_up(&mtd->wait_queue);
> + }
> mutex_unlock(&chip->lock);
>
> return ret;
> @@ -4581,10 +4584,12 @@ static void nand_resume(struct mtd_info *mtd)
> struct nand_chip *chip = mtd_to_nand(mtd);
>
> mutex_lock(&chip->lock);
> - if (chip->suspended) {
> + if (atomic_read(&chip->suspended)) {
> if (chip->ops.resume)
> chip->ops.resume(chip);
> - chip->suspended = 0;
> +
> + atomic_dec(&chip->suspended);
> + wake_up(&mtd->wait_queue);
> } else {
> pr_err("%s called for a chip which is not in suspended state\n",
> __func__);
> @@ -5099,6 +5104,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
> (int)(targetsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
> +
> + init_waitqueue_head(&mtd->wait_queue);
> +
It's an MTD field. It should be initialized somewhere in mtdcore.c.
> return 0;
>
> free_detect_allocation:
> @@ -6264,6 +6272,8 @@ static int nand_scan_tail(struct nand_chip *chip)
> if (chip->options & NAND_SKIP_BBTSCAN)
> return 0;
>
> + atomic_set(&chip->suspended, 0);
> +
> /* Build bad block table */
> ret = nand_create_bbt(chip);
> if (ret)
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 88227044fc86..f7dcbc336170 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -360,6 +360,8 @@ struct mtd_info {
> int (*_get_device) (struct mtd_info *mtd);
> void (*_put_device) (struct mtd_info *mtd);
>
> + wait_queue_head_t wait_queue;
> +
wait_queue doesn't really describe what this waitqueue is used for
(maybe resume_wq), and the suspended state should be here as well
(actually, there's one already).
Actually, what we need is a way to prevent the device from being
suspended while accesses are still in progress, and new accesses from
being queued if a suspend is pending. So, I think you need a readwrite
lock here:
* take the lock in read mode for all IO accesses, check the
mtd->suspended value
- if true, release the lock, and wait (retry on wakeup)
- if false, just do the IO
* take the lock in write mode when you want to suspend/resume the
device and update the suspended field. Call wake_up_all() in the
resume path
On Thu, Oct 07, 2021 at 02:18:58PM +0200, Boris Brezillon wrote:
> On Thu, 7 Oct 2021 13:43:51 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
[ ... ]
> >
> > I have a proposal [0] and yes I have ended up in many deadlocks during
> > testing. The hardest part is the locking when going into suspend.
> > I'm not sure the wait_queue is initialized the right place :)
> > And I'm kinda abusing the nand_get_device() for this...
> >
> > Who do you think we should add to the discussion?
> >
> > /Sean
> >
> > [0]:
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 3d6c6e880520..735dfff18143 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
>
> As I said previously, I think this should be handled MTD level
> (drivers/mtd/mtdcore.c) not in the raw NAND framework.
>
> > @@ -337,11 +337,10 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs)
> > */
> > static int nand_get_device(struct nand_chip *chip)
> > {
> > + struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > + wait_event(mtd->wait_queue, atomic_read(&chip->suspended) == 0);
> > mutex_lock(&chip->lock);
> > - if (chip->suspended) {
> > - mutex_unlock(&chip->lock);
> > - return -EBUSY;
> > - }
>
> There's a race here: the device might enter suspend again before you're
> able to acquire the lock.
>
Thought so :)
> > mutex_lock(&chip->controller->lock);
> >
> > return 0;
> > @@ -4562,11 +4561,15 @@ static int nand_suspend(struct mtd_info *mtd)
> > struct nand_chip *chip = mtd_to_nand(mtd);
> > int ret = 0;
> >
> > + atomic_inc(&chip->suspended);
> > mutex_lock(&chip->lock);
>
> And it's racy here as well: you mark the device as suspended before you
> even acquired the lock.
>
> > if (chip->ops.suspend)
> > ret = chip->ops.suspend(chip);
> > - if (!ret)
> > - chip->suspended = 1;
> > + if (ret) {
> > + /* Wake things up again if suspend fails */
> > + atomic_dec(&chip->suspended);
> > + wake_up(&mtd->wait_queue);
> > + }
> > mutex_unlock(&chip->lock);
> >
> > return ret;
> > @@ -4581,10 +4584,12 @@ static void nand_resume(struct mtd_info *mtd)
> > struct nand_chip *chip = mtd_to_nand(mtd);
> >
> > mutex_lock(&chip->lock);
> > - if (chip->suspended) {
> > + if (atomic_read(&chip->suspended)) {
> > if (chip->ops.resume)
> > chip->ops.resume(chip);
> > - chip->suspended = 0;
> > +
> > + atomic_dec(&chip->suspended);
> > + wake_up(&mtd->wait_queue);
> > } else {
> > pr_err("%s called for a chip which is not in suspended state\n",
> > __func__);
> > @@ -5099,6 +5104,9 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type)
> > pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size: %d\n",
> > (int)(targetsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC",
> > mtd->erasesize >> 10, mtd->writesize, mtd->oobsize);
> > +
> > + init_waitqueue_head(&mtd->wait_queue);
> > +
>
> It's an MTD field. It should be initialized somewhere in mtdcore.c.
>
> > return 0;
> >
> > free_detect_allocation:
> > @@ -6264,6 +6272,8 @@ static int nand_scan_tail(struct nand_chip *chip)
> > if (chip->options & NAND_SKIP_BBTSCAN)
> > return 0;
> >
> > + atomic_set(&chip->suspended, 0);
> > +
> > /* Build bad block table */
> > ret = nand_create_bbt(chip);
> > if (ret)
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 88227044fc86..f7dcbc336170 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -360,6 +360,8 @@ struct mtd_info {
> > int (*_get_device) (struct mtd_info *mtd);
> > void (*_put_device) (struct mtd_info *mtd);
> >
> > + wait_queue_head_t wait_queue;
> > +
>
> wait_queue doesn't really describe what this waitqueue is used for
> (maybe resume_wq), and the suspended state should be here as well
> (actually, there's one already).
I'll rename to something meaningful.
>
> Actually, what we need is a way to prevent the device from being
> suspended while accesses are still in progress, and new accesses from
> being queued if a suspend is pending. So, I think you need a readwrite
> lock here:
>
> * take the lock in read mode for all IO accesses, check the
> mtd->suspended value
> - if true, release the lock, and wait (retry on wakeup)
> - if false, just do the IO
>
> * take the lock in write mode when you want to suspend/resume the
> device and update the suspended field. Call wake_up_all() in the
> resume path
Could we use the chip->lock mutex for this? It's does kinda what you
described above?
If we introduce a new lock, do we really need to have the suspended as
an atomic?
I will test with some wait and retry added to nand_get_device().
On Thu, 7 Oct 2021 14:39:16 +0200
Sean Nyekjaer <[email protected]> wrote:
> > > return 0;
> > >
> > > free_detect_allocation:
> > > @@ -6264,6 +6272,8 @@ static int nand_scan_tail(struct nand_chip *chip)
> > > if (chip->options & NAND_SKIP_BBTSCAN)
> > > return 0;
> > >
> > > + atomic_set(&chip->suspended, 0);
> > > +
> > > /* Build bad block table */
> > > ret = nand_create_bbt(chip);
> > > if (ret)
> > > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > > index 88227044fc86..f7dcbc336170 100644
> > > --- a/include/linux/mtd/mtd.h
> > > +++ b/include/linux/mtd/mtd.h
> > > @@ -360,6 +360,8 @@ struct mtd_info {
> > > int (*_get_device) (struct mtd_info *mtd);
> > > void (*_put_device) (struct mtd_info *mtd);
> > >
> > > + wait_queue_head_t wait_queue;
> > > +
> >
> > wait_queue doesn't really describe what this waitqueue is used for
> > (maybe resume_wq), and the suspended state should be here as well
> > (actually, there's one already).
>
> I'll rename to something meaningful.
> >
> > Actually, what we need is a way to prevent the device from being
> > suspended while accesses are still in progress, and new accesses from
> > being queued if a suspend is pending. So, I think you need a readwrite
> > lock here:
> >
> > * take the lock in read mode for all IO accesses, check the
> > mtd->suspended value
> > - if true, release the lock, and wait (retry on wakeup)
> > - if false, just do the IO
> >
> > * take the lock in write mode when you want to suspend/resume the
> > device and update the suspended field. Call wake_up_all() in the
> > resume path
>
> Could we use the chip->lock mutex for this? It's does kinda what you
> described above?
No you can't. Remember I suggested to move all of that logic to
mtdcore.c, which doesn't know about the nand_chip struct.
> If we introduce a new lock, do we really need to have the suspended as
> an atomic?
Nope, I thought we could do without a lock, but we actually need to
track active IO requests, not just the suspended state.
>
> I will test with some wait and retry added to nand_get_device().
Again, I think there's a misunderstanding here: if you move it to the
mtd layer, it can't be done in nand_get_device(). But once you've
implemented it in mtdcore.c, you should be able to get rid of the
nand_chip->suspended field.
On Thu, Oct 07, 2021 at 03:14:26PM +0200, Boris Brezillon wrote:
> On Thu, 7 Oct 2021 14:39:16 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > >
> > > wait_queue doesn't really describe what this waitqueue is used for
> > > (maybe resume_wq), and the suspended state should be here as well
> > > (actually, there's one already).
> >
> > I'll rename to something meaningful.
> > >
> > > Actually, what we need is a way to prevent the device from being
> > > suspended while accesses are still in progress, and new accesses from
> > > being queued if a suspend is pending. So, I think you need a readwrite
> > > lock here:
> > >
> > > * take the lock in read mode for all IO accesses, check the
> > > mtd->suspended value
> > > - if true, release the lock, and wait (retry on wakeup)
> > > - if false, just do the IO
> > >
> > > * take the lock in write mode when you want to suspend/resume the
> > > device and update the suspended field. Call wake_up_all() in the
> > > resume path
> >
> > Could we use the chip->lock mutex for this? It's does kinda what you
> > described above?
>
> No you can't. Remember I suggested to move all of that logic to
> mtdcore.c, which doesn't know about the nand_chip struct.
>
> > If we introduce a new lock, do we really need to have the suspended as
> > an atomic?
>
> Nope, I thought we could do without a lock, but we actually need to
> track active IO requests, not just the suspended state.
I have only added wait_queue to read and write operations.
I'll have a look into where we should add further checks.
>
> >
> > I will test with some wait and retry added to nand_get_device().
>
> Again, I think there's a misunderstanding here: if you move it to the
> mtd layer, it can't be done in nand_get_device(). But once you've
> implemented it in mtdcore.c, you should be able to get rid of the
> nand_chip->suspended field.
I have moved the suspended atomic and wake_queue to mtdcore.c. And kept
the suspended variable in nand_base as is fine for chip level suspend
status.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c8fd7f758938..6492071eb4da 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -42,15 +42,24 @@ static int mtd_cls_suspend(struct device *dev)
{
struct mtd_info *mtd = dev_get_drvdata(dev);
- return mtd ? mtd_suspend(mtd) : 0;
+ if (mtd) {
+ atomic_inc(&mtd->suspended);
+ return mtd_suspend(mtd);
+ }
+ + return 0;
}
static int mtd_cls_resume(struct device *dev)
{
struct mtd_info *mtd = dev_get_drvdata(dev);
- if (mtd)
+ if (mtd) {
mtd_resume(mtd);
+ atomic_dec(&mtd->suspended);
+ wake_up_all(&mtd->resume_wq);
+ }
+
return 0;
}
@@ -678,6 +687,10 @@ int add_mtd_device(struct mtd_info *mtd)
if (error)
goto fail_nvmem_add;
+ init_waitqueue_head(&mtd->resume_wq);
+
+ atomic_set(&mtd->suspended, 0);
+
mtd_debugfs_populate(mtd);
device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
@@ -1558,6 +1571,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
struct mtd_ecc_stats old_stats = master->ecc_stats;
int ret_code;
+ wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
+
ops->retlen = ops->oobretlen = 0;
ret_code = mtd_check_oob_ops(mtd, from, ops);
@@ -1597,6 +1612,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
struct mtd_info *master = mtd_get_master(mtd);
int ret;
+ wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
+
ops->retlen = ops->oobretlen = 0;
if (!(mtd->flags & MTD_WRITEABLE))
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 88227044fc86..70ede36092a9 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -360,6 +360,9 @@ struct mtd_info {
int (*_get_device) (struct mtd_info *mtd);
void (*_put_device) (struct mtd_info *mtd);
+ atomic_t suspended;
+ wait_queue_head_t resume_wq;
+
/*
* flag indicates a panic write, low level drivers can take appropriate
* action if required to ensure writes go through
Hi Sean,
On Fri, 8 Oct 2021 12:04:25 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Thu, Oct 07, 2021 at 03:14:26PM +0200, Boris Brezillon wrote:
> > On Thu, 7 Oct 2021 14:39:16 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > >
> > > > wait_queue doesn't really describe what this waitqueue is used for
> > > > (maybe resume_wq), and the suspended state should be here as well
> > > > (actually, there's one already).
> > >
> > > I'll rename to something meaningful.
> > > >
> > > > Actually, what we need is a way to prevent the device from being
> > > > suspended while accesses are still in progress, and new accesses from
> > > > being queued if a suspend is pending. So, I think you need a readwrite
> > > > lock here:
> > > >
> > > > * take the lock in read mode for all IO accesses, check the
> > > > mtd->suspended value
> > > > - if true, release the lock, and wait (retry on wakeup)
> > > > - if false, just do the IO
> > > >
> > > > * take the lock in write mode when you want to suspend/resume the
> > > > device and update the suspended field. Call wake_up_all() in the
> > > > resume path
> > >
> > > Could we use the chip->lock mutex for this? It's does kinda what you
> > > described above?
> >
> > No you can't. Remember I suggested to move all of that logic to
> > mtdcore.c, which doesn't know about the nand_chip struct.
> >
> > > If we introduce a new lock, do we really need to have the suspended as
> > > an atomic?
> >
> > Nope, I thought we could do without a lock, but we actually need to
> > track active IO requests, not just the suspended state.
>
> I have only added wait_queue to read and write operations.
It's still racy (see below).
> I'll have a look into where we should add further checks.
>
> >
> > >
> > > I will test with some wait and retry added to nand_get_device().
> >
> > Again, I think there's a misunderstanding here: if you move it to the
> > mtd layer, it can't be done in nand_get_device(). But once you've
> > implemented it in mtdcore.c, you should be able to get rid of the
> > nand_chip->suspended field.
>
> I have moved the suspended atomic and wake_queue to mtdcore.c.
That doesn't work (see below).
> And kept
> the suspended variable in nand_base as is fine for chip level suspend
> status.
Why? If you handle that at the MTD level you shouldn't need it at the
NAND level? BTW, would you please care to detail your reasoning when
you say you did or didn't do something. It's a bit hard to guess what
led you to this conclusion...
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c8fd7f758938..6492071eb4da 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -42,15 +42,24 @@ static int mtd_cls_suspend(struct device *dev)
> {
> struct mtd_info *mtd = dev_get_drvdata(dev);
>
> - return mtd ? mtd_suspend(mtd) : 0;
> + if (mtd) {
> + atomic_inc(&mtd->suspended);
> + return mtd_suspend(mtd);
> + }
> + + return 0;
> }
>
> static int mtd_cls_resume(struct device *dev)
> {
> struct mtd_info *mtd = dev_get_drvdata(dev);
>
> - if (mtd)
> + if (mtd) {
> mtd_resume(mtd);
> + atomic_dec(&mtd->suspended);
> + wake_up_all(&mtd->resume_wq);
> + }
> +
> return 0;
> }
> @@ -678,6 +687,10 @@ int add_mtd_device(struct mtd_info *mtd)
> if (error)
> goto fail_nvmem_add;
>
> + init_waitqueue_head(&mtd->resume_wq);
> +
> + atomic_set(&mtd->suspended, 0);
> +
> mtd_debugfs_populate(mtd);
>
> device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> @@ -1558,6 +1571,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
> struct mtd_ecc_stats old_stats = master->ecc_stats;
> int ret_code;
>
> + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
That's racy:
thread A thread B
|
enters mtd_read() |
passes the !suspended test |
| enters mtd_suspend()
| sets suspended to 1
|
starts the IO |
| suspends the device
tries to finish the IO |
on a suspended device |
BOOM!
Using an atomic doesn't solve any of that, you really need to make sure
nothing tries to communicate with the device while you're suspending
it, hence the suggestion to use a rw_semaphore to protect against that.
> +
> ops->retlen = ops->oobretlen = 0;
>
> ret_code = mtd_check_oob_ops(mtd, from, ops);
> @@ -1597,6 +1612,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> struct mtd_info *master = mtd_get_master(mtd);
> int ret;
>
> + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
> +
Please don't open-code this in every IO path, add helpers hiding all the
complexity.
To sum-up, that's more or less what I add in mind:
static void mtd_start_access(struct mtd_info *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 (!mtd->_suspend)
return;
/*
* Wait until the device is resumed. Should we have a
* non-blocking mode here?
*/
while (1) {
down_read(&mtd->suspend_lock);
if (!mtd->suspended)
return;
up_read(&mtd->suspend_lock);
wait_event(mtd->resume_wq, mtd->suspended == false);
}
}
static void mtd_end_access(struct mtd_info *mtd)
{
if (!mtd->_suspend)
return;
up_read(&mtd->suspend_lock);
}
static void mtd_suspend(struct mtd_info *mtd)
{
int ret;
if (!mtd->_suspend)
return;
down_write(&mtd->suspend_lock);
if (mtd->suspended == false) {
ret = mtd->_suspend(mtd);
if (!ret)
mtd->suspended = true;
}
up_write(&mtd->suspend_lock);
}
static void mtd_resume(struct mtd_info *mtd)
{
if (!mtd->_suspend)
return;
down_write(&mtd->suspend_lock);
if (mtd->suspended) {
if (mtd->_resume)
mtd->_resume(mtd);
mtd->suspended = false;
/* The MTD dev has been resumed, wake up all waiters. */
wake_up_all(&mtd->resume_wq)
}
up_write(&mtd->suspend_lock);
}
You then need to call mtd_{start,end}_access() in all MTD IO path
(read/write/erase and maybe others too).
Regards,
Boris
On Fri, Oct 08, 2021 at 01:20:38PM +0200, Boris Brezillon wrote:
> Hi Sean,
>
> On Fri, 8 Oct 2021 12:04:25 +0200
> Sean Nyekjaer <[email protected]> wrote:
>
> > On Thu, Oct 07, 2021 at 03:14:26PM +0200, Boris Brezillon wrote:
> > > On Thu, 7 Oct 2021 14:39:16 +0200
> > > Sean Nyekjaer <[email protected]> wrote:
> > >
> > > > >
> > > > > wait_queue doesn't really describe what this waitqueue is used for
> > > > > (maybe resume_wq), and the suspended state should be here as well
> > > > > (actually, there's one already).
> > > >
> > > > I'll rename to something meaningful.
> > > > >
> > > > > Actually, what we need is a way to prevent the device from being
> > > > > suspended while accesses are still in progress, and new accesses from
> > > > > being queued if a suspend is pending. So, I think you need a readwrite
> > > > > lock here:
> > > > >
> > > > > * take the lock in read mode for all IO accesses, check the
> > > > > mtd->suspended value
> > > > > - if true, release the lock, and wait (retry on wakeup)
> > > > > - if false, just do the IO
> > > > >
> > > > > * take the lock in write mode when you want to suspend/resume the
> > > > > device and update the suspended field. Call wake_up_all() in the
> > > > > resume path
> > > >
> > > > Could we use the chip->lock mutex for this? It's does kinda what you
> > > > described above?
> > >
> > > No you can't. Remember I suggested to move all of that logic to
> > > mtdcore.c, which doesn't know about the nand_chip struct.
> > >
> > > > If we introduce a new lock, do we really need to have the suspended as
> > > > an atomic?
> > >
> > > Nope, I thought we could do without a lock, but we actually need to
> > > track active IO requests, not just the suspended state.
> >
> > I have only added wait_queue to read and write operations.
>
> It's still racy (see below).
>
> > I'll have a look into where we should add further checks.
> >
> > >
> > > >
> > > > I will test with some wait and retry added to nand_get_device().
> > >
> > > Again, I think there's a misunderstanding here: if you move it to the
> > > mtd layer, it can't be done in nand_get_device(). But once you've
> > > implemented it in mtdcore.c, you should be able to get rid of the
> > > nand_chip->suspended field.
> >
> > I have moved the suspended atomic and wake_queue to mtdcore.c.
>
> That doesn't work (see below).
>
> > And kept
> > the suspended variable in nand_base as is fine for chip level suspend
> > status.
>
> Why? If you handle that at the MTD level you shouldn't need it at the
> NAND level? BTW, would you please care to detail your reasoning when
> you say you did or didn't do something. It's a bit hard to guess what
> led you to this conclusion...
>
> >
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index c8fd7f758938..6492071eb4da 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -42,15 +42,24 @@ static int mtd_cls_suspend(struct device *dev)
> > {
> > struct mtd_info *mtd = dev_get_drvdata(dev);
> >
> > - return mtd ? mtd_suspend(mtd) : 0;
> > + if (mtd) {
> > + atomic_inc(&mtd->suspended);
> > + return mtd_suspend(mtd);
> > + }
> > + + return 0;
> > }
> >
> > static int mtd_cls_resume(struct device *dev)
> > {
> > struct mtd_info *mtd = dev_get_drvdata(dev);
> >
> > - if (mtd)
> > + if (mtd) {
> > mtd_resume(mtd);
> > + atomic_dec(&mtd->suspended);
> > + wake_up_all(&mtd->resume_wq);
> > + }
> > +
> > return 0;
> > }
> > @@ -678,6 +687,10 @@ int add_mtd_device(struct mtd_info *mtd)
> > if (error)
> > goto fail_nvmem_add;
> >
> > + init_waitqueue_head(&mtd->resume_wq);
> > +
> > + atomic_set(&mtd->suspended, 0);
> > +
> > mtd_debugfs_populate(mtd);
> >
> > device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> > @@ -1558,6 +1571,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
> > struct mtd_ecc_stats old_stats = master->ecc_stats;
> > int ret_code;
> >
> > + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
>
> That's racy:
>
> thread A thread B
> |
> enters mtd_read() |
> passes the !suspended test |
> | enters mtd_suspend()
> | sets suspended to 1
> |
> starts the IO |
> | suspends the device
> tries to finish the IO |
> on a suspended device |
>
> BOOM!
>
>
> Using an atomic doesn't solve any of that, you really need to make sure
> nothing tries to communicate with the device while you're suspending
> it, hence the suggestion to use a rw_semaphore to protect against that.
>
> > +
> > ops->retlen = ops->oobretlen = 0;
> >
> > ret_code = mtd_check_oob_ops(mtd, from, ops);
> > @@ -1597,6 +1612,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> > struct mtd_info *master = mtd_get_master(mtd);
> > int ret;
> >
> > + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
> > +
>
> Please don't open-code this in every IO path, add helpers hiding all the
> complexity.
>
> To sum-up, that's more or less what I add in mind:
>
> static void mtd_start_access(struct mtd_info *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 (!mtd->_suspend)
> return;
>
> /*
> * Wait until the device is resumed. Should we have a
> * non-blocking mode here?
> */
> while (1) {
> down_read(&mtd->suspend_lock);
> if (!mtd->suspended)
> return;
>
> up_read(&mtd->suspend_lock);
> wait_event(mtd->resume_wq, mtd->suspended == false);
> }
> }
>
> static void mtd_end_access(struct mtd_info *mtd)
> {
> if (!mtd->_suspend)
> return;
>
> up_read(&mtd->suspend_lock);
> }
>
> static void mtd_suspend(struct mtd_info *mtd)
> {
> int ret;
>
> if (!mtd->_suspend)
> return;
>
> down_write(&mtd->suspend_lock);
> if (mtd->suspended == false) {
> ret = mtd->_suspend(mtd);
> if (!ret)
> mtd->suspended = true;
> }
> up_write(&mtd->suspend_lock);
> }
>
> static void mtd_resume(struct mtd_info *mtd)
> {
> if (!mtd->_suspend)
> return;
>
> down_write(&mtd->suspend_lock);
> if (mtd->suspended) {
> if (mtd->_resume)
> mtd->_resume(mtd);
>
> mtd->suspended = false;
>
> /* The MTD dev has been resumed, wake up all waiters. */
> wake_up_all(&mtd->resume_wq)
> }
> up_write(&mtd->suspend_lock);
> }
>
> You then need to call mtd_{start,end}_access() in all MTD IO path
> (read/write/erase and maybe others too).
Looks cool.
But you are introducing a new lock that basically does the
same as chip->lock in nand_base.c one level above ;)
You wrote that we didn't want to introduce a new lock :)
I will this code...
On Fri, 8 Oct 2021 13:54:13 +0200
Sean Nyekjaer <[email protected]> wrote:
> On Fri, Oct 08, 2021 at 01:20:38PM +0200, Boris Brezillon wrote:
> > Hi Sean,
> >
> > On Fri, 8 Oct 2021 12:04:25 +0200
> > Sean Nyekjaer <[email protected]> wrote:
> >
> > > On Thu, Oct 07, 2021 at 03:14:26PM +0200, Boris Brezillon wrote:
> > > > On Thu, 7 Oct 2021 14:39:16 +0200
> > > > Sean Nyekjaer <[email protected]> wrote:
> > > >
> > > > > >
> > > > > > wait_queue doesn't really describe what this waitqueue is used for
> > > > > > (maybe resume_wq), and the suspended state should be here as well
> > > > > > (actually, there's one already).
> > > > >
> > > > > I'll rename to something meaningful.
> > > > > >
> > > > > > Actually, what we need is a way to prevent the device from being
> > > > > > suspended while accesses are still in progress, and new accesses from
> > > > > > being queued if a suspend is pending. So, I think you need a readwrite
> > > > > > lock here:
> > > > > >
> > > > > > * take the lock in read mode for all IO accesses, check the
> > > > > > mtd->suspended value
> > > > > > - if true, release the lock, and wait (retry on wakeup)
> > > > > > - if false, just do the IO
> > > > > >
> > > > > > * take the lock in write mode when you want to suspend/resume the
> > > > > > device and update the suspended field. Call wake_up_all() in the
> > > > > > resume path
> > > > >
> > > > > Could we use the chip->lock mutex for this? It's does kinda what you
> > > > > described above?
> > > >
> > > > No you can't. Remember I suggested to move all of that logic to
> > > > mtdcore.c, which doesn't know about the nand_chip struct.
> > > >
> > > > > If we introduce a new lock, do we really need to have the suspended as
> > > > > an atomic?
> > > >
> > > > Nope, I thought we could do without a lock, but we actually need to
> > > > track active IO requests, not just the suspended state.
> > >
> > > I have only added wait_queue to read and write operations.
> >
> > It's still racy (see below).
> >
> > > I'll have a look into where we should add further checks.
> > >
> > > >
> > > > >
> > > > > I will test with some wait and retry added to nand_get_device().
> > > >
> > > > Again, I think there's a misunderstanding here: if you move it to the
> > > > mtd layer, it can't be done in nand_get_device(). But once you've
> > > > implemented it in mtdcore.c, you should be able to get rid of the
> > > > nand_chip->suspended field.
> > >
> > > I have moved the suspended atomic and wake_queue to mtdcore.c.
> >
> > That doesn't work (see below).
> >
> > > And kept
> > > the suspended variable in nand_base as is fine for chip level suspend
> > > status.
> >
> > Why? If you handle that at the MTD level you shouldn't need it at the
> > NAND level? BTW, would you please care to detail your reasoning when
> > you say you did or didn't do something. It's a bit hard to guess what
> > led you to this conclusion...
> >
> > >
> > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > > index c8fd7f758938..6492071eb4da 100644
> > > --- a/drivers/mtd/mtdcore.c
> > > +++ b/drivers/mtd/mtdcore.c
> > > @@ -42,15 +42,24 @@ static int mtd_cls_suspend(struct device *dev)
> > > {
> > > struct mtd_info *mtd = dev_get_drvdata(dev);
> > >
> > > - return mtd ? mtd_suspend(mtd) : 0;
> > > + if (mtd) {
> > > + atomic_inc(&mtd->suspended);
> > > + return mtd_suspend(mtd);
> > > + }
> > > + + return 0;
> > > }
> > >
> > > static int mtd_cls_resume(struct device *dev)
> > > {
> > > struct mtd_info *mtd = dev_get_drvdata(dev);
> > >
> > > - if (mtd)
> > > + if (mtd) {
> > > mtd_resume(mtd);
> > > + atomic_dec(&mtd->suspended);
> > > + wake_up_all(&mtd->resume_wq);
> > > + }
> > > +
> > > return 0;
> > > }
> > > @@ -678,6 +687,10 @@ int add_mtd_device(struct mtd_info *mtd)
> > > if (error)
> > > goto fail_nvmem_add;
> > >
> > > + init_waitqueue_head(&mtd->resume_wq);
> > > +
> > > + atomic_set(&mtd->suspended, 0);
> > > +
> > > mtd_debugfs_populate(mtd);
> > >
> > > device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> > > @@ -1558,6 +1571,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
> > > struct mtd_ecc_stats old_stats = master->ecc_stats;
> > > int ret_code;
> > >
> > > + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
> >
> > That's racy:
> >
> > thread A thread B
> > |
> > enters mtd_read() |
> > passes the !suspended test |
> > | enters mtd_suspend()
> > | sets suspended to 1
> > |
> > starts the IO |
> > | suspends the device
> > tries to finish the IO |
> > on a suspended device |
> >
> > BOOM!
> >
> >
> > Using an atomic doesn't solve any of that, you really need to make sure
> > nothing tries to communicate with the device while you're suspending
> > it, hence the suggestion to use a rw_semaphore to protect against that.
> >
> > > +
> > > ops->retlen = ops->oobretlen = 0;
> > >
> > > ret_code = mtd_check_oob_ops(mtd, from, ops);
> > > @@ -1597,6 +1612,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> > > struct mtd_info *master = mtd_get_master(mtd);
> > > int ret;
> > >
> > > + wait_event(mtd->resume_wq, atomic_read(&mtd->suspended) == 0);
> > > +
> >
> > Please don't open-code this in every IO path, add helpers hiding all the
> > complexity.
> >
> > To sum-up, that's more or less what I add in mind:
> >
> > static void mtd_start_access(struct mtd_info *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 (!mtd->_suspend)
> > return;
> >
> > /*
> > * Wait until the device is resumed. Should we have a
> > * non-blocking mode here?
> > */
> > while (1) {
> > down_read(&mtd->suspend_lock);
> > if (!mtd->suspended)
> > return;
> >
> > up_read(&mtd->suspend_lock);
> > wait_event(mtd->resume_wq, mtd->suspended == false);
> > }
> > }
> >
> > static void mtd_end_access(struct mtd_info *mtd)
> > {
> > if (!mtd->_suspend)
> > return;
> >
> > up_read(&mtd->suspend_lock);
> > }
> >
> > static void mtd_suspend(struct mtd_info *mtd)
> > {
> > int ret;
> >
> > if (!mtd->_suspend)
> > return;
> >
> > down_write(&mtd->suspend_lock);
> > if (mtd->suspended == false) {
> > ret = mtd->_suspend(mtd);
> > if (!ret)
> > mtd->suspended = true;
> > }
> > up_write(&mtd->suspend_lock);
> > }
> >
> > static void mtd_resume(struct mtd_info *mtd)
> > {
> > if (!mtd->_suspend)
> > return;
> >
> > down_write(&mtd->suspend_lock);
> > if (mtd->suspended) {
> > if (mtd->_resume)
> > mtd->_resume(mtd);
> >
> > mtd->suspended = false;
> >
> > /* The MTD dev has been resumed, wake up all waiters. */
> > wake_up_all(&mtd->resume_wq)
> > }
> > up_write(&mtd->suspend_lock);
> > }
> >
> > You then need to call mtd_{start,end}_access() in all MTD IO path
> > (read/write/erase and maybe others too).
>
> Looks cool.
>
> But you are introducing a new lock that basically does the
> same as chip->lock in nand_base.c one level above ;)
It doesn't serve the same purpose, no. This one is making sure suspend
can't happen while IOs are in-flight, and IOs can't happen while the
device is being suspended. The nand_chip->lock serializes all IO going
through a chip (the new mtd->suspend_lock doesn't guarantee that). This
being said, once you have this, you should be able to get rid of the
nand_chip->suspended field.
> You wrote that we didn't want to introduce a new lock :)
Again, that's not what I said. I said using a lock to wait on devices
going out of suspend was a bad idea, because then the lock is held when
you enter suspend, and only released when the device gets resumed.
That's quite a big/unbounded critical section, and we try to
avoid that in general (ideally locks should be taken/released in the
same function). What I do here is quite different, see how the
mtd->suspend_lock is released before calling wait_event().
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.
Exec_op[0] speed things up, so we see this race when rawnand devices going
into suspend. But it's actually "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 <[email protected]>
---
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;
+
+ /*
+ * 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);
+}
+
#ifdef CONFIG_PM_SLEEP
static int mtd_cls_suspend(struct device *dev)
@@ -1000,6 +1038,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
ret = mtd_otp_nvmem_add(mtd);
+ init_waitqueue_head(&mtd->master.resume_wq);
+ init_rwsem(&mtd->master.suspend_lock);
+
out:
if (ret && device_is_registered(&mtd->dev))
del_mtd_device(mtd);
@@ -1241,6 +1282,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
struct erase_info adjinstr;
int ret;
+ mtd_start_access(mtd);
+
instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
adjinstr = *instr;
@@ -1278,6 +1321,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
}
}
+ mtd_end_access(mtd);
+
return ret;
}
EXPORT_SYMBOL_GPL(mtd_erase);
@@ -1558,6 +1603,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
struct mtd_ecc_stats old_stats = master->ecc_stats;
int ret_code;
+ mtd_start_access(mtd);
+
ops->retlen = ops->oobretlen = 0;
ret_code = mtd_check_oob_ops(mtd, from, ops);
@@ -1577,6 +1624,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
mtd_update_ecc_stats(mtd, master, &old_stats);
+ mtd_end_access(mtd);
+
/*
* In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
* similar to mtd->_read(), returning a non-negative integer
@@ -1597,6 +1646,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
struct mtd_info *master = mtd_get_master(mtd);
int ret;
+ mtd_start_access(mtd);
+
ops->retlen = ops->oobretlen = 0;
if (!(mtd->flags & MTD_WRITEABLE))
@@ -1615,7 +1666,11 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
return mtd_io_emulated_slc(mtd, to, false, ops);
- return mtd_write_oob_std(mtd, to, ops);
+ ret = mtd_write_oob_std(mtd, to, ops);
+
+ mtd_end_access(mtd);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(mtd_write_oob);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 88227044fc86..cfab07b02dc9 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 {
@@ -546,30 +548,42 @@ 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;
+ int ret = 0;
- if (master->master.suspended)
- return 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);
- master->master.suspended = 0;
+ down_write(&master->master.suspend_lock);
+ if (master->master.suspended) {
+ if (master->_resume)
+ master->_resume(master);
+
+ 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
Rawnand access is protected in upper mtd layer when mtd devices are
suspended. So remove this obsolute check/lock
Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
Signed-off-by: Sean Nyekjaer <[email protected]>
---
Hmm what to do in nand_sync()? Fine as is?
drivers/mtd/nand/raw/nand_base.c | 50 ++++++++------------------------
include/linux/mtd/rawnand.h | 5 +---
2 files changed, 13 insertions(+), 42 deletions(-)
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3d6c6e880520..f1f85866c87a 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);
@@ -4349,9 +4336,7 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
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
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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
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);
> +}
> +
Hi Sean,
[email protected] wrote on Fri, 8 Oct 2021 16:38:24 +0200:
> 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.
Maybe you can use the present tense, as in:
Prevent reading/...
s/mtd/MTD/
Force mtd_write/...
I would suggest something like:
Prevent accessing the devices while in a suspended state. Also
prevent suspending a device which is still currently in use.
> Exec_op[0] speed things up, so we see this race when rawnand devices going
I believe you can quote the commit inline, like below (please use the
12-digit hash below as well).
I am not sure ->exec_op() is to be blamed here, maybe this change
revealed the issue but I doubt it is because of its efficiency. The
problem was just laying silently IMHO.
> into suspend. But it's actually "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.
I don't think so, I believe it was broken in the same way but was just
not returning errors.
>
> Tested on a iMX6ULL.
>
> [0]:
> ef347c0cfd61 ("mtd: rawnand: gpmi: Implement exec_op")
>
Suggested-by: Boris...
would be nice.
> Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> Signed-off-by: Sean Nyekjaer <[email protected]>
> ---
>
> 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;
> +
> + /*
> + * 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);
"var == 0" translates well to "!var"
> + }
> +}
> +
> +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);
> +}
> +
> #ifdef CONFIG_PM_SLEEP
>
> static int mtd_cls_suspend(struct device *dev)
> @@ -1000,6 +1038,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>
> ret = mtd_otp_nvmem_add(mtd);
>
> + init_waitqueue_head(&mtd->master.resume_wq);
> + init_rwsem(&mtd->master.suspend_lock);
what about setting this in mtd_set_dev_defaults()?
> +
> out:
> if (ret && device_is_registered(&mtd->dev))
> del_mtd_device(mtd);
> @@ -1241,6 +1282,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
> struct erase_info adjinstr;
> int ret;
>
> + mtd_start_access(mtd);
I believe we should cover all the ioctls, even if they are not
accessing the device. I don't think it's a problem to stop
interacting when the devices are suspended?
> +
> instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> adjinstr = *instr;
>
> @@ -1278,6 +1321,8 @@ int mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
> }
> }
>
> + mtd_end_access(mtd);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(mtd_erase);
> @@ -1558,6 +1603,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
> struct mtd_ecc_stats old_stats = master->ecc_stats;
> int ret_code;
>
> + mtd_start_access(mtd);
> +
> ops->retlen = ops->oobretlen = 0;
>
> ret_code = mtd_check_oob_ops(mtd, from, ops);
> @@ -1577,6 +1624,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
>
> mtd_update_ecc_stats(mtd, master, &old_stats);
>
> + mtd_end_access(mtd);
> +
> /*
> * In cases where ops->datbuf != NULL, mtd->_read_oob() has semantics
> * similar to mtd->_read(), returning a non-negative integer
> @@ -1597,6 +1646,8 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> struct mtd_info *master = mtd_get_master(mtd);
> int ret;
>
> + mtd_start_access(mtd);
> +
> ops->retlen = ops->oobretlen = 0;
>
> if (!(mtd->flags & MTD_WRITEABLE))
> @@ -1615,7 +1666,11 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to,
> if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
> return mtd_io_emulated_slc(mtd, to, false, ops);
>
> - return mtd_write_oob_std(mtd, to, ops);
> + ret = mtd_write_oob_std(mtd, to, ops);
> +
> + mtd_end_access(mtd);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(mtd_write_oob);
>
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 88227044fc86..cfab07b02dc9 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 {
> @@ -546,30 +548,42 @@ 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;
> + int ret = 0;
>
> - if (master->master.suspended)
> - return 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);
>
> - master->master.suspended = 0;
> + down_write(&master->master.suspend_lock);
> + if (master->master.suspended) {
> + if (master->_resume)
> + master->_resume(master);
> +
> + 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)
The other patch lgtm.
Thanks,
Miquèl
On Fri, 8 Oct 2021 17:35:26 +0200
Miquel Raynal <[email protected]> wrote:
> > into suspend. But it's actually "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.
>
> I don't think so, I believe it was broken in the same way but was just
> not returning errors.
Actually I was wrong, 013e6292aaf5 ("mtd: rawnand: Simplify the
locking") removed the blocking wait (returning -EBUSY when the device
is suspended instead of putting the thread on a waitqueue). At that
time, I assumed all threads would be paused when the device is
suspended, which appeared to be incorrect. So I guess the Fixes tag
should remain, and we might want to consider backporting a less
invasive patch to stable releases (one touching only the raw NAND
layer).
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
> 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);
> > +}
> > +
On Fri, Oct 08, 2021 at 06:08:11PM +0200, Boris Brezillon wrote:
> On Fri, 8 Oct 2021 17:35:26 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > > into suspend. But it's actually "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.
> >
> > I don't think so, I believe it was broken in the same way but was just
> > not returning errors.
>
> Actually I was wrong, 013e6292aaf5 ("mtd: rawnand: Simplify the
> locking") removed the blocking wait (returning -EBUSY when the device
> is suspended instead of putting the thread on a waitqueue). At that
> time, I assumed all threads would be paused when the device is
> suspended, which appeared to be incorrect. So I guess the Fixes tag
> should remain, and we might want to consider backporting a less
> invasive patch to stable releases (one touching only the raw NAND
> layer).
Thanks Miquel add Reviewed-By you on the second patch.
I'll remove the mentioning of commit ef347c0cfd61
("mtd: rawnand: gpmi: Implement exec_op") in this commit msg.
Is it possible to backport another(less invasive) patch to stable
releases? I thought only upstream commits could be backported.
/Sean