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

[Boris Brezillon <boris.brezillon@collabora.com>]

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