Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 04/12] mtd: rawnand: stm32_fmc2: manage all errors
 cases at probe time
To:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Christophe Kerello <christophe.kerello@st.com>
Cc:     richard@nod.at, vigneshr@ti.com, lee.jones@linaro.org,
        robh+dt@kernel.org, mark.rutland@arm.com, tony@atomide.com,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        devicetree@vger.kernel.org
References: <1586966256-29548-1-git-send-email-christophe.kerello@st.com>
 <1586966256-29548-5-git-send-email-christophe.kerello@st.com>
 <20200427194747.224a2402@xps13>
From:   Marek Vasut <marex@denx.de>
Message-ID: <40a9bac7-9ed4-b781-f2c2-2d90b4e82749@denx.de>
Date:   Mon, 27 Apr 2020 19:59:34 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200427194747.224a2402@xps13>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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