Re: [PATCH v2 16/16] mtd: rawnand: s3c2410: enhance the probe function error path

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

On Tue, 27 Mar 2018 11:06:50 +0200, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Wed, 21 Mar 2018 14:01:57 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Prepare the migration of the lpc32xx_slc driver to use nand_scan() by
> > cleaning the error path in the probe function.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/s3c2410.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/s3c2410.c b/drivers/mtd/nand/raw/s3c2410.c
> > index b5bc5f106c09..1bc0458063d8 100644
> > --- a/drivers/mtd/nand/raw/s3c2410.c
> > +++ b/drivers/mtd/nand/raw/s3c2410.c
> > @@ -124,13 +124,11 @@ struct s3c2410_nand_info;
> >   * @chip: The NAND chip information.
> >   * @set: The platform information supplied for this set of NAND chips.
> >   * @info: Link back to the hardware information.
> > - * @scan_res: The result from calling nand_scan_ident().
> >  */
> >  struct s3c2410_nand_mtd {
> >  	struct nand_chip		chip;
> >  	struct s3c2410_nand_set		*set;
> >  	struct s3c2410_nand_info	*info;
> > -	int				scan_res;
> >  };
> >  
> >  enum s3c_cpu_type {
> > @@ -1163,17 +1161,19 @@ static int s3c24xx_nand_probe(struct platform_device *pdev)
> >  		mtd->dev.parent = &pdev->dev;
> >  		s3c2410_nand_init_chip(info, nmtd, sets);
> >  
> > -		nmtd->scan_res = nand_scan_ident(mtd,
> > -						 (sets) ? sets->nr_chips : 1,
> > -						 NULL);
> > +		err = nand_scan_ident(mtd, (sets) ? sets->nr_chips : 1, NULL);
> > +		if (err)
> > +			goto exit_error;
> >  
> > -		if (nmtd->scan_res == 0) {
> > -			err = s3c2410_nand_update_chip(info, nmtd);
> > -			if (err < 0)
> > -				goto exit_error;
> > -			nand_scan_tail(mtd);
> > -			s3c2410_nand_add_partition(info, nmtd, sets);
> > -		}
> > +		err = s3c2410_nand_update_chip(info, nmtd);
> > +		if (err < 0)
> > +			goto exit_error;
> > +
> > +		err = nand_scan_tail(mtd);
> > +		if (err)
> > +			goto exit_error;
> > +
> > +		s3c2410_nand_add_partition(info, nmtd, sets);  
> 
> Not related to this patch, but it seems this we're ignoring the return
> value of s3c2410_nand_add_partition(). Don't know if this is
> intentional.

I known, and that is not the only one having very weird probe
functions, but because I cannot actually test the code and because my
main goal was absolutely not to fix every single NAND driver I chose to
not go into deeper details for now and focus on my target. This logic
should be fixed though.
 
> 
> And there's something even weirder: s3c2410_nand_add_partition() only
> registers the MTD device if sets != NULL, so what's the point of
> scanning the bus if we know we'll not register the NAND chips we find
> there? Looks like this probe path is overly complex for no real reason.
> 
> Anyway, I'll take your patch since it tends to improve a bit the code.

I addressed all your other comments and will send a new version of the
remaining patches.

Thank you,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com