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

Andi Shyti
2023-06-08T08:39:13Z

Hi Thierry,

On Thu, Jun 08, 2023 at 09:56:06AM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> 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@nvidia.com>
> ---
> 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
>