Re: [PATCH v2 04/12] mtd: rawnand: stm32_fmc2: manage all errors cases at probe time

[Miquel Raynal]

Hi Marek,

Marek Vasut <marex@xxxxxxx> wrote on Mon, 27 Apr 2020 19:59:34 +0200:

> On 4/27/20 7:47 PM, Miquel Raynal wrote:
> > Hi Christophe,
> > 
> > Christophe Kerello <christophe.kerello@xxxxxx> wrote on Wed, 15 Apr
> > 2020 17:57:28 +0200:
> >   
> >> This patch defers its probe when the expected reset control is not
> >> yet ready. This patch also handles properly all errors cases at probe
> >> time.
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@xxxxxx>
> >> ---
> >>  drivers/mtd/nand/raw/stm32_fmc2_nand.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> index b6d45cd..0a96797 100644
> >> --- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
> >> @@ -1967,7 +1967,11 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >>  	}
> >>  
> >>  	rstc = devm_reset_control_get(dev, NULL);
> >> -	if (!IS_ERR(rstc)) {
> >> +	if (IS_ERR(rstc)) {
> >> +		ret = PTR_ERR(rstc);
> >> +		if (ret == -EPROBE_DEFER)
> >> +			goto err_clk_disable;
> >> +	} else {
> >>  		reset_control_assert(rstc);
> >>  		reset_control_deassert(rstc);
> >>  	}
> >> @@ -1975,7 +1979,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >>  	/* DMA setup */
> >>  	ret = stm32_fmc2_dma_setup(fmc2);
> >>  	if (ret)
> >> -		return ret;
> >> +		goto err_dma_setup;
> >>  
> >>  	/* FMC2 init routine */
> >>  	stm32_fmc2_init(fmc2);
> >> @@ -1997,7 +2001,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >>  	/* Scan to find existence of the device */
> >>  	ret = nand_scan(chip, nand->ncs);
> >>  	if (ret)
> >> -		goto err_scan;
> >> +		goto err_dma_setup;
> >>  
> >>  	ret = mtd_device_register(mtd, NULL, 0);
> >>  	if (ret)
> >> @@ -2010,7 +2014,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >>  err_device_register:
> >>  	nand_cleanup(chip);
> >>  
> >> -err_scan:
> >> +err_dma_setup:
> >>  	if (fmc2->dma_ecc_ch)
> >>  		dma_release_channel(fmc2->dma_ecc_ch);
> >>  	if (fmc2->dma_tx_ch)
> >> @@ -2021,6 +2025,7 @@ static int stm32_fmc2_probe(struct platform_device *pdev)
> >>  	sg_free_table(&fmc2->dma_data_sg);
> >>  	sg_free_table(&fmc2->dma_ecc_sg);
> >>  
> >> +err_clk_disable:
> >>  	clk_disable_unprepare(fmc2->clk);
> >>  
> >>  	return ret;  
> > 
> > I didn't spot it during my earlier reviews but I really prefer using
> > labels explaining what you do than having the same name of the function
> > which failed. This way you don't have to rework the error path when
> > you handle an additional error.
> > 
> > So, would you mind doing this in two steps:
> > 
> > 1/
> > Replace
> > 
> >     err_scan:
> > 
> > with, eg.
> > 
> >     release_dma_objs:  
> 
> The ^err_ prefix in failpath labels is useful, since it's easily
> possible to match on it with regexes ; not so much on arbitrary label name.

I guess so, but is it actually useful to catch labels in a regex? (real
question)

Any way I suppose catching ":\n" is already a good approximation to
find labels?

> 
> btw would it make sense to split the first three patches of this series
> into a separate series ? This rawnand part seems more like an unrelated
> cleanup.

As it seems that the MFD discussion can take longer, then I would say
yes, at least for the cleanup/misc changes part.


Thanks,
MiquÃl