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

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

[Thierry Reding]

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(+)

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

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

[Akhil R]

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

Tested-by: Akhil R <akhilrajeev@nvidia.com>

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

[Shanker Donthineni]

On 7/17/23 12:15, Akhil R wrote:
>> 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.
> 
> Tested-by: Akhil R <akhilrajeev@nvidia.com>

The kernel crash issue has been fixed with ACPI based kernel on T241 (Server) platform.

Tested-by: Shanker Donthineni <sdonthineni@nvidia.com>

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

[Akhil R]

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

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

[Thierry Reding]

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

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
> 

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

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

[Andi Shyti]

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
> 

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

[Thierry Reding]

[-- 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 --]

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

[Andi Shyti]

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

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

[Wolfram Sang]

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

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!


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