| 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: <aUO2nEPdSHSAP8lp@tom-desktop>
Date: Thu, 18 Dec 2025 09:09:00 +0100
From: Tommaso Merciai <tommaso.merciai.xr@...renesas.com>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: tomm.merciai@...il.com, linux-renesas-soc@...r.kernel.org,
biju.das.jz@...renesas.com, 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-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [PATCH] i2c: riic: Move suspend handling to NOIRQ phase
Hi Claudiu,
Thanks for your review!
On Wed, Dec 17, 2025 at 11:41:43AM +0200, Claudiu Beznea wrote:
> Hi, Tommaso,
>
> On 12/12/25 13:58, Tommaso Merciai wrote:
> > 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);
>
> Reset assert/de-assert was moved to the RPM callbacks. Is this intended?
> You may want to at least mention it in the commit description.
Ack, thanks.
I will add that into the commit description.
>
> > }
> >
> > -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);
>
> Moving reset assert/de-assert and controller deinit/re-initialization on
> runtime suspend/resume may increase the runtime suspend/resume latency. In
> case of consecutive requests this may translate into controller performance
> downgrade. If you keep it like this, you may want to increase the default
> autosuspend delay, at least, to avoid controller reconfiguration after each
> individual struct i2c_algorithm::xfer() call.
We can set autosuspend delay to 250 ms.
What do you think?
pm_runtime_set_autosuspend_delay(dev, 250);
>
> > 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;
>
> This one could be dropped
>
> > }
> >
> > + return 0;
>
> And use here directly, like:
>
> return ret;
Ack. Thanks.
Will fix that into the next version.
Kind Regards,
Tommaso
>
> Thank you,
> Claudiu
Powered by blists - more mailing lists