Re: [PATCH v2] mtd: nand: gpmi: Release DMA channels on failure

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 28 Dec 2017 16:17:58 -0200
Fabio Estevam <festevam@gmail.com> wrote:

> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> release_dma_channels() should be called in the case of error in
> gpmi_init() or bch_set_geometry(), because acquire_dma_channels() has
> been called previously.

I'm really not sure this is a good idea. According to [1], the error
code returned by PM hooks is not taken into account, and the device
will be considered as functional by the system even if
->suspend() returns an error. This means, next time we enter suspend,
gpmi_pm_suspend() will be called which will lead to an unbalanced
release_dma_channels()/acquire_dma_channels().

> 
> Handle the error cases correctly.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
> Changes since v1:
> - Remove extra blank line
> 
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 2ef8979..db0d924 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -2137,20 +2137,24 @@ static int gpmi_pm_resume(struct device *dev)
>  	ret = gpmi_init(this);
>  	if (ret) {
>  		dev_err(this->dev, "Error setting GPMI : %d\n", ret);
> -		return ret;
> +		goto release_dma_channels;
>  	}
>  
>  	/* re-init the BCH registers */
>  	ret = bch_set_geometry(this);
>  	if (ret) {
>  		dev_err(this->dev, "Error setting BCH : %d\n", ret);
> -		return ret;
> +		goto release_dma_channels;
>  	}
>  
>  	/* re-init others */
>  	gpmi_extra_init(this);
>  
>  	return 0;
> +
> +release_dma_channels:
> +	release_dma_channels(this);
> +	return ret;
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  

[1]https://elixir.free-electrons.com/linux/v4.15-rc6/source/include/linux/pm.h#L262