Re: [PATCH] i2c: tegra: Fix failure during probe deferral cleanup

[Andi Shyti <andi.shyti@xxxxxxxxxx>]

Hi Thierry,

On Thu, Jun 08, 2023 at 09:56:06AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> If the driver fails to obtain a DMA channel, it will initiate cleanup
> and try to release the DMA channel that couldn't be retrieved. This will
> cause a crash because the cleanup will try to dereference an ERR_PTR()-
> encoded error code.
> 
> However, there's nothing to clean up at this point yet, so we can avoid
> this by simply propagating the error code.
> 
> Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index f155e9028f94..0eab199900ae 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -458,10 +458,8 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>  	 * with existing devicetrees.
>  	 */
>  	i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx");
> -	if (IS_ERR(i2c_dev->dma_chan)) {
> -		err = PTR_ERR(i2c_dev->dma_chan);
> -		goto err_out;
> -	}
> +	if (IS_ERR(i2c_dev->dma_chan))
> +		return PTR_ERR(i2c_dev->dma_chan);

Actually you are ignoring the case when the driver would use
programmed i/o only as a backup plan.

I think a possible fix could be:

--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -460,7 +460,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
        i2c_dev->dma_chan = dma_request_chan(i2c_dev->dev, "tx");
        if (IS_ERR(i2c_dev->dma_chan)) {
                err = PTR_ERR(i2c_dev->dma_chan);
-               goto err_out;
+               goto err_probe_defer;
        }
 
        i2c_dev->dma_dev = i2c_dev->dma_chan->device->dev;
@@ -482,6 +482,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 
 err_out:
        tegra_i2c_release_dma(i2c_dev);
+err_probe_defer:
        if (err != -EPROBE_DEFER) {
                dev_err(i2c_dev->dev, "cannot use DMA: %d\n", err);
                dev_err(i2c_dev->dev, "falling back to PIO\n");

Thank you,
Andi

(BTW, those are not dev_err's but they should be either dev_info
or dev_warn())

>  
>  	i2c_dev->dma_dev = i2c_dev->dma_chan->device->dev;
>  	i2c_dev->dma_buf_size = i2c_dev->hw->quirks->max_write_len +
> -- 
> 2.40.1
>