| Message ID | fcfcf9b3-c8c4-9b34-2ff8-cd60a3d490bd@connecttech.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | i2c: tegra: Fix i2c-tegra DMA config option processing | expand |
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
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
> > 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
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
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
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
> > 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!
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!
> > > 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>
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.
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
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;
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(-)