Re: [PATCH 2/3] spi: atmel-quadspi: Free resources even if runtime resume failed in .remove()

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

On 3/17/23 08:42, Uwe Kleine-König wrote:
> An early error exit in atmel_qspi_remove() doesn't prevent the device
> unbind. So this results in an spi controller with an unbound parent
> and unmapped register space (because devm_ioremap_resource() is undone).
> So using the remaining spi controller probably results in an oops.
> 
> Instead unregister the controller unconditionally and only skip hardware
> access and clk disable.
> 
> Also add a warning about resume failing and return zero unconditionally.
> The latter has the only effect to suppress a less helpful error message by
> the spi core.
> 
> Fixes: 4a2f83b7f780 ("spi: atmel-quadspi: add runtime pm support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>

Reviewed-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>

> ---
>  drivers/spi/atmel-quadspi.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 0c6f80ddea57..713a4d6700fd 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -706,18 +706,28 @@ static int atmel_qspi_remove(struct platform_device *pdev)
>  	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
>  	int ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> -	if (ret < 0)
> -		return ret;
> -
>  	spi_unregister_controller(ctrl);
> -	atmel_qspi_write(QSPI_CR_QSPIDIS, aq, QSPI_CR);
> +
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (ret >= 0) {
> +		atmel_qspi_write(QSPI_CR_QSPIDIS, aq, QSPI_CR);
> +		clk_disable(aq->qspick);
> +		clk_disable(aq->pclk);
> +	} else {
> +		/*
> +		 * atmel_qspi_runtime_{suspend,resume} just disable and enable
> +		 * the two clks respectively. So after resume failed these are
> +		 * off, and we skip hardware access and disabling these clks again.
> +		 */
> +		dev_warn(&pdev->dev, "Failed to resume device on remove\n");
> +	}
> +
> +	clk_unprepare(aq->qspick);
> +	clk_unprepare(aq->pclk);
>  
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
>  
> -	clk_disable_unprepare(aq->qspick);
> -	clk_disable_unprepare(aq->pclk);
>  	return 0;
>  }
>