[PATCH] i2c: sh_mobile: request IRQs after mapping registers

[PATCH] i2c: sh_mobile: request IRQs after mapping registers

[Myeonghun Pak]

The managed IRQ handlers are acquired before the managed register
mapping, so devres unmaps the registers before freeing the IRQs. A
transfer wakes its waiter before the ISR's final register read, and
unregistering the adapter does not synchronize these IRQs. An in-flight
handler can therefore still access the mapping when it is released
during driver detach.

With forced-threaded IRQs, the handler can be preempted after waking the
transfer waiter and before its final ICSR read.

Map the registers before requesting the IRQs so that managed cleanup
frees and synchronizes all handlers before unmapping their registers.
Keep IRQs available while unregistering the adapter because client
removal may perform transfers.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: 7fe8a9993337 ("i2c: sh_mobile: devm conversion, irq setup")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---
Compile-tested with Clang, W=1. No hardware runtime testing.

Based on mainline 0fcb05b72344f40806cbe810de8872f1698bf053.

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
index dae8967..3a55f80 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -880,10 +880,6 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 		return PTR_ERR(pd->clk);
 	}
 
-	ret = sh_mobile_i2c_hook_irqs(dev, pd);
-	if (ret)
-		return ret;
-
 	pd->dev = &dev->dev;
 	platform_set_drvdata(dev, pd);
 
@@ -891,6 +887,10 @@ static int sh_mobile_i2c_probe(struct platform_device *dev)
 	if (IS_ERR(pd->reg))
 		return PTR_ERR(pd->reg);
 
+	ret = sh_mobile_i2c_hook_irqs(dev, pd);
+	if (ret)
+		return ret;
+
 	ret = of_property_read_u32(dev->dev.of_node, "clock-frequency", &bus_speed);
 	pd->bus_speed = (ret || !bus_speed) ? I2C_MAX_STANDARD_MODE_FREQ : bus_speed;
 	pd->clks_per_count = 1;

Re: [PATCH] i2c: sh_mobile: request IRQs after mapping registers

[Andi Shyti]

Hi Myeounghun,

On Sat, Oct 17, 2026 at 11:35:16PM -0400, Myeonghun Pak wrote:
> The managed IRQ handlers are acquired before the managed register
> mapping, so devres unmaps the registers before freeing the IRQs. A
> transfer wakes its waiter before the ISR's final register read, and
> unregistering the adapter does not synchronize these IRQs. An in-flight
> handler can therefore still access the mapping when it is released
> during driver detach.
> 
> With forced-threaded IRQs, the handler can be preempted after waking the
> transfer waiter and before its final ICSR read.
> 
> Map the registers before requesting the IRQs so that managed cleanup
> frees and synchronizes all handlers before unmapping their registers.
> Keep IRQs available while unregistering the adapter because client
> removal may perform transfers.
> 
> This issue was identified as part of our ongoing research while
> reviewing kernel code.
> 
> Fixes: 7fe8a9993337 ("i2c: sh_mobile: devm conversion, irq setup")

Normally, I don't like taking fixes that don't address real
issues, especially speculative AI-generated fixes.

However, this looks like a harmless reordering that seems
correct, although the issue it addresses is rather
hypothetical.

I'll remove the Fixes: tag, since this is purely speculative,
and apply the patch to i2c/i2c.

Thanks,
Andi

Re: [PATCH] i2c: sh_mobile: request IRQs after mapping registers

[Geert Uytterhoeven]

On Sat, 24 Oct 2026 at 00:09, Andi Shyti <andi.shyti@kernel.org> wrote:
> On Sat, Oct 17, 2026 at 11:35:16PM -0400, Myeonghun Pak wrote:
> > The managed IRQ handlers are acquired before the managed register
> > mapping, so devres unmaps the registers before freeing the IRQs. A
> > transfer wakes its waiter before the ISR's final register read, and
> > unregistering the adapter does not synchronize these IRQs. An in-flight
> > handler can therefore still access the mapping when it is released
> > during driver detach.
> >
> > With forced-threaded IRQs, the handler can be preempted after waking the
> > transfer waiter and before its final ICSR read.
> >
> > Map the registers before requesting the IRQs so that managed cleanup
> > frees and synchronizes all handlers before unmapping their registers.
> > Keep IRQs available while unregistering the adapter because client
> > removal may perform transfers.
> >
> > This issue was identified as part of our ongoing research while
> > reviewing kernel code.
> >
> > Fixes: 7fe8a9993337 ("i2c: sh_mobile: devm conversion, irq setup")
>
> Normally, I don't like taking fixes that don't address real
> issues, especially speculative AI-generated fixes.
>
> However, this looks like a harmless reordering that seems
> correct, although the issue it addresses is rather
> hypothetical.
>
> I'll remove the Fixes: tag, since this is purely speculative,
> and apply the patch to i2c/i2c.

FWIW, no issues seen when boot-testing several platforms using
this driver.
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] i2c: sh_mobile: request IRQs after mapping registers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Sat, Oct 17, 2026 at 11:35:16PM -0400, Myeonghun Pak wrote:
> The managed IRQ handlers are acquired before the managed register
> mapping, so devres unmaps the registers before freeing the IRQs. A
> transfer wakes its waiter before the ISR's final register read, and
> unregistering the adapter does not synchronize these IRQs. An in-flight
> handler can therefore still access the mapping when it is released
> during driver detach.
> 
> With forced-threaded IRQs, the handler can be preempted after waking the
> transfer waiter and before its final ICSR read.
> 
> Map the registers before requesting the IRQs so that managed cleanup
> frees and synchronizes all handlers before unmapping their registers.
> Keep IRQs available while unregistering the adapter because client
> removal may perform transfers.
> 
> This issue was identified as part of our ongoing research while
> reviewing kernel code.
> 
> Fixes: 7fe8a9993337 ("i2c: sh_mobile: devm conversion, irq setup")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

I know I am late to the game, but I still want to say that while
changing the order to be "regs then irqs" is in general a good thing, it
won't matter here, as I see it. Unless the later pm_runtime_get_sync()
the device should not(tm) be able to send irqs. Famous last words...

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: sh_mobile: request IRQs after mapping registers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]


> won't matter here, as I see it. Unless the later pm_runtime_get_sync()
> the device should not(tm) be able to send irqs. Famous last words...

Which only matters during probe(), not during teardown... sorry for the
noise! Patch is still good.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]