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

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

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.