[RFC PATCH] mtd: rawnand: use mutex to protect access while in suspend
Boris Brezillon
boris.brezillon at collabora.com
Fri Oct 8 05:15:24 PDT 2021
On Fri, 8 Oct 2021 13:54:13 +0200
Sean Nyekjaer <sean at 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 at 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 at 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().
More information about the linux-mtd
mailing list