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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]

On Thu, Jun 08, 2023 at 10:39:05AM +0200, Andi Shyti wrote:
> 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.

Good point.

> 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");

I think it's even simpler. I'll send out a v2 that simply resets
i2c_dev->dma_chan to NULL when an error occurs. That way any subsequent
code will already know to ignore it. Another case where this could
happen is during regular cleanup (i.e. driver unbind), and resetting to
NULL should fix that as well.

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

Yeah. I can send up a follow-up patch to change these to dev_warn(). I
think dev_info() is perhaps a bit too weak, since we really do want to
use DMA if at all possible. Any case where it can't be used would be a
configuration problem. I think I recall that we only have this in the
driver for backwards-compatibility with old device trees that may not
have had the DMA channels specified, so this serves as a reminder that
the DT should be updated.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]