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

[Samuel Holland <samuel.holland@sifive.com>]

Hi Troy,

On 2026-03-16 2:23 AM, Troy Mitchell wrote:
> 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.

Why isn't K1 using the SBI SRST extension? Resetting the platform from S-mode
directly causes problems if you ever want to run another supervisor domain (for
example a TEE or EFI runtime services), which may need to clean up before a
system reset.

As you mention, other platforms use the standard SBI SRST interface, event if
they need to poke a PMIC to perform a system reset. Is there something
preventing K1 from following this path?

Regards,
Samuel