| Message ID | 20230707132619.2998382-1-thierry.reding@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: tegra: Fix failure during probe deferral cleanup | expand |
>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 resetting the DMA channel to NULL instead of storing the >error code. > >Fixes: fcc8a89a1c83 ("i2c: tegra: Share same DMA channel for RX and >TX") >Signed-off-by: Thierry Reding mailto:treding@nvidia.com This fixed the crash issue seen when there is no dmas property in the device tree. Resending this with the correct headers to get it recorded by patchwork. Tested-by: Akhil R <akhilrajeev@nvidia.com>
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. > > 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") > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - simplify patch by setting dma_chan = NULL on channel request failure > > drivers/i2c/busses/i2c-tegra.c | 1 + > 1 file changed, 1 insertion(+) Hi Andy, Wolfram, do you have any reservations about this? This fixes a bug that was introduced in v6.5-rc1, so it'd be great if this could make v6.5. Thanks, Thierry > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index f155e9028f94..2a13f11edfd1 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -460,6 +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); > + i2c_dev->dma_chan = NULL; > goto err_out; > } > > -- > 2.41.0 >
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... > 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+ Thanks, Andi > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - simplify patch by setting dma_chan = NULL on channel request failure > > drivers/i2c/busses/i2c-tegra.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index f155e9028f94..2a13f11edfd1 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -460,6 +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); > + i2c_dev->dma_chan = NULL; > goto err_out; > } > > -- > 2.41.0 >
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
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
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. > > 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") > Signed-off-by: Thierry Reding <treding@nvidia.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index f155e9028f94..2a13f11edfd1 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -460,6 +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); + i2c_dev->dma_chan = NULL; goto err_out; }