Re: [PATCH v3 3/8] riscv: Add support for srmcfg CSR from Ssqosid extension

[Drew Fustini <fustini@kernel.org>]

On Sun, Jun 28, 2026 at 09:28:20PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Failing cpuhp_setup_state skips PM notifier registration, silently breaking QoS IDs across system suspend/resume.
> --
> 
> commit e3ff463a672a85eb328bf3048323a464af88e408
> Author: Drew Fustini <fustini@kernel.org>
> 
> riscv: Add support for srmcfg CSR from Ssqosid extension
> 
> This commit adds support for the srmcfg CSR defined in the Ssqosid ISA
> extension. It introduces a per-task thread field and a per-CPU cache to track
> Resource Control ID (RCID) and Monitoring Counter ID (MCID) fields, updating
> the CSR during context switches.
> 
> Link: https://github.com/riscv/riscv-ssqosid/releases/tag/v1.0
> 
> [ ... ]
> 
> > diff --git a/arch/riscv/kernel/qos.c b/arch/riscv/kernel/qos.c
> > new file mode 100644
> > index 0000000000000..c8900d91996f2
> > --- /dev/null
> > +++ b/arch/riscv/kernel/qos.c
> 
> [ ... ]
> 
> > +static int __init riscv_srmcfg_init(void)
> > +{
> > +	int err;
> > +
> > +	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSQOSID))
> > +		return 0;
> > +
> > +	/*
> > +	 * cpuhp_setup_state() invokes the startup callback locally on every
> > +	 * already-online CPU, so no separate seed loop is needed here.
> > +	 */
> > +	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "riscv/srmcfg:online",
> > +				riscv_srmcfg_reset_cache, riscv_srmcfg_reset_cache);
> > +	if (err < 0) {
> > +		pr_warn("srmcfg cpuhp registration failed (%d), cpus brought online after boot will not invalidate the CSR_SRMCFG cache\n",
> > +			err);
> > +		return err;
> 
> [Severity: Medium]
> If cpuhp_setup_state() fails and returns early here, the
> cpu_pm_register_notifier() registration is skipped, but the hardware
> capability RISCV_ISA_EXT_SSQOSID remains active.
> 
> Because returning an error from an arch_initcall does not halt the boot
> process, the QoS feature will remain active in the kernel.
> 
> If the PM notifier is not registered to invalidate the software cache upon
> exit from low-power states, will subsequent context switches find a cache
> match, mistakenly bypass the csr_write(), and execute tasks with the wrong
> hardware QoS ID?

Yes, it is possible. The early return in riscv_srmcfg_init() does skip
cpu_pm_register_notifier(), and that notifier is what invalidates the
per-cpu srmcfg cache across suspend/resume, so __switch_to_srmcfg()
could find a stale match and skip the csr write.

The risk is low, though. CPUHP_AP_ONLINE_DYN is a valid state, the
registration name is not null, and riscv_srmcfg_reset_cache() cannot
fail, so the only way cpuhp_setup_state() can fail is when no dynamic
hotplug states remain.

However, the fix is simple. The PM notifier is independent of the cpuhp
state, so I'll change riscv_srmcfg_init() to register it even if the
cpuhp setup failed.

Thanks,
Drew