lkml.org 
[lkml]   [2021]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] mtd: rawnand: use mutex to protect access while in suspend
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.

\
 
 \ /
  Last update: 2021-10-07 15:14    [W:0.077 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site