[PATCH v7 2/5] Re: i2c: core: run atomic i2c xfer when !preemptible
From: Michael Walle
Date: Tue Jan 02 2024 - 10:04:07 EST
Hi,
> Since bae1d3a05a8b, i2c transfers are non-atomic if preemption is
> disabled. However, non-atomic i2c transfers require preemption (e.g. in
> wait_for_completion() while waiting for the DMA).
>
> panic() calls preempt_disable_notrace() before calling
> emergency_restart(). Therefore, if an i2c device is used for the
> restart, the xfer should be atomic. This avoids 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
>
> Use !preemptible() instead, which is basically the same check as
> pre-v5.2.
>
> Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> Cc: stable@xxxxxxxxxxxxxxx # v5.2+
> Suggested-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> Acked-by: Wolfram Sang <wsa@xxxxxxxxxx>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
> Tested-by: Nishanth Menon <nm@xxxxxx>
> Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> ---
> drivers/i2c/i2c-core.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 1247e6e6e975..05b8b8dfa9bd 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,7 +29,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
> */
> static inline bool i2c_in_atomic_xfer_mode(void)
> {
> - return system_state > SYSTEM_RUNNING && irqs_disabled();
> + return system_state > SYSTEM_RUNNING && !preemptible();
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.692313] WARNING: CPU: 6 PID: 275 at drivers/i2c/i2c-core.h:40 i2c_smbus_xfer+0x100/0x118
[ 12.700745] Modules linked in:
[ 12.703788] CPU: 6 PID: 275 Comm: reboot Not tainted 6.7.0-rc6-next-20231222+ #2494
[ 12.711431] Hardware name: Kontron 3.5"-SBC-i1200 (DT)
[ 12.716555] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 12.723504] pc : i2c_smbus_xfer+0x100/0x118
[ 12.727674] lr : i2c_smbus_xfer+0x100/0x118
[ 12.731844] sp : ffff80008389b7c0
[ 12.735146] x29: ffff80008389b7c0 x28: ffff0000c561b4c0 x27: ffff0000c2b06400
[ 12.742268] x26: ffff0000c65aec4a x25: 000000000000000b x24: 0000000000000001
[ 12.749389] x23: 0000000000000000 x22: 0000000000000064 x21: 0000000000000008
[ 12.756510] x20: ffff80008389b836 x19: ffff0000c2dda080 x18: ffffffffffffffff
[ 12.763631] x17: ffff800080813f48 x16: ffff80008081bd38 x15: 0730072d07630732
[ 12.770752] x14: 0769072707200772 x13: 0730072d07630732 x12: 0769072707200772
[ 12.777873] x11: 0720072007200720 x10: ffff8000827dc828 x9 : ffff80008012e154
[ 12.784994] x8 : 00000000ffffefff x7 : ffff8000827dc828 x6 : 80000000fffff000
[ 12.792116] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
[ 12.799236] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c561b4c0
[ 12.806359] Call trace:
[ 12.808793] i2c_smbus_xfer+0x100/0x118
[ 12.812616] i2c_smbus_read_i2c_block_data+0x60/0xb0
[ 12.817569] mt6360_regmap_read+0xa4/0x1b8
[ 12.821654] _regmap_raw_read+0xfc/0x270
[ 12.825566] _regmap_bus_read+0x4c/0x90
[ 12.829389] _regmap_read+0x6c/0x168
[ 12.832953] _regmap_update_bits+0xf8/0x150
[ 12.837124] regmap_update_bits_base+0x6c/0xa8
[ 12.841555] regulator_disable_regmap+0x48/0x68
[ 12.846073] _regulator_do_disable+0x130/0x1e8
[ 12.850504] _regulator_disable+0x154/0x1d8
[ 12.854676] regulator_disable+0x44/0x90
[ 12.858587] mmc_regulator_set_ocr+0x8c/0x118
[ 12.862933] msdc_ops_set_ios+0x3f4/0x938
[ 12.866932] mmc_set_initial_state+0x90/0xa8
[ 12.871191] mmc_power_off+0x40/0x68
[ 12.874754] _mmc_sd_suspend+0x5c/0x190
[ 12.878577] mmc_sd_suspend+0x20/0x78
[ 12.882227] mmc_bus_shutdown+0x48/0x90
[ 12.886051] device_shutdown+0x134/0x298
[ 12.889961] kernel_restart+0x48/0xd0
[ 12.893613] __do_sys_reboot+0x1e0/0x278
[ 12.897523] __arm64_sys_reboot+0x2c/0x40
[ 12.901519] invoke_syscall+0x50/0x128
[ 12.905257] el0_svc_common.constprop.0+0x48/0xf0
[ 12.909950] do_el0_svc+0x24/0x38
[ 12.913253] el0_svc+0x38/0xd8
[ 12.916296] el0t_64_sync_handler+0x100/0x130
[ 12.920639] el0t_64_sync+0x1a4/0x1a8
[ 12.924289] ---[ end trace 0000000000000000 ]---
...
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.
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@xxxxxxxxxxx/