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

[Andi Shyti <andi.shyti@kernel.org>]

Hi Thierry,

> > > 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.

that's indeed an easy one-liner fix... that's why I proposed my
r-b in the earlier mail.

> > > 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.

Yes, you are right! the patch commit you mentioned is actually
releasing the channel by checking i2c_dev->dma_chan which might
store the error number and therefore is not NULL.

Thanks for the clarification!

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

> I hope that answers all your questions.
> 
> Thanks,
> Thierry