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/[email protected]/
system_state: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
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,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
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: [email protected] # v5.2+
Signed-off-by: Benjamin Bara <[email protected]>
---
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);
--
2.34.1
From: Benjamin Bara <[email protected]>
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: [email protected] # v5.2+
Suggested-by: Dmitry Osipenko <[email protected]>
Signed-off-by: Benjamin Bara <[email protected]>
---
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)
--
2.34.1
> - 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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).