[PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Parker Newman]

This patch fixes the Tegra DMA config option processing in the
i2c-tegra driver.

Tegra processors prior to Tegra186 used APB DMA for I2C requiring
CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
CONFIG_TEGRA186_GPC_DMA=y.

The check for if the processor uses APB DMA is inverted and so the wrong
DMA config options are checked.

This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
with a Tegra186 or later processor the driver will incorrectly think DMA is
enabled and attempt to request DMA channels that will never be availible,
leaving the driver in a perpetual EPROBE_DEFER state.

Signed-off-by: Parker Newman <pnewman@connecttech.com>
---
 drivers/i2c/busses/i2c-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index bcbbf23aa530..dc6ed3a8d69e 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -442,7 +442,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
    if (IS_VI(i2c_dev))
        return 0;

-   if (!i2c_dev->hw->has_apb_dma) {
+   if (i2c_dev->hw->has_apb_dma) {
        if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
            dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
            return 0;
-- 
2.17.1

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Andi Shyti]

Hi Laxman and/or Dmitry,

On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
> 
> This patch fixes the Tegra DMA config option processing in the
> i2c-tegra driver.
> 
> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
> CONFIG_TEGRA186_GPC_DMA=y.
> 
> The check for if the processor uses APB DMA is inverted and so the wrong
> DMA config options are checked.
> 
> This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
> with a Tegra186 or later processor the driver will incorrectly think DMA is
> enabled and attempt to request DMA channels that will never be availible,
> leaving the driver in a perpetual EPROBE_DEFER state.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index bcbbf23aa530..dc6ed3a8d69e 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -442,7 +442,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>     if (IS_VI(i2c_dev))
>         return 0;
> 
> -   if (!i2c_dev->hw->has_apb_dma) {
> +   if (i2c_dev->hw->has_apb_dma) {
>         if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
>             dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
>             return 0;

Can I have your opinion here, please?

Andi

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Andi Shyti]

BTW...

On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
> 

you have a blank line here.

> This patch fixes the Tegra DMA config option processing in the
> i2c-tegra driver.
> 
> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
> CONFIG_TEGRA186_GPC_DMA=y.
> 
> The check for if the processor uses APB DMA is inverted and so the wrong
> DMA config options are checked.
> 
> This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
> with a Tegra186 or later processor the driver will incorrectly think DMA is
> enabled and attempt to request DMA channels that will never be availible,
> leaving the driver in a perpetual EPROBE_DEFER state.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>

As this is a fix you also need to add

Fixes: 48cb6356fae1 ("i2c: tegra: Add GPCDMA support")
Cc: Akhil R <akhilrajeev@nvidia.com>
Cc: <stable@vger.kernel.org> # v6.1+

Cc'eing Akhil as well for his opinion on this.

Andi

RE: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Akhil R]

> 
> BTW...
> 
> On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
> >
> 
> you have a blank line here.
> 
> > This patch fixes the Tegra DMA config option processing in the
> > i2c-tegra driver.
> >
> > Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> > CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA
> > requiring CONFIG_TEGRA186_GPC_DMA=y.
> >
> > The check for if the processor uses APB DMA is inverted and so the
> > wrong DMA config options are checked.
> >
> > This means if CONFIG_TEGRA20_APB_DMA=y but
> CONFIG_TEGRA186_GPC_DMA=n
> > with a Tegra186 or later processor the driver will incorrectly think
> > DMA is enabled and attempt to request DMA channels that will never be
> > availible, leaving the driver in a perpetual EPROBE_DEFER state.
> >
> > Signed-off-by: Parker Newman <pnewman@connecttech.com>
> 
> As this is a fix you also need to add
> 
> Fixes: 48cb6356fae1 ("i2c: tegra: Add GPCDMA support")
> Cc: Akhil R <akhilrajeev@nvidia.com>
> Cc: <stable@vger.kernel.org> # v6.1+
> 
> Cc'eing Akhil as well for his opinion on this.
The fix looks valid to me. Must have been a typo there.

Regards,
Akhil

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Parker Newman]

