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

[Christophe Kerello]

On 4/27/20 10:10 PM, Marek Vasut wrote:
> On 4/27/20 10:08 PM, Miquel Raynal wrote:
> > Hi Marek,
> >
> > Marek Vasut <marex@xxxxxxx> wrote on Mon, 27 Apr 2020 21:46:44 +0200:
> >
> > > On 4/27/20 8:08 PM, Miquel Raynal wrote:
> > > [...]
> > > > > > >   	/* 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)
> > >
> > > I find it useful to have a unified way to find those labels, e.g.
> > > err_because_foo:
> > > err_because_bar:
> > > err_last_one:
> > > is much nicer than:
> > > foo_failed:
> > > bar_also_failed:
> > > its_total_randomness:
> >
> > My point being, Christophe, you can use err_ as a prefix but I think
> > it's better to use:
> >
> >      err_do_this_cleanup
> >
> > than
> >
> >     err_this_failed
>
> That's fine either way.

Hi Miquel,

I will rename the label in v3:
 - err_device_register => err_nand_cleanup
 - err_dma_setup => err_release_dma
 - err_clk_disable => will keep this one

Regards,
Christophe Kerello.

> > > > Any way I suppose catching ":\n" is already a good approximation to
> > > > find labels?
> > >
> > > Not very practical with git grep (^err.*: works nicely though)
> >
> > I suppose ^.*:$ would work the same ;)
>
> Try and see how much other irrelevant stuff that sucks in ;-)