| 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: <aSXZ8LntcvsvpKSZ@lizhi-Precision-Tower-5810>
Date: Tue, 25 Nov 2025 11:31:44 -0500
From: Frank Li <Frank.li@....com>
To: Carlos Song <carlos.song@....com>
Cc: aisheng.dong@....com, andi.shyti@...nel.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
linux-i2c@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c: imx-lpi2c: Add runtime PM support for IRQ and
clock management on i.MX8QXP/8QM
On Tue, Nov 25, 2025 at 04:47:18PM +0800, Carlos Song wrote:
> On i.MX8QXP/8QM SoCs, both the lvds/mipi and lvds/mipi-lpi2c power domains
> must enter low-power mode during runtime suspend to achieve deep power
> savings.
>
> LPI2C resides in the lvds-lpi2c/mipi-lpi2c power domain, while its IRQ is
> routed through an irqsteer located in the lvds/mipi power domain. The LPI2C
> clock source comes from an LPCG within the lvds-lpi2c domain.
>
> For example, the hierarchy for lvds0 and lvds0-lpi2c0 domains is:
> ┌───────────────────────┐
> │ pm-domain : lvds0 │
> │ │
> │ ┌──────────────┐ │
> │ │ irqsteer │ │
> │ └───────▲──────┘ │
> │ │irq │
> │ │ │
> └────────────┼──────────┘
> ┌────────────┼──────────┐
> │ ┌───┼───┐ │
> │ │lpi2c0 │ │
> │ └───┬───┘clk │
> │ ┌────────┼───────┐ │
> │ │ LPCG │ │
> │ └────────────────┘ │
> │pm-domain:lvds0-lpi2c0 │
> └───────────────────────┘
>
> To allow these domains to power down in system runtime suspend:
>
> - All irqsteer clients must release IRQs.
> - All LPCG clients must disable and unprepare clocks.
>
> Thus, LPI2C must:
>
> - Free its IRQ during runtime suspend and re-request it on resume.
> - Disable and unprepare all clocks during runtime suspend and prepare
> and rne ble them on resume.
>
> This enables the lvds/mipi domains to enter deep low-power mode,
> significantly reducing power consumption compared to active mode.
>
> Signed-off-by: Carlos Song <carlos.song@....com>
> ---
> Changes since V1:
> * Add unit for I2C_PM_LONG_TIMEOUT to I2C_PM_LONG_TIMEOUT_MS
> * Reuqest lpi2c_imx->irq uncondtionally
> * Remove help function and direct put code in original
> lpi2c_runtime_suspend() and lpi2c_runtime_resume()
> * Give more comments to explain why prolong PM timeout
> ---
> 1. Why not apply prepare and unprepare clocks management for all platforms:
> As the report from me early:
> https://lists.openwall.net/linux-kernel/2025/07/01/139
> Scope of global prepare_lock is too big, it will cause dead clock
> between RPM and prepare_lock in some specail case. But clock
> prepare/unprepare is also necessary for low power consumption in I.MX8QXP
> and 8QM, so I add separate clock management for these platforms to
> avoid impacting other I.MX platforms. But it's possible that some
> customers might encounter deadlock issues in IMX8QXP/8QM, so I prolong the
> runtime PM timeout for 8QXP/QM platforms, which is currently a suitable
> workaround method I think.
>
> The dead lock happen as below call stacks
>
> Task 117 Task 120
>
> schedule()
> clk_prepare_lock()--> wait prepare_lock(mutex_lock) schedule() wait for power.runtime_status exit RPM_SUSPENDING
> ^^^^ A ^^^^ B
> clk_bulk_unprepare() rpm_resume()
> lpi2c_runtime_suspend() pm_runtime_resume_and_get()
> ... lpi2c_imx_xfer()
> ...
> rpm_suspend() set RPM_SUSPENDING pcf857x_set();
> ^^^^ B ...
> clk_prepare_lock() --> hold prepare_lock
> ^^^^ A
> ...
>
> Task 117 set power.runtime_status to RPM_SUSPENDING (A) and wait for task 120 release clock's global prepare mutex (B).
>
> Task 120 hold global prepare mutex (B) and wait for power.runtime_status finish suspend (A).
>
> So if RPM doesn't enter auto suspend too quick after hold prepare lock, this dead lock can be avoided.
> So I prolong the runtime PM timeout, it can ensure that LPI2C does not enter auto suspend mode too
> frequently. It has been verified valid for the above case.
>
> 2. Low power status report
> Power domain status can be shown in pm_genpd_summary and sc firmware. Take
> 8QM platform lvds0 and lvds0-lpi2c0 power domain as example, before apply
> this patch set:
> root@...8qmmek:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain status children performance
> /device runtime status managed by
> ---------------------------------------------------------------------------
> lvds0-lpi2c0 on 0
> lvds0_i2c0_clk active 0 SW
> 56243014.clock-controller active 0 SW
> 56247000.i2c suspended 0 SW
> lvds0 on 0
> lvds0_bypass_clk suspended 0 SW
> lvds0_pixel_clk suspended 0 SW
> lvds0_phy_clk suspended 0 SW
> 56240000.interrupt-controller active 0 SW
> sc firmware will show the power domain status:
> >$ power.r
> LVDS_0 = on
> LVDS_0_I2C_0 = on
>
> After apply this patch set:
> root@...8qmmek:~# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain status children performance
> /device runtime status managed by
> ---------------------------------------------------------------------------
> lvds0-lpi2c0 off-0 0
> lvds0_i2c0_clk suspended 0 SW
> 56243014.clock-controller suspended 0 SW
> 56247000.i2c suspended 0 SW
> lvds0 off-0 0
> lvds0_bypass_clk suspended 0 SW
> lvds0_pixel_clk suspended 0 SW
> lvds0_phy_clk suspended 0 SW
> 56240000.interrupt-controller suspended 0 SW
>
> sc firmware will show the power domain status:
> >$ power.r
> LVDS_0 = lp
> LVDS_0_I2C_0 = lp
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 84 +++++++++++++++++++++++++-----
> 1 file changed, 71 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index dfacb0aec3c0..41ad82595583 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -132,6 +132,7 @@
> #define CHUNK_DATA 256
>
> #define I2C_PM_TIMEOUT 10 /* ms */
> +#define I2C_PM_LONG_TIMEOUT_MS 1000 /* Avoid dead lock caused by big clock prepare lock */
Mark:
I don't know where is best place to record clock dead lock issue
with runtime PM in this patch. There are some thread disucssion this
https://lore.kernel.org/imx/20250707-careful-pragmatic-quail-e1a2d8-mkl@pengutronix.de/
https://lore.kernel.org/all/20230421-kinfolk-glancing-e185fd9c47b4-mkl@pengutronix.de/
https://lore.kernel.org/all/20250326-cross-lock-dep-v1-0-3199e49e8652@bootlin.com/
Anyways, I am fine with this patch. It takes long time to resolve
deadlock problem.
Reviewed-by: Frank Li <Frank.Li@....com>
> #define I2C_DMA_THRESHOLD 8 /* bytes */
>
> enum lpi2c_imx_mode {
> @@ -149,6 +150,11 @@ enum lpi2c_imx_pincfg {
> FOUR_PIN_PP,
> };
>
...
>
> i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> platform_set_drvdata(pdev, lpi2c_imx);
> @@ -1493,7 +1520,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> return dev_err_probe(&pdev->dev, -EINVAL,
> "can't get I2C peripheral clock rate\n");
>
> - pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> + if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
> + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
> + else
> + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> +
> pm_runtime_use_autosuspend(&pdev->dev);
> pm_runtime_get_noresume(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
...
>
> return 0;
> --
> 2.34.1
>
Powered by blists - more mailing lists