[PATCH v2 2/2] mtd: rawnand: add hooks that may be called during nand_scan()

[miquel.raynal@xxxxxxxxxxx (Miquel Raynal)]

Hi Boris,

Boris Brezillon <boris.brezillon at bootlin.com> wrote on Thu, 19 Jul 2018
00:46:36 +0200:

> On Thu, 19 Jul 2018 00:42:25 +0200
> Boris Brezillon <boris.brezillon at bootlin.com> wrote:
> 
> > On Wed, 18 Jul 2018 23:50:56 +0200
> > Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> >   
> > > In order to remove the limitation that forbids dynamic allocation in
> > > nand_scan_ident(), we must create a path that will be the same for all
> > > controller drivers. The idea is to use nand_scan() instead of the widely
> > > used nand_scan_ident()/nand_scan_tail() couple. In order to achieve
> > > this, controller drivers will need to adjust some parameters between
> > > these two functions depending on the NAND chip wired on them.
> > > 
> > > For that, a hook called ->attach_chip() is created as part of the
> > > nand_controller_ops structure, itself embedded in the nand_controller
> > > structure. The later may be referenced by two ways:
> > > 1/ if the driver does not implement its own controller, the
> > >    chip->controller hook is not populated before nand_scan() so it
> > >    cannot be dereferenced: use chip->dummy_controller instead (which is
> > >    statically allocated and will be referenced later by chip->controller
> > >    anyway).
> > > 2/ through chip->controller if the driver implements its own controller.
> > > 
> > > Another hook, ->detach_chip() is also introduced in order to clean the
> > > controller driver's potential allocations in case of failure of
> > > nand_scan_tail(). There is no need for the controller driver to call the    
> > > ->detach_chip() hook directly upon error after a successful nand_scan().      
> > > In this situation, calling nand_release() as before is enough.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > > Acked-by: Boris Brezillon <boris.brezillon at bootlin.com>
> > > ---
> > >  drivers/mtd/nand/raw/nand_base.c | 32 ++++++++++++++++++++++++++++++--
> > >  include/linux/mtd/rawnand.h      | 16 ++++++++++++++++
> > >  2 files changed, 46 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > > index 0aef0299f4f2..a64a344bf7a9 100644
> > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > @@ -6697,6 +6697,20 @@ EXPORT_SYMBOL(nand_scan_tail);
> > >  	is_module_text_address((unsigned long)__builtin_return_address(0))
> > >  #endif
> > >  
> > > +static int nand_attach(struct nand_chip *chip)
> > > +{
> > > +	if (chip->controller->ops && chip->controller->ops->attach_chip)
> > > +		return chip->controller->ops->attach_chip(chip);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void nand_detach(struct nand_chip *chip)
> > > +{
> > > +	if (chip->controller->ops && chip->controller->ops->detach_chip)
> > > +		chip->controller->ops->detach_chip(chip);
> > > +}
> > > +
> > >  /**
> > >   * nand_scan_with_ids - [NAND Interface] Scan for the NAND device
> > >   * @mtd: MTD device structure
> > > @@ -6710,11 +6724,21 @@ EXPORT_SYMBOL(nand_scan_tail);
> > >  int nand_scan_with_ids(struct mtd_info *mtd, int maxchips,
> > >  		       struct nand_flash_dev *ids)
> > >  {
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > >  	int ret;
> > >  
> > >  	ret = nand_scan_ident(mtd, maxchips, ids);
> > > -	if (!ret)
> > > -		ret = nand_scan_tail(mtd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = nand_attach(chip);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = nand_scan_tail(mtd);
> > > +	if (ret)
> > > +		nand_detach(chip);
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL(nand_scan_with_ids);
> > > @@ -6742,7 +6766,11 @@ void nand_cleanup(struct nand_chip *chip)
> > >  
> > >  	/* Free manufacturer priv data. */
> > >  	nand_manufacturer_cleanup(chip);
> > > +
> > > +	/* Free controller specific allocations after chip identification */
> > > +	nand_detach(chip);
> > >  }
> > > +
> > >  EXPORT_SYMBOL_GPL(nand_cleanup);
> > >  
> > >  /**
> > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > index eb5a053fd06b..d046b3a61205 100644
> > > --- a/include/linux/mtd/rawnand.h
> > > +++ b/include/linux/mtd/rawnand.h
> > > @@ -509,6 +509,19 @@ struct nand_id {
> > >  	int len;
> > >  };
> > >  
> > > +/**
> > > + * struct nand_controller_ops - Controller operations
> > > + *
> > > + * @attach_chip:	Callback that may be called between nand_detect() and
> > > + *			nand_scan_tail() during nand_scan() (optional).
> > > + * @detach_chip:	Callback that may be called if nand_scan_tail() fails
> > > + *			(optional).
> > > + */
> > > +struct nand_controller_ops {
> > > +	int (*attach_chip)(struct nand_chip *chip);
> > > +	void (*detach_chip)(struct nand_chip *chip);
> > > +};
> > > +
> > >  /**
> > >   * struct nand_controller - Control structure for hardware controller
> > >   *                          shared among independent devices
> > > @@ -517,11 +530,13 @@ struct nand_id {
> > >   * @wq:			wait queue to sleep on if a NAND operation is in
> > >   *			progress used instead of the per chip wait queue
> > >   *			when a hw controller is available.
> > > + * @ops:		NAND controller operations.
> > >   */
> > >  struct nand_controller {
> > >  	spinlock_t lock;
> > >  	struct nand_chip *active;
> > >  	wait_queue_head_t wq;
> > > +	const struct nand_controller_ops *ops;
> > >  };
> > >  
> > >  static inline void nand_controller_init(struct nand_controller *nfc)
> > > @@ -529,6 +544,7 @@ static inline void nand_controller_init(struct nand_controller *nfc)
> > >  	nfc->active = NULL;
> > >  	spin_lock_init(&nfc->lock);
> > >  	init_waitqueue_head(&nfc->wq);
> > > +	nfc->ops = NULL;    
> > 
> > Hm, actually it does not work for all dummy controllers since this
> > function is called from nand_set_defaults() which is called from
> > nand_scan_ident(). That means the previous chip->dummy_controller.ops
> > assignment you might have done before calling nand_scan() is gone.
> > 
> > Looks like we'll have to patch all drivers to call
> > nand_controller_init(&chip->dummy_controller), unless you see a better
> > option.  
> 
> Alternatively, we could assume that the nand_controller object is
> always zeroed at allocation time, and simply not assign nfc->ops from
> nand_controller_init() so that dummy controller drivers that need to
> pass non-NULL ops can do so before calling nand_scan().

Exactly what I just answered, then I'll just remove this line and we
should be good.

Thanks,
Miqu?l