On 2023-08-06 10:21, Akhil R wrote:
>>
>> BTW...
>>
>> On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
>>>
>>
>> you have a blank line here.
>>
>>> This patch fixes the Tegra DMA config option processing in the
>>> i2c-tegra driver.
>>>
>>> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
>>> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA
>>> requiring CONFIG_TEGRA186_GPC_DMA=y.
>>>
>>> The check for if the processor uses APB DMA is inverted and so the
>>> wrong DMA config options are checked.
>>>
>>> This means if CONFIG_TEGRA20_APB_DMA=y but
>> CONFIG_TEGRA186_GPC_DMA=n
>>> with a Tegra186 or later processor the driver will incorrectly think
>>> DMA is enabled and attempt to request DMA channels that will never be
>>> availible, leaving the driver in a perpetual EPROBE_DEFER state.
>>>
>>> Signed-off-by: Parker Newman <pnewman@connecttech.com>
>>
>> As this is a fix you also need to add
>>
>> Fixes: 48cb6356fae1 ("i2c: tegra: Add GPCDMA support")
>> Cc: Akhil R <akhilrajeev@nvidia.com>
>> Cc: <stable@vger.kernel.org> # v6.1+
>>
>> Cc'eing Akhil as well for his opinion on this.
> The fix looks valid to me. Must have been a typo there.
> 
> Regards,
> Akhil
> 
Yes it appears to be a simple typo and if you have both DMA Config options 
set the bug would get missed. 

I am new to the Linux mailing list, should I send a new message to 
stable@vger.kernel.org or CC them on this one?

Thanks,
Parker

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Andi Shyti]

Hi Parker,

On Tue, Aug 08, 2023 at 12:42:28PM +0000, Parker Newman wrote:
> On 2023-08-06 10:21, Akhil R wrote:
> >>
> >> BTW...
> >>
> >> On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
> >>>
> >>
> >> you have a blank line here.
> >>
> >>> This patch fixes the Tegra DMA config option processing in the
> >>> i2c-tegra driver.
> >>>
> >>> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> >>> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA
> >>> requiring CONFIG_TEGRA186_GPC_DMA=y.
> >>>
> >>> The check for if the processor uses APB DMA is inverted and so the
> >>> wrong DMA config options are checked.
> >>>
> >>> This means if CONFIG_TEGRA20_APB_DMA=y but
> >> CONFIG_TEGRA186_GPC_DMA=n
> >>> with a Tegra186 or later processor the driver will incorrectly think
> >>> DMA is enabled and attempt to request DMA channels that will never be
> >>> availible, leaving the driver in a perpetual EPROBE_DEFER state.
> >>>
> >>> Signed-off-by: Parker Newman <pnewman@connecttech.com>
> >>
> >> As this is a fix you also need to add
> >>
> >> Fixes: 48cb6356fae1 ("i2c: tegra: Add GPCDMA support")
> >> Cc: Akhil R <akhilrajeev@nvidia.com>
> >> Cc: <stable@vger.kernel.org> # v6.1+
> >>
> >> Cc'eing Akhil as well for his opinion on this.
> > The fix looks valid to me. Must have been a typo there.
> > 
> > Regards,
> > Akhil
> > 
> Yes it appears to be a simple typo and if you have both DMA Config options 
> set the bug would get missed. 
> 
> I am new to the Linux mailing list, should I send a new message to 
> stable@vger.kernel.org or CC them on this one?

First of all... welcome to the Linux mailing list :)

No need to resend, I can add it... I was hoping for an official
ack, but as that's not coming, I will ack it

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

Thank you Parker and wishing to see more patches from you ;)

Andi

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Andi Shyti]

Hi

On Thu, 03 Aug 2023 17:10:02 +0000, Parker Newman wrote:
> This patch fixes the Tegra DMA config option processing in the
> i2c-tegra driver.
> 
> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
> CONFIG_TEGRA186_GPC_DMA=y.
> 
> [...]

With the Fixes tag added, applied to i2c/andi-for-current on

https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Please note that this patch may still undergo further evaluation
and the final decision will be made in collaboration with
Wolfram.

Thank you,
Andi

Patches applied
===============
[1/1] i2c: tegra: Fix i2c-tegra DMA config option processing
      commit: fc9a464f3d9a2a361e8bcb960345270a9dc46054

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Wolfram Sang]

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


> > Cc'eing Akhil as well for his opinion on this.
> The fix looks valid to me. Must have been a typo there.

I'll count this as an ack. Thanks!


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

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Wolfram Sang]

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

On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
> 
> This patch fixes the Tegra DMA config option processing in the
> i2c-tegra driver.
> 
> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
> CONFIG_TEGRA186_GPC_DMA=y.
> 
> The check for if the processor uses APB DMA is inverted and so the wrong
> DMA config options are checked.
> 
> This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
> with a Tegra186 or later processor the driver will incorrectly think DMA is
> enabled and attempt to request DMA channels that will never be availible,
> leaving the driver in a perpetual EPROBE_DEFER state.
> 
> Signed-off-by: Parker Newman <pnewman@connecttech.com>

