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

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

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

On Tue, Jul 25, 2023 at 11:34:04PM +0200, Andi Shyti wrote:
> Hi Thierry,
> 
> On Fri, Jul 07, 2023 at 03:26:19PM +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.
> 
> while this is a valid solution I think the best thing here is to
> clear the exit path by adding another goto label.
> 
> By setting dma_chan = NULL you would go through some extra checks
> that are not needed.
> 
> I guess that by doing this we could even remove the
> 
> 	if (i2c_dev->dma_buf)
> 	if (i2c_dev->dma_chan)
> 
> in tegra_i2c_release_dma(). However you see it cleaner. I'm not
> going to be picky, though. Let me know if you are up for some
> more clean up, otherwise I can give you an r-b... after a little
> clarification...

The problem is that DMA support is optional, so we will typically
succeed probe even when the DMA channel cannot be retrieved. The
tegra_i2c_release_dma() is going to get called in any case and if
we were to remove those checks, it would try and release a NULL
buffer and a NULL channel for the non-DMA case.

That's also the reason why we set dma_chan = NULL rather than use
an error label. We could technically skip tegra_i2c_release_dma()
when we fail to get the channel, but we do want to run it when we
fail to allocate the DMA buffer. So that would mean we end up with
two different cleanup paths rather than just one. So overall the
cleanup is simpler if we treat both code paths the same.

> > However, there's nothing to clean up at this point yet, so we can avoid
> > this by simply resetting the DMA channel to NULL instead of storing the
> > error code.
> > 
> > Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and TX")
> 
> ... is this the correct commit that is getting fixed? I think
> it's this one:
> 
> Fixes: 86c92b9965ff ("i2c: tegra: Add DMA support")
> Cc: <stable@vger.kernel.org> # v5.1+

The original DMA support patch didn't have this issue because it was
storing the DMA channel (or error code) in a local variable first and
only assigned it to the i2c_dev->{rx,tx}_dma_channel fields after
checking for errors. Hence, those fields would never end up with an
error code and therefore this wasn't causing any issues previously.

I hope that answers all your questions.

Thanks,
Thierry

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