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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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 ;-)


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/



[Index of Archives]     [LARTC]     [Bugtraq]     [Yosemite Forum]     [Photo]

  Powered by Linux