Re: [PATCH RFC] riscv: disable local interrupts and stop other CPUs before restart

[Troy Mitchell]

Hi Vivian,

Thanks for the review and the insights!

On Thu Mar 12, 2026 at 11:05 AM CST, Vivian Wang wrote:
> On 3/11/26 10:51, Troy Mitchell wrote:
>> Currently, the RISC-V implementation of machine_restart() directly calls
>> do_kernel_restart() without disabling local interrupts or stopping other
>> CPUs. This missing architectural setup causes fatal issues for systems
>> that rely on external peripherals (e.g., I2C PMICs) to execute the system
>> restart when CONFIG_PREEMPT_RCU is enabled.
>>
>> When a restart handler relies on the I2C subsystem, the I2C core checks
>> i2c_in_atomic_xfer_mode() to decide whether to use the sleepable xfer
>> or the polling atomic_xfer. This check evaluates to true if
>> (!preemptible() || irqs_disabled()).
>>
>> During do_kernel_restart(), the restart handlers are invoked via
>> atomic_notifier_call_chain(), which holds an RCU read lock.
>> The behavior diverges based on the preemption model:
>> 1. Under CONFIG_PREEMPT_VOLUNTARY or CONFIG_PREEMPT_NONE, rcu_read_lock()
>>    implicitly disables preemption. preemptible() evaluates to false, and
>>    the I2C core correctly routes to the atomic, polling transfer path.
>> 2. Under CONFIG_PREEMPT_RCU, rcu_read_lock() does NOT disable preemption.
>>    Since machine_restart() left local interrupts enabled, irqs_disabled()
>>    is false, and preempt_count is 0. Consequently, preemptible() evaluates
>>    to true.
>>
>> As a result, the I2C core falsely assumes a sleepable context and routes
>> the transfer to the standard master_xfer path. This inevitably triggers a
>> schedule() call while holding the RCU read lock, resulting in a fatal splat:
>> "Voluntary context switch within RCU read-side critical section!" and
>> a system hang.
>>
>> Align RISC-V with other major architectures (e.g., ARM64) by adding
>> local_irq_disable() and smp_send_stop() to machine_restart().
>> - local_irq_disable() guarantees a strict atomic context, forcing sub-
>>   systems like I2C to always fall back to polling mode.
>> - smp_send_stop() ensures exclusive hardware access by quiescing other
>>   CPUs, preventing them from holding bus locks (e.g., I2C spinlocks)
>>   during the final restart phase.
>
> Maybe while we're at it, we can fix the other functions in this file as
> well?
Nice catch. I'll fix other functions in the next version.
>
> I think the reason we ended up with the "unsafe" implementations of the
> reboot/shutdown functions is that on the backend it is usually SBI SRST
> calls, which can protect itself from other CPUs and interrupts. Since on
> K1 we're going to be poking I2C directly, we run into the problem
> described above. So all of these should disable interrupts and stop
> other CPUs before calling the handlers, and can't assume the handlers
> are all SBI SRST.
Yes, we cannot assume that all platforms rely on this.
>
>> Signed-off-by: Troy Mitchell <troy.mitchell@xxxxxxxxx>
>> ---
>>  arch/riscv/kernel/reset.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
>> index 912288572226..7a5dcfdc3674 100644
>> --- a/arch/riscv/kernel/reset.c
>> +++ b/arch/riscv/kernel/reset.c
>> @@ -5,6 +5,7 @@
>>  
>>  #include <linux/reboot.h>
>>  #include <linux/pm.h>
>> +#include <linux/smp.h>
>>  
>>  static void default_power_off(void)
>>  {
>> @@ -17,6 +18,10 @@ EXPORT_SYMBOL(pm_power_off);
>>  
>>  void machine_restart(char *cmd)
>>  {
>> +	/* Disable interrupts first */
>> +	local_irq_disable();
>> +	smp_send_stop();
>> +
>>  	do_kernel_restart(cmd);
>>  	while (1);
>>  }
>
> So... one thing I'm not certain is that arm64 also has some EFI handling
> here. But I think the safe choice is to ignore EFI for now until it's
> needed, instead of preemptively doing it. Who knows what shenanigans
> firmwares can get up to.
I agree at all.

>
> Thanks for the patch!
I'll prepare v2 with those functions fixed. Looking forward to your further review
on the updated version

                                                - Troy
>
> Vivian "dramforever" Wang