[RFC] mtd: rawnand: use mutex to protect access while in suspend

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 <sean@geanix.com>
---

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(-)

On Mon,  4 Oct 2021 08:56:09 +0200
Sean Nyekjaer <sean@geanix.com> 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 <sean@geanix.com>
> ---
> 
> 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 <sean@geanix.com> 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 <sean@geanix.com>
> > ---
> > 
> > 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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 <sean@geanix.com> 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().