| Message ID | 20180321130157.9524-17-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | Fix probe functions error path | expand |
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. 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. > > if (sets != NULL) > sets++;
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
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); if (sets != NULL) sets++;
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(-)