Hi!
The Tegra20 requires an enabled VDE power domain during startup. As the VDE is currently not used, it is disabled during runtime. Since [1], there is a workaround for the "normal restart path" which enables the VDE before doing PMC's warm reboot. This workaround is not executed in the "emergency restart path", leading to a hang-up during start.
This series implements and registers a new pmic-based restart handler for boards with tps6586x. This cold reboot ensures that the VDE power domain is enabled on tegra20-based boards.
During panic(), preemption is disabled. This should be correctly detected by i2c_in_atomic_xfer_mode() to use atomic i2c xfer in this late stage. This avoids warnings regarding "Voluntary context switch within RCU".
[1] 8f0c714ad9be1ef774c98e8819a7a571451cb019 v2: https://lore.kernel.org/all/20230320220345.1463687-1-bbara93@gmail.com/ system_state: https://lore.kernel.org/all/20230320213230.1459532-1-bbara93@gmail.com/ v1: https://lore.kernel.org/all/20230316164703.1157813-1-bbara93@gmail.com/
v3: - bring system_state back in this series - do atomic i2c xfer if not preemptible (as suggested by Dmitry) - fix style issues mentioned by Dmitry - add cc stable as suggested by Dmitry - add explanation why this is needed for Jon
v2: - use devm-based restart handler - convert the existing power_off handler to a devm-based handler - handle system_state in extra series
--- Benjamin Bara (4): kernel/reboot: emergency_restart: set correct system_state i2c: core: run atomic i2c xfer when !preemptible mfd: tps6586x: use devm-based power off handler mfd: tps6586x: register restart handler
drivers/i2c/i2c-core.h | 2 +- drivers/mfd/tps6586x.c | 43 +++++++++++++++++++++++++++++++++++-------- kernel/reboot.c | 1 + 3 files changed, 37 insertions(+), 9 deletions(-) --- base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa change-id: 20230327-tegra-pmic-reboot-4175ff814a4b
Best regards,
From: Benjamin Bara benjamin.bara@skidata.com
As the emergency restart does not call kernel_restart_prepare(), the system_state stays in SYSTEM_RUNNING.
This e.g. hinders i2c_in_atomic_xfer_mode() from becoming active, and therefore might lead to avoidable warnings in the restart handlers, e.g.:
[ 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
Avoid these by setting the correct system_state.
Cc: stable@vger.kernel.org # v5.2+ Signed-off-by: Benjamin Bara benjamin.bara@skidata.com --- kernel/reboot.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/kernel/reboot.c b/kernel/reboot.c index 3bba88c7ffc6..6ebef11c8876 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void); void emergency_restart(void) { kmsg_dump(KMSG_DUMP_EMERG); + system_state = SYSTEM_RESTART; machine_emergency_restart(); } EXPORT_SYMBOL_GPL(emergency_restart);
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(-)
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(); }
static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
- 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, 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)?
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.
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.
Thanks & best regards, Benjamin
[1] drivers/i2c/busses/i2c-imx.c [2] https://www.kernel.org/doc/Documentation/scheduler/completion.txt
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).
linux-stable-mirror@lists.linaro.org
-
Benjamin Bara -
Peter Zijlstra -
Wolfram Sang