[BUG] exynos5_i2c_xfer_atomic() can sleep.

[BUG] exynos5_i2c_xfer_atomic() can sleep.

[Sebastian Andrzej Siewior]

Hi,

While looking into the xfer_atomic i2c thingy, I've been looking at the
exynos5_i2c_xfer_atomic() function and noticed that it has a
disable_irq() invocation. Given that i2c_algorithm::xfer_atomic is
called from atomic context (with either preemption or interrupts
disabled) the might_sleep() in disable_irq() must lead to splat here.

This looks very light tested because exynos5_i2c_poll_irqs_timeout()
which should be invoked from atomic context has usleep_range(). This
will schedule from atomic context is not good. I *think* this gets back
with enabled interrupts.

Sebastian

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Marek Szyprowski]

On 06.05.2026 08:51, Sebastian Andrzej Siewior wrote:
> While looking into the xfer_atomic i2c thingy, I've been looking at the
> exynos5_i2c_xfer_atomic() function and noticed that it has a
> disable_irq() invocation. Given that i2c_algorithm::xfer_atomic is
> called from atomic context (with either preemption or interrupts
> disabled) the might_sleep() in disable_irq() must lead to splat here.

Its me, who added that code, commit 445094c8a9fb1. I've tested that in the reboot/shutdown path and I'm quite sure that I've copied that irq_disable/enable from some other driver. I also cannot locate that might_sleep() in disable_irq(), where exactly it is?


> This looks very light tested because exynos5_i2c_poll_irqs_timeout()
> which should be invoked from atomic context has usleep_range(). This
> will schedule from atomic context is not good. I *think* this gets back
> with enabled interrupts.

Indeed this should be replaced with udelay.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Sebastian Andrzej Siewior]

On 2026-05-08 11:45:15 [+0200], Marek Szyprowski wrote:
> On 06.05.2026 08:51, Sebastian Andrzej Siewior wrote:
> > While looking into the xfer_atomic i2c thingy, I've been looking at the
> > exynos5_i2c_xfer_atomic() function and noticed that it has a
> > disable_irq() invocation. Given that i2c_algorithm::xfer_atomic is
> > called from atomic context (with either preemption or interrupts
> > disabled) the might_sleep() in disable_irq() must lead to splat here.
> 
> Its me, who added that code, commit 445094c8a9fb1. I've tested that in
> the reboot/shutdown path and I'm quite sure that I've copied that
> irq_disable/enable from some other driver. I also cannot locate that
> might_sleep() in disable_irq(), where exactly it is?

kernel/irq/manage.c:
| void disable_irq(unsigned int irq)
| {
|         might_sleep();
|         if (!__disable_irq_nosync(irq))
^^
|                 synchronize_irq(irq);
| }
| EXPORT_SYMBOL(disable_irq);

Sebastian

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Marek Szyprowski]

On 08.05.2026 12:09, Sebastian Andrzej Siewior wrote:
> On 2026-05-08 11:45:15 [+0200], Marek Szyprowski wrote:
>> On 06.05.2026 08:51, Sebastian Andrzej Siewior wrote:
>>> While looking into the xfer_atomic i2c thingy, I've been looking at the
>>> exynos5_i2c_xfer_atomic() function and noticed that it has a
>>> disable_irq() invocation. Given that i2c_algorithm::xfer_atomic is
>>> called from atomic context (with either preemption or interrupts
>>> disabled) the might_sleep() in disable_irq() must lead to splat here.
>> Its me, who added that code, commit 445094c8a9fb1. I've tested that in
>> the reboot/shutdown path and I'm quite sure that I've copied that
>> irq_disable/enable from some other driver. I also cannot locate that
>> might_sleep() in disable_irq(), where exactly it is?
> kernel/irq/manage.c:
> | void disable_irq(unsigned int irq)
> | {
> |         might_sleep();
> |         if (!__disable_irq_nosync(irq))
> ^^
> |                 synchronize_irq(irq);
> | }
> | EXPORT_SYMBOL(disable_irq);

Ah, it looks that I've checked irq_disable() code, so that's why I didn't find might_sleep() call. In the case of exynos5_i2c driver, probably switching to disable_irq_nosync() will be enough. I assume that all previous transfers have to be finished to start this atomic one, so waiting for interrupts to finish is not needed.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Sebastian Andrzej Siewior]

On 2026-05-08 12:35:51 [+0200], Marek Szyprowski wrote:
> Ah, it looks that I've checked irq_disable() code, so that's why I
> didn't find might_sleep() call. In the case of exynos5_i2c driver,
> probably switching to disable_irq_nosync() will be enough. I assume
> that all previous transfers have to be finished to start this atomic
> one, so waiting for interrupts to finish is not needed.

This looks kind of odd. Are the "other" transfers really done at this
point? Do you have a backtrace for me from the atomic path?
Is this needed because you need to send the "power off" command via i2c?

Could you try to test with PREEMPT_RT? I'm sure how this works there.

> Best regards

Sebastian

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Marek Szyprowski]

On 08.05.2026 12:42, Sebastian Andrzej Siewior wrote:
> On 2026-05-08 12:35:51 [+0200], Marek Szyprowski wrote:
>> Ah, it looks that I've checked irq_disable() code, so that's why I
>> didn't find might_sleep() call. In the case of exynos5_i2c driver,
>> probably switching to disable_irq_nosync() will be enough. I assume
>> that all previous transfers have to be finished to start this atomic
>> one, so waiting for interrupts to finish is not needed.
> This looks kind of odd. Are the "other" transfers really done at this
> point? Do you have a backtrace for me from the atomic path?

I thought that i2c core somehow serializes the transfers for the given master.

> Is this needed because you need to send the "power off" command via i2c?

Yes, most embedded boards do power off by sending i2c command to PMIC chip.


> Could you try to test with PREEMPT_RT? I'm sure how this works there.

I will check this, but so far I run all my tests without PREEMPT_RT

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [BUG] exynos5_i2c_xfer_atomic() can sleep.

[Sebastian Andrzej Siewior]

On 2026-05-08 17:28:10 [+0200], Marek Szyprowski wrote:
> On 08.05.2026 12:42, Sebastian Andrzej Siewior wrote:
> > On 2026-05-08 12:35:51 [+0200], Marek Szyprowski wrote:
> >> Ah, it looks that I've checked irq_disable() code, so that's why I
> >> didn't find might_sleep() call. In the case of exynos5_i2c driver,
> >> probably switching to disable_irq_nosync() will be enough. I assume
> >> that all previous transfers have to be finished to start this atomic
> >> one, so waiting for interrupts to finish is not needed.
> > This looks kind of odd. Are the "other" transfers really done at this
> > point? Do you have a backtrace for me from the atomic path?
> 
> I thought that i2c core somehow serializes the transfers for the given master.
> 
> > Is this needed because you need to send the "power off" command via i2c?
> 
> Yes, most embedded boards do power off by sending i2c command to PMIC chip.

I'm wondering if you could "avoid" the interrupts-off part since
kernel_power_off() is called from preemtible context.

> 
> > Could you try to test with PREEMPT_RT? I'm sure how this works there.
> 
> I will check this, but so far I run all my tests without PREEMPT_RT

Right. I am curious if interrupts are really disabled on PREEMPT_RT or
if this happens as a side-effect of something that does not happen with
PREEMPT_RT enabled.

> Best regards

Sebastian