| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<AM0PR0402MB3937CD206F229B750EB790E4E8292@AM0PR0402MB3937.eurprd04.prod.outlook.com>
Date: Thu, 28 Nov 2024 06:54:04 +0000
From: Carlos Song <carlos.song@....com>
To: Ahmad Fatoum <a.fatoum@...gutronix.de>, "mkl@...gutronix.de"
<mkl@...gutronix.de>, Frank Li <frank.li@....com>, "o.rempel@...gutronix.de"
<o.rempel@...gutronix.de>, "kernel@...gutronix.de" <kernel@...gutronix.de>,
"andi.shyti@...nel.org" <andi.shyti@...nel.org>, "shawnguo@...nel.org"
<shawnguo@...nel.org>, "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>
CC: "imx@...ts.linux.dev" <imx@...ts.linux.dev>, "linux-i2c@...r.kernel.org"
<linux-i2c@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v4] i2c: imx: support DMA defer probing
> -----Original Message-----
> From: Carlos Song
> Sent: Wednesday, November 27, 2024 6:43 PM
> To: Ahmad Fatoum <a.fatoum@...gutronix.de>; mkl@...gutronix.de; Frank Li
> <frank.li@....com>; o.rempel@...gutronix.de; kernel@...gutronix.de;
> andi.shyti@...nel.org; shawnguo@...nel.org; s.hauer@...gutronix.de;
> festevam@...il.com
> Cc: imx@...ts.linux.dev; linux-i2c@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v4] i2c: imx: support DMA defer probing
>
>
>
> > -----Original Message-----
> > From: Ahmad Fatoum <a.fatoum@...gutronix.de>
> > Sent: Wednesday, November 27, 2024 4:38 PM
> > To: Carlos Song <carlos.song@....com>; mkl@...gutronix.de; Frank Li
> > <frank.li@....com>; o.rempel@...gutronix.de; kernel@...gutronix.de;
> > andi.shyti@...nel.org; shawnguo@...nel.org; s.hauer@...gutronix.de;
> > festevam@...il.com
> > Cc: imx@...ts.linux.dev; linux-i2c@...r.kernel.org;
> > linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
> > Subject: [EXT] Re: [PATCH v4] i2c: imx: support DMA defer probing
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using
> > the 'Report this email' button
> >
> >
> > Hello Carlos,
> >
> > On 27.11.24 09:38, carlos.song@....com wrote:
> > > From: Carlos Song <carlos.song@....com>
> > >
> > > Return -EPROBE_DEFER when dma_request_slave_channel() because DMA
> > > driver have not ready yet.
> > >
> > > Move i2c_imx_dma_request() before registering I2C adapter to avoid
> > > infinite loop of .probe() calls to the same driver, see
> > > "e8c220fac415 Revert "i2c: imx: improve the error handling in
> i2c_imx_dma_request()""
> > > and "Documentation/driver-api/driver-model/driver.rst".
> > >
> > > Use CPU mode to avoid stuck registering i2c adapter when DMA
> > > resources are unavailable.
> >
> > Please try to address open questions before sending new versions of
> > the patch set. Otherwise, it's difficult to follow the conversation.
> >
> > Did you see my question[1] on your v2:
> >
>
> Hi, thank you so much! So sorry about it... I missed it yesterday. I will answer
> your question[1] in this mail.
>
>
> > | Wouldn't this break probe for all i2c-imx users who have
> > | CONFIG_IMX_SDMA disabled?
> > |
>
> I have tested i2c probe at IMX and LS platform when DMA disabled, it won't
> break i2c-imx probe.
> When require DMA channel in i2c_imx_dma_request, find no devices and return
> -ENODEV, as you see at V4 patch, it will continue to probe and work in PIO
> mode.
> I2C adapter should keep available whatever DMA mode is or isn't enabled.
>
> > | Also I am wondering on what kernel version and what configuration
> > | (CONFIG_I2C_IMX=?, CONFIG_IMX_SDMA=?) you have that made you run
> > | into this situation.
> > |
>
> I want to correct something, these code about DMA in i2c-imx.c is for eDMA not
> for SDMA.
> For eDMA mode, I have tested this patch at layerscape-1043 platform. My patch
> is based on
> cfba9f07a1d6 (tag: next-20241122, origin/master, origin/HEAD).
>
> Test log is :
> No apply this patch:
> CONFIG_I2C_IMX=y
> CONFIG_FSL_EDMA=y
> root@...043ardb:~# dmesg | grep i2c
> [ 1.162053] i2c i2c-0: IMX I2C adapter registered
> [ 1.166826] i2c i2c-0: using dma0chan16 (tx) and dma0chan17 (rx) for DMA
> transfers
> [ 4.722057] i2c_dev: i2c /dev entries driver
>
> Not apply the patch:
> CONFIG_I2C_IMX=y
> CONFIG_FSL_EDMA=m
> root@...043ardb:~# dmesg | grep i2c
> [ 1.166381] i2c i2c-0: IMX I2C adapter registered
> [ 4.719226] i2c_dev: i2c /dev entries driver
> (result shows i2c not enabled the eDMA mode) root@...043ardb:~# i2cdetect
> -y -l
> i2c-0 i2c 2180000.i2c I2C
> adapter
> root@...043ardb:~# i2cdetect -y 0
> 0 1 2 3 4 5 6 7 8 9 a b c d e f
> 00: 08 -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- --
> 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- --
> 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
> 70: -- -- -- -- -- -- -- --
>
> After apply the patch:
> CONFIG_I2C_IMX=y
> CONFIG_FSL_EDMA=m
> root@...043ardb:~#
> root@...043ardb:~# dmesg | grep i2c
> [ 4.697046] i2c_dev: i2c /dev entries driver
> [ 7.304142] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17
> (rx) for DMA transfers
> [ 7.313532] i2c i2c-0: IMX I2C adapter registered
> (result shows i2c probed after eDMA module installed) root@...043ardb:~#
> root@...043ardb:~# i2cdetect -y -l
> i2c-0 i2c 2180000.i2c I2C
> adapter
> root@...043ardb:~# i2cdetect -y 0
> 0 1 2 3 4 5 6 7 8 9 a b c d e f
> 00: 08 -- -- -- -- -- -- --
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 40: UU -- -- -- -- -- -- -- -- -- -- -- UU -- -- --
> 50: -- UU UU UU -- -- -- -- -- -- -- -- -- -- -- --
> 60: -- -- -- -- -- -- -- -- -- 69 -- -- -- -- -- --
> 70: -- -- -- -- -- -- -- --
>
>
> > | I'd have expected that with fw_devlink enabled, the I2C controller
> > | wouldn't be probed before the DMA provider is available.
> >
>
> This is a legacy patch, it has been in our local tree for a long time. The related
> history is relatively vague.
> I reproduced the problem and found this patch is effective, so I referred the
> community patch and legacy patch to rewrite the commit log(I am not sure if
> this would happened in some cases so I kept this information).
> Now it seems that these descriptions are redundant. I should completely
> removed this in the commit log:
> Move i2c_imx_dma_request() before registering I2C adapter to avoid
> infinite loop of .probe() calls to the same driver, see "e8c220fac415
> Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
> and "Documentation/driver-api/driver-model/driver.rst".
>
> [1]:
> https://lore.kernel.org/all/19a43db4-db5c-4638-9778-d94fb571a206@pengutr
> onix.de/
> [2]:https://lore.kernel.org/all/153e8e36-7b0e-4379-9cc3-6dacb5d705be@peng
> utronix.de/
>
Hi, Ahmad
I have done more test about following comment in [1]:
----------------
> | I'd have expected that with fw_devlink enabled, the I2C controller
> | wouldn't be probed before the DMA provider is available.
>
This is a legacy patch, it has been in our local tree for a long time. The related history is relatively vague.
I reproduced the problem and found this patch is effective, so I referred the community patch and
legacy patch to rewrite the commit log(I am not sure if this would happened in some cases so I kept this information).
Now it seems that these descriptions are redundant. I should completely removed this in the commit log:
Move i2c_imx_dma_request() before registering I2C adapter to avoid
infinite loop of .probe() calls to the same driver, see "e8c220fac415
Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
and "Documentation/driver-api/driver-model/driver.rst".
----------------
You can refer the link about the history:
[1]: https://lore.kernel.org/all/AM0PR0402MB39370E69BC4B71C761EE8377E8282@AM0PR0402MB3937.eurprd04.prod.outlook.com/
I don't know if I have understanded you rightly. But I am trying your expect.
I create a new case, only add "return defer probe and error in i2c_imx_dma_request()" and
still keep i2c_imx_dma_request() after the adapter register.
The code step like:
ret = i2c_add_numbered_adapter()
...
ret = i2c_imx_dma_request(i2c_imx, phy_addr);
if (ret == -EPROBE_DEFER)
goto delete_adapter;
...
return 0;
delete_adapter:
/* remove adapter */
dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
i2c_del_adapter(&i2c_imx->adapter);
clk_notifier_unregister:
...
Now,
CONFIG_I2C_IMX=y
CONFIG_FSL_EDMA=m
As you expected, if I understand rightly, I think I shouldn't see any i2c probe log until DMA is available.
When I make all i2c devices module installed, i2c still defer probe if DMA is not available.
root@...043ardb:~# dmesg | grep i2c
[ 1.153535] i2c i2c-0: IMX I2C adapter registered
[ 4.705068] i2c_dev: i2c /dev entries driver
[ 4.995118] i2c i2c-0: IMX I2C adapter registered
[ 5.003324] i2c i2c-0: IMX I2C adapter registered
[ 8.079458] i2c i2c-0: IMX I2C adapter registered
[ 8.213536] i2c i2c-0: IMX I2C adapter registered
[ 8.278704] i2c i2c-0: IMX I2C adapter registered
[ 8.283669] i2c i2c-0: using dma1chan16 (tx) and dma1chan17 (rx) for DMA transfers
However when I make some i2c devices build-in, the infinite loop of .probe() calls
will happened:
[ 5.004716] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 5.012526] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 5.021217] pcf85363 0-0051: registered as rtc0
[ 5.027042] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934)
[ 5.035644] i2c i2c-0: IMX I2C adapter registered
[ 5.045357] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 5.053111] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 5.061748] pcf85363 0-0051: registered as rtc0
[ 5.067569] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934)
[ 5.076148] i2c i2c-0: IMX I2C adapter registered
[ 5.097323] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 5.105081] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 5.113585] pcf85363 0-0051: registered as rtc0
[ 5.119402] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934)
[ 5.127958] i2c i2c-0: IMX I2C adapter registered
[ 5.139289] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 5.147014] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 5.155584] pcf85363 0-0051: registered as rtc0
[ 5.161427] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934)
[ 5.169968] i2c i2c-0: IMX I2C adapter registered
[ 5.181278] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 5.189015] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 5.197515] pcf85363 0-0051: registered as rtc0
[ 5.203394] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:14 UTC (1607176934)
[ 5.211930] i2c i2c-0: IMX I2C adapter registered
...
[ 26.405830] i2c i2c-0: IMX I2C adapter registered
[ 26.413765] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 26.421485] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 26.429980] pcf85363 0-0051: registered as rtc0
[ 26.435735] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955)
[ 26.444324] i2c i2c-0: IMX I2C adapter registered
[ 26.452250] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 26.459968] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 26.468395] pcf85363 0-0051: registered as rtc0
[ 26.474233] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955)
[ 26.482774] i2c i2c-0: IMX I2C adapter registered
[ 26.490694] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 26.498405] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 26.506897] pcf85363 0-0051: registered as rtc0
[ 26.512678] pcf85363 0-0051: setting system clock to 2020-12-05T14:02:35 UTC (1607176955)
Is above test not in line with your expectations?(with fw_devlink enabled, I2C controller wouldn't be probed before the DMA provider is available.)
When I apply the V4 patch. I2C adapter can defer probe and whatever i2c devices module installed/build-in, no issue happened. The build-in device
driver also start probing after adapter really registered:
(-------------i2c_imx_dma_request--------------)
[ 7.437482] imx-i2c 2180000.i2c: using dma0chan16 (tx) and dma0chan17 (rx) for DMA transfers
(----------i2c_add_numbered_adapter-------------)
[ 7.446581] ina2xx 0-0040: supply vs not found, using dummy regulator
[ 7.455945] ina2xx 0-0040: power monitor ina220 (Rshunt = 1000 uOhm)
[ 7.465146] pcf85363 0-0051: registered as rtc0
[ 7.471052] pcf85363 0-0051: setting system clock to 2020-12-05T17:54:39 UTC (1607190879)
[ 7.479902] i2c i2c-0: IMX I2C adapter registered
Inevitably, on some boards some i2c device drivers are build-in. The V4 patch will avoid some issue and is safer for i2c-imx user at different platform.
So I think it is necessary to move i2c_imx_dma_request() before registering I2C adapter and I should also keep the log part.
> > >
> > > Signed-off-by: Carlos Song <carlos.song@....com>
> > > Signed-off-by: Clark Wang <xiaoning.wang@....com>
> > > ---
> > > Change for V4:
> > > - Output "Only use PIO mode" log in debug level when no DMA configure.
> > > Change for V3:
> > > - According to Marc's comment, remove error print when defer probe.
> > > Add info log when DMA has not been enabled and add error log when
> > > DMA error, both won't stuck the i2c adapter register, just for reminding,
> > > i2c adapter is working only in PIO mode.
> > > Change for V2:
> > > - According to Frank's comments, wrap at 75 char and Simplify fix code
> > > at i2c_imx_dma_request().
> > > - Use strict patch check, fix alignment warning at
> > > i2c_imx_dma_request()
> > > ---
> > > drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++--------
> > > 1 file changed, 23 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-imx.c
> > > b/drivers/i2c/busses/i2c-imx.c index 5ed4cb61e262..b11d66d56c55
> > > 100644
> > > --- a/drivers/i2c/busses/i2c-imx.c
> > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > @@ -397,17 +397,16 @@ static void i2c_imx_reset_regs(struct
> > > imx_i2c_struct *i2c_imx) }
> > >
> > > /* Functions for DMA support */
> > > -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > > - dma_addr_t
> > phy_addr)
> > > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > > +dma_addr_t phy_addr)
> > > {
> > > struct imx_i2c_dma *dma;
> > > struct dma_slave_config dma_sconfig;
> > > - struct device *dev = &i2c_imx->adapter.dev;
> > > + struct device *dev = i2c_imx->adapter.dev.parent;
> > > int ret;
> > >
> > > dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
> > > if (!dma)
> > > - return;
> > > + return -ENOMEM;
> > >
> > > dma->chan_tx = dma_request_chan(dev, "tx");
> > > if (IS_ERR(dma->chan_tx)) {
> > > @@ -452,7 +451,7 @@ static void i2c_imx_dma_request(struct
> > imx_i2c_struct *i2c_imx,
> > > dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
> > > dma_chan_name(dma->chan_tx),
> > > dma_chan_name(dma->chan_rx));
> > >
> > > - return;
> > > + return 0;
> > >
> > > fail_rx:
> > > dma_release_channel(dma->chan_rx);
> > > @@ -460,6 +459,8 @@ static void i2c_imx_dma_request(struct
> > imx_i2c_struct *i2c_imx,
> > > dma_release_channel(dma->chan_tx);
> > > fail_al:
> > > devm_kfree(dev, dma);
> > > +
> > > + return ret;
> > > }
> > >
> > > static void i2c_imx_dma_callback(void *arg) @@ -1803,6 +1804,23 @@
> > > static int i2c_imx_probe(struct platform_device *pdev)
> > > if (ret == -EPROBE_DEFER)
> > > goto clk_notifier_unregister;
> > >
> > > + /*
> > > + * Init DMA config if supported, -ENODEV means DMA not enabled
> at
> > > + * this platform, that is not a real error, so just remind "only
> > > + * PIO mode is used". If DMA is enabled, but meet error when
> request
> > > + * DMA channel, error should be showed in probe error log. PIO
> mode
> > > + * should be available regardless of DMA.
> > > + */
> > > + ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > > + if (ret) {
> > > + if (ret == -EPROBE_DEFER)
> > > + goto clk_notifier_unregister;
> > > + else if (ret == -ENODEV)
> > > + dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > > + else
> > > + dev_err_probe(&pdev->dev, ret, "Failed to
> > > + setup
> > DMA, only use PIO mode\n");
> > > + }
> > > +
> > > /* Add I2C adapter */
> > > ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
> > > if (ret < 0)
> > > @@ -1817,9 +1835,6 @@ static int i2c_imx_probe(struct
> > > platform_device
> > *pdev)
> > > i2c_imx->adapter.name);
> > > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter
> > > registered\n");
> > >
> > > - /* Init DMA config if supported */
> > > - i2c_imx_dma_request(i2c_imx, phy_addr);
> > > -
> > > return 0; /* Return OK */
> > >
> > > clk_notifier_unregister:
> >
> >
> > --
> > Pengutronix e.K. |
> > |
> > Steuerwalder Str. 21 |
> > http://www.p/
> > en
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C1acf840d499f
> >
> 49a7872408dd0ebedc39%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > %7C638682935131084746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> w
> >
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&s
> >
> data=Y9Qn9XEk15yu4CespwsNu6hl3%2FqfNTvEeOn4ZvnGxbo%3D&reserved=0
> > |
> > 31137 Hildesheim, Germany | Phone:
> +49-5121-206917-0
> > |
> > Amtsgericht Hildesheim, HRA 2686 | Fax:
> > +49-5121-206917-5555 |
Powered by blists - more mailing lists