Date: Wed, 03 Jan 2024 16:07:32 +0100
From: Michael Walle <mwalle@...nel.org>
To: Benjamin Bara <bbara93@...il.com>
Cc: benjamin.bara@...data.com, dmitry.osipenko@...labora.com,
 jonathanh@...dia.com, lee@...nel.org, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org, nm@...com,
 peterz@...radead.org, rafael.j.wysocki@...el.com, richard.leitner@...ux.dev,
 stable@...r.kernel.org, treding@...dia.com,
 wsa+renesas@...g-engineering.com, wsa@...nel.org
Subject: Re: [PATCH v7 2/5] Re: i2c: core: run atomic i2c xfer when
 !preemptible

Hi Benjamin,

>> >> With preemption disabled, this boils down to
>> >>   return system_state > SYSTEM_RUNNING (&& !0)
>> >>
>> >> and will then generate a backtrace splash on each reboot on our
>> >> board:
>> >>
>> >> # reboot -f
>> >> [   12.687169] No atomic I2C transfer handler for 'i2c-0'
>> >> ...
>> >> [   12.806359] Call trace:
>> >> [   12.808793]  i2c_smbus_xfer+0x100/0x118
>> >> ...
>> >>
>> >> I'm not sure if this is now the expected behavior or not. There will
>> >> be
>> >> no backtraces, if I build a preemptible kernel, nor will there be
>> >> backtraces if I revert this patch.
>> >
>> >
>> > thanks for the report.
>> >
>> > In your case, the warning comes from shutting down a regulator during
>> > device_shutdown(), so nothing really problematic here.
>> 
>> I tend to disagree. Yes it's not problematic. But from a users point 
>> of
>> view, you get a splash of *many* backtraces on every reboot. Btw, one
>> should really turn this into a WARN_ONCE(). But even in this case you
>> might scare users which will eventually lead to more bug reports.
> 
> Sure, but the correct "fix" would be to implement an atomic handler if
> the i2c is used during this late stage. I just meant that the
> device_shutdown() is less problematic than the actual reboot handler.
> Your PMIC seems to not have a reboot handler (registered (yet)), and is
> therefore not "affected".
> 
>> > However, later in
>> > the "restart sequence", IRQs are disabled before the restart handlers
>> > are called. If the reboot handlers would rely on irq-based
>> > ("non-atomic") i2c transfer, they might not work properly.
>> 
>> I get this from a technical point of view and agree that the correct
>> fix is to add the atomic variant to the i2c driver, which begs the
>> question, if adding the atomic variant to the driver will be 
>> considered
>> as a Fixes patch.
> 
> I can add a Fixes when I post it. Although the initial patch just makes
> the actual problem "noisier".

As far as I understand, there was no problem (for me at least),
because the interrupts were still enabled at this time. But now,
there is the problem with getting these backtraces and with that
the user reports.

Don't get me wrong, I'm all for the correct fix here. But at the
same time I fear all the reports we'll be getting. And in the meantime
there was already a new one.

>> Do I get it correct, that in my case the interrupts are still enabled?
>> Otherwise I'd have gotten this warning even before your patch, 
>> correct?
> 
> Yes, device_shutdown() is called during
> kernel_{shutdown,restart}_prepare(), before
> machine_{power_off,restart}() is called. The interrupts should 
> therefore
> still be enabled in your case.
> 
>> Excuse my ignorance, but when are the interrupts actually disabled
>> during shutdown?
> 
> This is usually one of the first things done in machine_restart(),
> before the architecture-specific restart handlers are called (which
> might use i2c). Same for machine_power_off().

Thanks for explaining.

>> >> OTOH, the driver I'm using (drivers/i2c/busses/i2c-mt65xx.c) has no
>> >> *_atomic(). So the warning is correct. There is also [1], which seems
>> >> to
>> >> be the same issue I'm facing.
>> >>
>> >> -michael
>> >>
>> >> [1]
>> >> https://lore.kernel.org/linux-i2c/13271b9b-4132-46ef-abf8-2c311967bb46@mailbox.org/
>> >
>> >
>> > I tried to implement an atomic handler for the mt65xx, but I don't have
>> > the respective hardware available to test it. I decided to use a
>> > similar
>> > approach as done in drivers/i2c/busses/i2c-rk3x.c, which calls the IRQ
>> > handler in a while loop if an atomic xfer is requested. IMHO, this
>> > should work with IRQs enabled and disabled, but I am not sure if this
>> > is
>> > the best approach...
>> 
>> Thanks for already looking into that. Do you want to submit it as an
>> actual patch? If so, you can add
>> 
>> Tested-by: Michael Walle <mwalle@...nel.org>
> 
> Yes, I can do that - thanks for the quick feedback.
> 
>> But again, it would be nice if we somehow can get rid of this huge
>> splash
>> of backtraces on 6.7.x (I guess it's already too late 6.7).
> 
> IMHO, converting the error to WARN_ONCE() makes sense to reduce the
> noise, but helps having more reliable reboot handling via i2c. Do you
> think this is a sufficient "short-term solution" to reduce the noise
> before the missing atomic handlers are actually implemented?

Turning that WARN into a WARN_ONCE is one thing. But it is still odd
that don't I get a warning with preemption enabled. Is that because
preemptible() will still return 1 until interrupts are actually 
disabled?
Can we achieve something similar with kernels without preemption 
support?
IOW, just warn iff there is an actual error, that is if i2c_xfer()
is called with interrupt off?

-michael

Powered by blists - more mailing lists