linux-kernel - Re: [PATCH v4] i2c: imx: support DMA defer probing

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ff7b4769-245c-4c3b-9b15-b8f2c032d3c1@pengutronix.de>
Date: Wed, 11 Dec 2024 08:06:48 +0100
From: Ahmad Fatoum <a.fatoum@...gutronix.de>
To: Carlos Song <carlos.song@....com>, "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

Hi,

On 06.12.24 10:07, Carlos Song wrote:
>> -----Original Message-----
>> From: Ahmad Fatoum <a.fatoum@...gutronix.de>
>> Sent: Friday, December 6, 2024 2:50 AM
> Refer previous history:
> https://lore.kernel.org/linux-arm-kernel/6b39268b-7487-427d-aff5-f3ca3b2afd42@pengutronix.de/


> Before replying to your question, I think we should synchronize SDMA and eDMA firstly.

[snip]

> SDMA initialization in i2c-imx driver:
> 
> +       dma->chan_tx = dma_request_chan(dev, "rx-tx");
> +       if (IS_ERR(dma->chan_tx)) {
> +               ret = PTR_ERR(dma->chan_tx);
> +               goto fail_dma;
> +       }
> +
> +       dma->chan_rx = dma->chan_tx;
> +       i2c_imx->dma = dma;
> 
> eDMA initialization in i2c-imx driver:
> 
> ...
> +       dma->chan_tx = dma_request_chan(dev, "tx");
> ....
> +       dma->chan_rx = dma_request_chan(dev, "rx");
> ....
>         i2c_imx->dma = dma;

Thanks for the clarification, I missed that.

> So if need to enable SDMA, should define a separate dma_request function and should not reuse the current i2c_imx_dma_request function.
> Also according to different platforms i2c-imx driver need to choose to use eDMA or SDMA.

Understood.

> So now we start to discuss about your confusion:
> 
>> My concern is this configuration:
>>
>>   - A user has eDMA/SDMA module or disabled, but enabled in DT
> [Carlos]:
> I delete edma channel at dts to disable eDMA before. It works in PIO mode.
> I also test your case : when enable dma channel in DT but disable eDMA module. It will meet error:
> 
> +++ b/arch/arm64/configs/defconfig
> ....
> -CONFIG_FSL_EDMA=y
> ....
> 
> root@...043ardb:~# dmesg | grep i2c
> [    4.697392] i2c_dev: i2c /dev entries driver
> [   18.357685] platform 2180000.i2c: deferred probe pending: (reason unknown)
> root@...043ardb:~# i2cdetect -y -l
> 
> The case you mentioned is completely inconsistent with the actual usage. Since you have defined the dma channel in dts, it means that you need to
> use DMA mode in i2c, but you disabled the eDMA module when building the Image. This makes no sense at all. I think this is a usage error.
> And the deferred probe pending error is predictable. Since there is no DMA driver loaded, I2C will keep defer probe and be hanged.

I (or rather the user) didn't knowingly define anything though. They just
used the upstream DT, which always enables the DMA nodes and it worked
fine without DMA support enabled in the config for years.
Now they update the kernel after your patches are merged and then the
I2C driver is no longer being probed.

> 
>>   - The I2C has a PMIC, which is needed for eMMC VCC
> [Carlos]:
> PMIC is critical for the whole board, so PMIC will finish all critical system power-on configuration(include eMMC VCC) in the uboot not at kernel.
> So pmic driver probe in kernel won't and shouldn't effect system boot.
> 
>>   - System startup is stuck or considerably delayed
>>
> The purpose of defer probe is to solve the problem of interdependence of module loading, and to try to load again after a while is to ensure that the required functions
> will not be unavailable because the resources are not ready. This delay is unavoidable. If a defer probe occurs, the first consideration should be to reasonably adjust the
> loading order of each module, rather than directly giving up the functions that should be enabled.

If I2C is a critical dependency, the system becomes unbootable instead of the
user just having to wait a little longer. To me that is a regression.

You clarified though that this is only for Layerscape, so the breakage is
more limited, because e.g. on LS1046A-RDB only EEPROM/sensors/RTC won't be
available and system should still be able to boot.

So while I am not yet convinced an equivalent of this change to be a good
idea for i.MX, which tends to have critical peripherals like PMIC on the
I2C, I don't object to it being done for Layerscape. You may add my:

Acked-by: Ahmad Fatoum <a.fatoum@...gutronix.de>

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |