Re: [PATCH v7 1/5] riscv_cbqri: Add capacity controller probe and allocation device ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware race condition: writing staging registers before verifying BUSY == 0
--

commit e6f3519d6bbd7a39edfda72f253929a5410c1ed9
Author: Drew Fustini <fustini@xxxxxxxxxx>

riscv_cbqri: Add capacity controller probe and allocation device ops

This commit adds support for the RISC-V CBQRI capacity controller. It provides
a platform driver that probes the controller and registers allocation device ops
with the resctrl filesystem.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> new file mode 100644
> index 0000000000000..e93261dfa4fa9
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_devices.c
[ ... ]
> +int cbqri_apply_cache_config(struct cbqri_controller *ctrl, u32 closid,
> +			     const struct cbqri_cc_config *cfg)
> +{
> +	bool need_at_mirror;
> +	u64 saved_cbm = 0;
> +	int err = 0;
> +	u64 reg;
> +
> +	mutex_lock(&ctrl->lock);
> +
> +	need_at_mirror = ctrl->cc.supports_alloc_at_code && !cfg->cdp_enabled;
> +
> +	/*
> +	 * Capture the cfg->at half CBM before any write so a partial
> +	 * AT-mirror failure can revert and keep the two halves consistent.
> +	 * Pre-clear cc_block_mask so a silent firmware no-op (status
> +	 * SUCCESS but staging not updated) shows as a zero readback
> +	 * rather than carrying stale data from a prior op.
> +	 */
> +	if (need_at_mirror) {
> +		cbqri_set_cbm(ctrl, 0);
> +		err = cbqri_cc_alloc_op(ctrl, CBQRI_CC_ALLOC_CTL_OP_READ_LIMIT,
> +					closid, cfg->at);

[Severity: Medium]
Does this overwrite the cc_block_mask staging register before verifying the
hardware is idle?

If a previous operation timed out but the controller is still actively
processing it, modifying the CBM register here before cbqri_cc_alloc_op()
checks the BUSY flag could corrupt the in-flight operation.

> +		if (err < 0)
> +			goto out;
> +		saved_cbm = cbqri_readq(ctrl->base + CBQRI_CC_BLOCK_MASK_OFF);
> +	}
> +
> +	/* Set capacity block mask (cc_block_mask) */
> +	cbqri_set_cbm(ctrl, cfg->cbm);
> +	cbqri_clear_cunits(ctrl);
> +
> +	/* Capacity config limit operation for the AT half implied by cfg->at */
> +	err = cbqri_cc_alloc_op(ctrl, CBQRI_CC_ALLOC_CTL_OP_CONFIG_LIMIT,
> +				closid, cfg->at);

[Severity: Medium]
Similarly, does this overwrite both the CBM and CUNITS staging registers
before verifying the hardware has finished any previous operations?

cbqri_cc_alloc_op() waits for BUSY == 0, but this happens after these
staging registers have already been written. If a previous caller's
operation timed out and the hardware is still busy, could this cause
the new operation to use corrupted staging data or disrupt the pending
operation?

> +	if (err < 0)
> +		goto out;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-dfustini-atl-sc-cbqri-dt-v7-0-ca2935d85622@xxxxxxxxxx?part=1