Applied to for-current, thanks!


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

RE: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Akhil R]

> > > Cc'eing Akhil as well for his opinion on this.
> > The fix looks valid to me. Must have been a typo there.
> 
> I'll count this as an ack. Thanks!
Oh Sorry. I just realized I didn't put an ack.

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

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Dmitry Osipenko]

05.08.2023 00:49, Andi Shyti пишет:
> Hi Laxman and/or Dmitry,
> 
> On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
>>
>> This patch fixes the Tegra DMA config option processing in the
>> i2c-tegra driver.
>>
>> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
>> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
>> CONFIG_TEGRA186_GPC_DMA=y.
>>
>> The check for if the processor uses APB DMA is inverted and so the wrong
>> DMA config options are checked.
>>
>> This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
>> with a Tegra186 or later processor the driver will incorrectly think DMA is
>> enabled and attempt to request DMA channels that will never be availible,
>> leaving the driver in a perpetual EPROBE_DEFER state.
>>
>> Signed-off-by: Parker Newman <pnewman@connecttech.com>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index bcbbf23aa530..dc6ed3a8d69e 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -442,7 +442,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>>     if (IS_VI(i2c_dev))
>>         return 0;
>>
>> -   if (!i2c_dev->hw->has_apb_dma) {
>> +   if (i2c_dev->hw->has_apb_dma) {
>>         if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
>>             dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
>>             return 0;
> 
> Can I have your opinion here, please?

The patch looks good, thanks Parker for fixing it. I'll be able to test
it only sometime later and let you all know if there will be any
problem. Previously I haven't noticed any Tegra I2C regressions, maybe
we should change that dev_dbg to dev_warn.

Re: [PATCH] i2c: tegra: Fix i2c-tegra DMA config option processing

[Parker Newman]

On 2023-08-14 11:25, Dmitry Osipenko wrote:
> 05.08.2023 00:49, Andi Shyti пишет:
>> Hi Laxman and/or Dmitry,
>>
>> On Thu, Aug 03, 2023 at 05:10:02PM +0000, Parker Newman wrote:
>>>
>>> This patch fixes the Tegra DMA config option processing in the
>>> i2c-tegra driver.
>>>
>>> Tegra processors prior to Tegra186 used APB DMA for I2C requiring
>>> CONFIG_TEGRA20_APB_DMA=y while Tegra186 and later use GPC DMA requiring
>>> CONFIG_TEGRA186_GPC_DMA=y.
>>>
>>> The check for if the processor uses APB DMA is inverted and so the wrong
>>> DMA config options are checked.
>>>
>>> This means if CONFIG_TEGRA20_APB_DMA=y but CONFIG_TEGRA186_GPC_DMA=n
>>> with a Tegra186 or later processor the driver will incorrectly think DMA is
>>> enabled and attempt to request DMA channels that will never be availible,
>>> leaving the driver in a perpetual EPROBE_DEFER state.
>>>
>>> Signed-off-by: Parker Newman <pnewman@connecttech.com>
>>> ---
>>>  drivers/i2c/busses/i2c-tegra.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>> index bcbbf23aa530..dc6ed3a8d69e 100644
>>> --- a/drivers/i2c/busses/i2c-tegra.c
>>> +++ b/drivers/i2c/busses/i2c-tegra.c
>>> @@ -442,7 +442,7 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
>>>     if (IS_VI(i2c_dev))
>>>         return 0;
>>>
>>> -   if (!i2c_dev->hw->has_apb_dma) {
>>> +   if (i2c_dev->hw->has_apb_dma) {
>>>         if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) {
>>>             dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
>>>             return 0;
>>
>> Can I have your opinion here, please?
> 
> The patch looks good, thanks Parker for fixing it. I'll be able to test
> it only sometime later and let you all know if there will be any
> problem. Previously I haven't noticed any Tegra I2C regressions, maybe
> we should change that dev_dbg to dev_warn.
> 

Hi Dmitry,

You will not notice any issues if both options are set (or not set) as it 
will fall through and configure the DMA or skip DMA setup as expected. 

I only noticed the issue after I enabled CONFIG_TEGRA20_APB_DMA which is a 
KConfig requirement for the Tegra HS-UART driver and my I2C stopped working. 

Which now as I write this I realize is possibly another "bug"... As far as I know
the HS UARTs also use GPC DMA on T186 or later? I would need to look into that.

I don't think the print needs to be a warning, not having DMA enabled
is a valid option, it just needed the correct CONFIG options to be checked for 
the DMA type (APB versus GPC).

-Parker