[v3,2/4] i2c: core: run atomic i2c xfer when !preemptible

From: Benjamin Bara <benjamin.bara@skidata.com>

A panic() calls preempt_disable_notrace() before calling
emergency_restart(). If an i2c device is used for the restart, the xfer
should not schedule out (to avoid warnings like):

[   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
[   12.676926] Voluntary context switch within RCU read-side critical section!
...
[   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
[   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
...
[   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
[   13.001050]  machine_restart from panic+0x2a8/0x32c

Therefore, not only check for disabled IRQs but also for preemptible.

Cc: stable@vger.kernel.org # v5.2+
Suggested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/i2c/i2c-core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

> -	return system_state > SYSTEM_RUNNING && irqs_disabled();
> +	return system_state > SYSTEM_RUNNING && !preemptible();

For the !CONFIG_PREEMPT_COUNT case, preemptible() is defined 0. So,
don't we lose the irqs_disabled() check in that case?

On Mon, Mar 27, 2023 at 06:23:24PM +0200, Benjamin Bara wrote:
> On Mon, 27 Mar 2023 at 16:54, Wolfram Sang <wsa@kernel.org> wrote:
> > For the !CONFIG_PREEMPT_COUNT case, preemptible() is defined 0. So,
> > don't we lose the irqs_disabled() check in that case?
> 
> Thanks for the feedback!
> PREEMPT_COUNT is selected by PREEMPTION, so I guess in the case of
> !PREEMPT_COUNT,
> we should be atomic (anyways)?

Could you make sure please? Asking Peter Zijlstra might be a good idea.
He helped me with the current implementation.

On Wed, 29 Mar 2023 at 21:50, Wolfram Sang <wsa@kernel.org> wrote:
> Could you make sure please?

Sure, I'll try. The check before bae1d3a was:
in_atomic() || irqs_disabled()
which boils down to:
(preempt_count() != 0) || irqs_disabled()
preemptible() is defined as:
(preempt_count() == 0 && !irqs_disabled())

so this patch should behave the same as pre-v5.2, but with the
additional system state check. From my point of view, the additional
value of the in_atomic() check was that it activated atomic i2c xfers
when preemption is disabled, like in the case of panic(). So reverting
that commit would also re-activate atomic i2c transfers during emergency
restarts. However, I think considering the system state makes sense
here.

On Sun, Apr 02, 2023 at 12:04:48PM +0200, Benjamin Bara wrote:
> On Wed, 29 Mar 2023 at 21:50, Wolfram Sang <wsa@kernel.org> wrote:
> > Could you make sure please?
> 
> Sure, I'll try. The check before bae1d3a was:
> in_atomic() || irqs_disabled()
> which boils down to:
> (preempt_count() != 0) || irqs_disabled()
> preemptible() is defined as:
> (preempt_count() == 0 && !irqs_disabled())
> 
> so this patch should behave the same as pre-v5.2, but with the
> additional system state check. From my point of view, the additional
> value of the in_atomic() check was that it activated atomic i2c xfers
> when preemption is disabled, like in the case of panic(). So reverting
> that commit would also re-activate atomic i2c transfers during emergency
> restarts. However, I think considering the system state makes sense
> here.
> 
> From my understanding, non-atomic i2c transfers require enabled IRQs,
> but atomic i2c transfers do not have any "requirements". So the
> irqs_disabled() check is not here to ensure that the following atomic
> i2c transfer works correctly, but to use non-atomic i2c xfer as
> long/often as possible.
> 
> Unfortunately, I am not sure yet about !CONFIG_PREEMPTION. I looked into
> some i2c-bus implementations which implement both, atomic and
> non-atomic. As far as I saw, the basic difference is that the non-atomic
> variants usually utilize the DMA and then call a variant of
> wait_for_completion(), like in i2c_imx_dma_write() [1]. However, the
> documentation of wait_for_completion [2] states that:
> "wait_for_completion() and its variants are only safe in process context
> (as they can sleep) but not (...) [if] preemption is disabled".
> Therefore, I am not quite sure yet if !CONFIG_PREEMPTION uses the
> non-atomic variant at all or if this case is handled differently.
> 
> > Asking Peter Zijlstra might be a good idea.
> > He helped me with the current implementation.
> 
> Thanks for the hint! I wrote an extra email to him and added him to CC.

So yeah, can't call schedule() if non preemptible (which is either
preempt_disable(), local_bh_disable() (true for bh handlers) or
local_irq_disable() (true for IRQ handlers) and mostly rcu_read_lock()).

You can mostly forget about CONFIG_PREEMPT=n (or more specifically
CONFIG_PREMPT_COUNT=n) things that work for PREEMPT typically also work
for !PREEMPT.

The question here seems to be if i2c_in_atomic_xfer_mode() should have
an in_atomic() / !preemptible() check, right? IIUC Wolfram doesn't like
it being used outside of extra special cicumstances?

On Tue, 4 Apr 2023 at 10:23, Peter Zijlstra <peterz@infradead.org> wrote:
> You can mostly forget about CONFIG_PREEMPT=n (or more specifically
> CONFIG_PREMPT_COUNT=n) things that work for PREEMPT typically also work
> for !PREEMPT.

Thanks for the clarification!

> The question here seems to be if i2c_in_atomic_xfer_mode() should have
> an in_atomic() / !preemptible() check, right? IIUC Wolfram doesn't like
> it being used outside of extra special cicumstances?

Sorry for expressing things more complicated than needed.
Yes, exactly. Wolfram expressed considerations of adding preemptible() as
check, as the now existing irq_disabled() check is lost with !PREEMPT. I tried
to clarify that the check seems to be there for "performance reasons" and that
the check essentially was !preemptible() before v5.2, so nothing should break.

However, what came to my mind during my "research", is that it might make sense
to handle all i2c transfers atomically when in a post RUNNING state (namely
HALT, POWER_OFF, RESTART, SUSPEND). The outcome would basically be the same as
with this patch series applied, but I guess the reasoning behind it would be
simplified: If in a restart handler, the i2c transfer is atomic, independent of
current IRQ or preemption state. Currently, the state depends on from where the
handler is triggered. As you have stated, most of the i2c transfers in the
mentioned states will just update single bits for power-off/sleep/suspend, so
IMHO nothing where not using a DMA would matter from a performance point of
view. But there might be other reasons for not doing so - I guess in this case
the provided patch is fine (at least from my side).