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