Re: [PATCH RFC v6 09/18] riscv_cbqri: Add bandwidth controller probe and allocation device ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Sentinel verification in cbqri_read_rbwb() and cbqri_read_mweight() spuriously fails with -EIO if the hardware's initial boot state matches the inverted software cache default.
- [High] Software enforcement of the sum(Rbwb) <= MRBWB hardware invariant is bypassed because the software cache ignores the initial hardware state.
- [High] Modifying a single bandwidth control field on an unconfigured RCID silently overwrites the other field with an artificial software default.
- [Medium] cbqri_read_rbwb() and cbqri_read_mweight() perform a read-modify-write on the staging register without waiting for the controller to be idle.
--

commit f4fbfec6ae3d51a8ce03e8b1e0e495abb3ac6e4d
Author: Drew Fustini <fustini@kernel.org>

riscv_cbqri: Add bandwidth controller probe and allocation device ops

Add support for CBQRI bandwidth controller (BC) discovery and the two BC
allocation control knobs. Rbwb is the number of reserved bandwidth
blocks per RCID. Mweight is the weighted share per RCID of the remaining
unreserved bandwidth.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> index c44bc50c31bab..6093706b1ec45 100644
> --- a/drivers/resctrl/cbqri_devices.c
> +++ b/drivers/resctrl/cbqri_devices.c

[ ... ]

> +static int cbqri_apply_bc_field(struct cbqri_controller *ctrl, u32 closid,
> +				enum cbqri_bc_field field, u64 val)
> +{
> +	u64 rbwb = ctrl->rbwb_cache[closid];
> +	u64 mweight = ctrl->mweight_cache[closid];

[ ... ]

> +	/*
> +	 * Wait for BUSY=0 before staging. A read-modify-write to the
> +	 * bc_bw_alloc staging register while an op is in flight can corrupt
> +	 * the unmodified field.
> +	 */
> +	if (cbqri_wait_busy_flag(ctrl, CBQRI_BC_ALLOC_CTL_OFF, NULL) < 0) {
> +		pr_err_ratelimited("BUSY timeout before staging bc_bw_alloc\n");
> +		return -EIO;
> +	}
> +
> +	cbqri_set_bc_bw_alloc(ctrl, rbwb, mweight);

[Severity: High]
Since rbwb_cache and mweight_cache are seeded with software defaults (0 and
FIELD_MAX) rather than the true initial hardware state, won't staging them
here overwrite the unmodified field's hardware state on the first
configuration?

For example, configuring Mweight on RCID 0 might silently overwrite its
hardware-initialized Rbwb value with 0 from the uninitialized cache.

[ ... ]

> +int cbqri_apply_rbwb(struct cbqri_controller *ctrl, u32 closid,
> +		     u64 rbwb, bool check_sum)
> +{
> +	u32 i;
> +	int ret;
> +
> +	if (rbwb > U16_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&ctrl->lock);
> +
> +	if (check_sum && rbwb > 0) {
> +		u64 sum = rbwb;
> +
> +		for (i = 0; i < ctrl->rcid_count; i++) {
> +			if (i == closid)
> +				continue;
> +			sum += ctrl->rbwb_cache[i];
> +		}
> +		if (sum > ctrl->bc.mrbwb) {

[Severity: High]
The rbwb_cache array is zero-initialized at probe, but the commit message
notes that hardware seeds RCID 0 with mrbwb - (rcid_count - 1) at reset.

Does calculating the sum from the unsynchronized software cache falsely pass
this check when configuring a different RCID to a large value? This might
allow the true hardware sum to exceed MRBWB.

[ ... ]

> +int cbqri_read_rbwb(struct cbqri_controller *ctrl, u32 closid, u64 *rbwb_out)
> +{
> +	u8 mweight_sentinel = ~ctrl->mweight_cache[closid];
> +	int err;
> +
> +	mutex_lock(&ctrl->lock);
> +
> +	/*
> +	 * Stage a sentinel into the unread Mweight field. A silent
> +	 * READ_LIMIT no-op (status SUCCESS but staging not refreshed) leaves
> +	 * the sentinel in place, while a real read overwrites Mweight with
> +	 * the hardware value, which differs from the inverted cache sentinel.
> +	 */
> +	cbqri_set_bc_bw_alloc(ctrl, ctrl->rbwb_cache[closid], mweight_sentinel);

[Severity: Medium]
In cbqri_apply_bc_field(), there is a wait for the BUSY flag to clear before
staging to prevent corrupting the register during an in-flight operation.

Should cbqri_read_rbwb() and cbqri_read_mweight() also wait for the BUSY flag
before calling cbqri_set_bc_bw_alloc() here?

> +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> +	if (err == 0) {
> +		if (cbqri_get_mweight(ctrl) == mweight_sentinel) {

[Severity: High]
The mweight_cache is seeded to 0xFF at probe, making mweight_sentinel equal
0x00.

If the hardware's initial boot state is also Mweight=0x00, a successful
READ_LIMIT overwrites the staging register with the true hardware value
(0x00). Can this legitimately match the sentinel and cause a spurious -EIO
failure?

[ ... ]

> +int cbqri_read_mweight(struct cbqri_controller *ctrl, u32 closid, u64 *mweight_out)
> +{
> +	u16 rbwb_sentinel = ~ctrl->rbwb_cache[closid];
> +	int err;
> +
> +	mutex_lock(&ctrl->lock);
> +
> +	/*
> +	 * Stage a sentinel into the unread Rbwb field so a silent READ_LIMIT
> +	 * no-op is detectable, mirroring cbqri_read_rbwb().
> +	 */
> +	cbqri_set_bc_bw_alloc(ctrl, rbwb_sentinel, ctrl->mweight_cache[closid]);

[Severity: Medium]
As mentioned above, does this read-modify-write operation also need to wait
for the controller to be idle before calling cbqri_set_bc_bw_alloc()?

> +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> +	if (err == 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-ssqosid-cbqri-rqsc-v7-0-v6-0-baf00f50028a@kernel.org?part=9