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@xxxxxxxxxxxxxxx> # 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@xxxxxxxxxx> Andi > I hope that answers all your questions. > > Thanks, > Thierry