| 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: <aUQJjqO2TuTPdo4W@tom-desktop>
Date: Thu, 18 Dec 2025 15:02:54 +0100
From: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
To: Biju Das <biju.das.jz@...renesas.com>
Cc: Tommaso Merciai <tomm.merciai@...il.com>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
Chris Brandt <Chris.Brandt@...esas.com>,
Andi Shyti <andi.shyti@...nel.org>,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
Hi Biju,
Thanks for your review!
On Thu, Dec 18, 2025 at 09:23:19AM +0000, Biju Das wrote:
> Hi Tommaso,
>
> > -----Original Message-----
> > From: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
> > Sent: 12 December 2025 11:59
> > Subject: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
> >
> > Commit 53326135d0e0 ("i2c: riic: Add suspend/resume support") added suspend support for the Renesas
> > I2C driver and following this change on RZ/G3E the following WARNING is seen on entering suspend ...
> >
> > [ 134.275704] Freezing remaining freezable tasks completed (elapsed 0.001 seconds) [ 134.285536] ---
> > ---------[ cut here ]------------ [ 134.290298] i2c i2c-2: Transfer while suspended [ 134.295174]
> > WARNING: drivers/i2c/i2c-core.h:56 at __i2c_smbus_xfer+0x1e4/0x214, CPU#0: systemd-sleep/388
> > [ 134.365507] Tainted: [W]=WARN [ 134.368485] Hardware name: Renesas SMARC EVK version 2 based on
> > r9a09g047e57 (DT) [ 134.375961] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [ 134.382935] pc : __i2c_smbus_xfer+0x1e4/0x214 [ 134.387329] lr : __i2c_smbus_xfer+0x1e4/0x214
> > [ 134.391717] sp : ffff800083f23860 [ 134.395040] x29: ffff800083f23860 x28: 0000000000000000 x27:
> > ffff800082ed5d60 [ 134.402226] x26: 0000001f4395fd74 x25: 0000000000000007 x24: 0000000000000001
> > [ 134.409408] x23: 0000000000000000 x22: 000000000000006f x21: ffff800083f23936 [ 134.416589] x20:
> > ffff0000c090e140 x19: ffff0000c090e0d0 x18: 0000000000000006 [ 134.423771] x17: 6f63657320313030 x16:
> > 2e30206465737061 x15: ffff800083f23280 [ 134.430953] x14: 0000000000000000 x13: ffff800082b16ce8 x12:
> > 0000000000000f09 [ 134.438134] x11: 0000000000000503 x10: ffff800082b6ece8 x9 : ffff800082b16ce8
> > [ 134.445315] x8 : 00000000ffffefff x7 : ffff800082b6ece8 x6 : 80000000fffff000 [ 134.452495] x5 :
> > 0000000000000504 x4 : 0000000000000000 x3 : 0000000000000000 [ 134.459672] x2 : 0000000000000000 x1 :
> > 0000000000000000 x0 : ffff0000c9ee9e80 [ 134.466851] Call trace:
> > [ 134.469311] __i2c_smbus_xfer+0x1e4/0x214 (P) [ 134.473715] i2c_smbus_xfer+0xbc/0x120
> > [ 134.477507] i2c_smbus_read_byte_data+0x4c/0x84
> > [ 134.482077] isl1208_i2c_read_time+0x44/0x178 [rtc_isl1208] [ 134.487703]
> > isl1208_rtc_read_time+0x14/0x20 [rtc_isl1208] [ 134.493226] __rtc_read_time+0x44/0x88 [ 134.497012]
> > rtc_read_time+0x3c/0x68 [ 134.500622] rtc_suspend+0x9c/0x170
> >
> > The warning is triggered because I2C transfers can still be attempted while the controller is already
> > suspended, due to inappropriate ordering of the system sleep callbacks.
> >
> > Fix this by moving the system sleep suspend/resume callbacks to the NOIRQ phase, ensuring the adapter
> > is fully quiesced after late suspend and properly resumed before the early resume phase.
> >
> > To support NOIRQ resume, the hardware initialization path must not invoke runtime PM APIs. Split the
> > init code so that the low-level hardware setup can be executed without pm_runtime_get/put(). This
> > avoids violating the constraint introduced by commit 1e2ef05bb8cf ("PM: Limit race conditions between
> > runtime PM and system sleep (v2)"), which forbids runtime PM calls during early resume.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 53326135d0e0 ("i2c: riic: Add suspend/resume support")
> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
> > ---
> > drivers/i2c/busses/i2c-riic.c | 65 ++++++++++++++++++++++-------------
> > 1 file changed, 41 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> > 3e8f126cb7f7..9acc8936cdf7 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -349,9 +349,8 @@ static const struct i2c_algorithm riic_algo = {
> > .functionality = riic_func,
> > };
> >
> > -static int riic_init_hw(struct riic_dev *riic)
> > +static int __riic_init_hw(struct riic_dev *riic)
> > {
> > - int ret;
> > unsigned long rate;
> > unsigned long ns_per_tick;
> > int total_ticks, cks, brl, brh;
> > @@ -431,10 +430,6 @@ static int riic_init_hw(struct riic_dev *riic)
> > rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
> > t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
> >
> > - ret = pm_runtime_resume_and_get(dev);
> > - if (ret)
> > - return ret;
> > -
> > /* Changing the order of accessing IICRST and ICE may break things! */
> > riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
> > riic_clear_set_bit(riic, 0, ICCR1_ICE, RIIC_ICCR1); @@ -451,10 +446,25 @@ static int
> > riic_init_hw(struct riic_dev *riic)
> >
> > riic_clear_set_bit(riic, ICCR1_IICRST, 0, RIIC_ICCR1);
> >
> > - pm_runtime_put_autosuspend(dev);
> > return 0;
> > }
> >
> > +static int riic_init_hw(struct riic_dev *riic) {
> > + struct device *dev = riic->adapter.dev.parent;
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __riic_init_hw(riic);
> > +
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + return ret;
> > +}
> > +
> > static int riic_get_scl(struct i2c_adapter *adap) {
> > struct riic_dev *riic = i2c_get_adapdata(adap); @@ -572,6 +582,8 @@ static int
> > riic_i2c_probe(struct platform_device *pdev)
> >
> > i2c_parse_fw_timings(dev, &riic->i2c_t, true);
> >
> > + platform_set_drvdata(pdev, riic);
> > +
> > /* Default 0 to save power. Can be overridden via sysfs for lower latency. */
> > pm_runtime_set_autosuspend_delay(dev, 0);
> > pm_runtime_use_autosuspend(dev);
> > @@ -585,8 +597,6 @@ static int riic_i2c_probe(struct platform_device *pdev)
> > if (ret)
> > goto out;
> >
> > - platform_set_drvdata(pdev, riic);
> > -
> > dev_info(dev, "registered with %dHz bus speed\n", riic->i2c_t.bus_freq_hz);
> > return 0;
> >
> > @@ -668,27 +678,17 @@ static const struct riic_of_data riic_rz_t2h_info = {
> > .num_irqs = ARRAY_SIZE(riic_rzt2h_irqs), };
> >
> > -static int riic_i2c_suspend(struct device *dev)
> > +static int riic_i2c_runtime_suspend(struct device *dev)
> > {
> > struct riic_dev *riic = dev_get_drvdata(dev);
> > - int ret;
> > -
> > - ret = pm_runtime_resume_and_get(dev);
> > - if (ret)
> > - return ret;
> > -
> > - i2c_mark_adapter_suspended(&riic->adapter);
> >
> > /* Disable output on SDA, SCL pins. */
> > riic_clear_set_bit(riic, ICCR1_ICE, 0, RIIC_ICCR1);
> >
> > - pm_runtime_mark_last_busy(dev);
> > - pm_runtime_put_sync(dev);
> > -
> > return reset_control_assert(riic->rstc); }
> >
> > -static int riic_i2c_resume(struct device *dev)
> > +static int riic_i2c_runtime_resume(struct device *dev)
> > {
> > struct riic_dev *riic = dev_get_drvdata(dev);
> > int ret;
> > @@ -697,7 +697,7 @@ static int riic_i2c_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > - ret = riic_init_hw(riic);
> > + ret = __riic_init_hw(riic);
> > if (ret) {
> > /*
> > * In case this happens there is no way to recover from this @@ -708,13 +708,30 @@ static
> > int riic_i2c_resume(struct device *dev)
> > return ret;
> > }
> >
> > + return 0;
> > +}
> > +
> > +static int riic_i2c_suspend(struct device *dev) {
> > + struct riic_dev *riic = dev_get_drvdata(dev);
> > +
> > + i2c_mark_adapter_suspended(&riic->adapter);
> > +
> > + return pm_runtime_force_suspend(dev);
> > +}
> > +
> > +static int riic_i2c_resume(struct device *dev) {
> > + struct riic_dev *riic = dev_get_drvdata(dev);
> > +
> > i2c_mark_adapter_resumed(&riic->adapter);
>
>
> Is it not possible reconfigure i2c controller here like [1]
>
> So that we can avoid reinitializing the i2ccontroller after every transfer (ie, rpm)
>
> [1]
> https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/i2c/busses/i2c-tegra.c#L1965
Thanks for sharing. On my side I found [2] that can be used as reference
also for riic_i2c using:
NOIRQ_SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend_noirq, riic_i2c_resume_noirq)
SYSTEM_SLEEP_PM_OPS(riic_i2c_suspend, riic_i2c_resume)
I'm preparing v2 based on that commit:
4262df2a69c3 ("i2c: imx-lpi2c: make controller available until the system
enters suspend_noirq() and from resume_noirq().")
Kind Regards,
Tommaso
[2] https://elixir.bootlin.com/linux/v6.18-rc7/source/drivers/i2c/busses/i2c-imx-lpi2c.c#L1537
>
> Cheers,
> Biju
>
Powered by blists - more mailing lists