Re: [PATCH 3/3] mtd: mtdconcat: add suspend lock handling

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

On Mon, 11 Oct 2021 15:35:56 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On Mon, Oct 11, 2021 at 03:27:03PM +0200, Boris Brezillon wrote:
> > On Mon, 11 Oct 2021 15:15:01 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Mon, 11 Oct 2021 13:52:53 +0200
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > >   
> > > > Use new suspend lock handling for this special case for concatenated
> > > > MTD devices.
> > > > 
> > > > Fixes: 013e6292aaf5 ("mtd: rawnand: Simplify the locking")
> > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > > ---
> > > >  drivers/mtd/mtdconcat.c | 11 +++++++++--
> > > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c
> > > > index f685a581df48..c497c851481f 100644
> > > > --- a/drivers/mtd/mtdconcat.c
> > > > +++ b/drivers/mtd/mtdconcat.c
> > > > @@ -561,25 +561,32 @@ static void concat_sync(struct mtd_info *mtd)
> > > >  
> > > >  static int concat_suspend(struct mtd_info *mtd)
> > > >  {
> > > > +	struct mtd_info *master = mtd_get_master(mtd);
> > > >  	struct mtd_concat *concat = CONCAT(mtd);
> > > >  	int i, rc = 0;
> > > >  
> > > >  	for (i = 0; i < concat->num_subdev; i++) {
> > > >  		struct mtd_info *subdev = concat->subdev[i];
> > > > -		if ((rc = mtd_suspend(subdev)) < 0)
> > > > +
> > > > +		down_write(&master->master.suspend_lock);
> > > > +		if ((rc = __mtd_suspend(subdev)) < 0)
> > > >  			return rc;
> > > > +		up_write(&master->master.suspend_lock);
> > > >  	}
> > > >  	return rc;
> > > >  }
> > > >  
> > > >  static void concat_resume(struct mtd_info *mtd)
> > > >  {
> > > > +	struct mtd_info *master = mtd_get_master(mtd);
> > > >  	struct mtd_concat *concat = CONCAT(mtd);
> > > >  	int i;
> > > >  
> > > >  	for (i = 0; i < concat->num_subdev; i++) {
> > > >  		struct mtd_info *subdev = concat->subdev[i];
> > > > -		mtd_resume(subdev);
> > > > +		down_write(&master->master.suspend_lock);
> > > > +		__mtd_resume(subdev);
> > > > +		up_write(&master->master.suspend_lock);
> > > >  	}
> > > >  }
> > > >      
> > > 
> > > Why do we need to implement the _suspend/_resume() hooks here? The
> > > underlying MTD devices should be suspended at some point (when the
> > > class ->suspend() method is called on those device), and there's
> > > nothing mtdconcat-specific to do here. Looks like implementing this
> > > suspend-all-subdevs loop results in calling mtd->_suspend()/_resume()
> > > twice, which is useless. The only issue I see is if the subdevices
> > > haven't been registered to the device model, but that happens, I
> > > believe we have bigger issues (those devices won't be suspended when
> > > mtdconcat is not used).  
> > 
> > 
> > Uh, just had a look at mtd_concat_create() callers, and they indeed
> > don't register the subdevices, so I guess the suspend-all-subdevs loop
> > is needed. I really thought mtdconcat was something more generic
> > aggregating already registered devices...  
> 
> Hi Boris,
> 
> Cool, mtd_concat should be seen as mtd devices concatenated? Could be
> spi-nors and rawnand. So _suspend() needs to be called for every device
> layer?

If those subdevices were registered as MTD devices too, you wouldn't
have to call ->_suspend()/_resume() on the subdevices (the core does it
for you for every registered 'struct device' object).

> 
> From what I see here, mtd_suspend()/mtd_resume() is called for every mtd
> device. Before this patch mtd_suspend() would only have effect on the
> first device as master->master.suspended is set and then calls to
> device specific _suspend() is skipped.

Unfortunately, subdevices are not exposed to the MTD framework, only the
aggregate is, so suspend is called once in that case, and the mtdconcat
layer has to propagate it to all its subdevices. This makes mtdconcat
unsuitable for a generic MTD aggregation layer BTW (which I remember
discussing with someone on the MTD ML in the past).