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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Apr 2020 19:59:34 +0200
From:   Marek Vasut <marex@...x.de>
To:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Christophe Kerello <christophe.kerello@...com>
Cc:     richard@....at, vigneshr@...com, lee.jones@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, tony@...mide.com,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 04/12] mtd: rawnand: stm32_fmc2: manage all errors
 cases at probe time

On 4/27/20 7:47 PM, Miquel Raynal wrote:
> Hi Christophe,
> 
> Christophe Kerello <christophe.kerello@...com> 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@...com>
>> ---
>>  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.

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.