Re: [PATCH RFC v4 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 5 potential issue(s) to consider:
- [High] State divergence on hardware verification failure breaks capacity invariants.
- [High] Unprotected initial `READ_LIMIT` is vulnerable to silent hardware failure and causes cross-RCID state corruption.
- [High] A single transient hardware timeout permanently disables the controller due to a latched `faulted` flag.
- [Medium] Feature probing fails to clear `EVT_ID`, risking false-negative feature detection.
- [Medium] Missing bounds check on `mweight` causes silent truncation and false I/O errors.
--

commit 40fea10bc5e942138ec126160ec0621edcc9673f
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. Both fields share the bc_bw_alloc register, so
each write needs READ_LIMIT-modify-CONFIG_LIMIT-verify with a sentinel.

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> index e46b02d2c50d7..7e5decd7a6b2e 100644
> --- a/drivers/resctrl/cbqri_devices.c
> +++ b/drivers/resctrl/cbqri_devices.c
> @@ -145,6 +184,44 @@ int cbqri_mon_op(struct cbqri_controller *ctrl, int reg_offset,
>  	return 0;
>  }
>  
> +/*
> + * Perform bandwidth allocation control operation on bandwidth controller.
> + * Caller must hold ctrl->lock.
> + */
> +static int cbqri_bc_alloc_op(struct cbqri_controller *ctrl, int operation, int rcid)
> +{
> +	int reg_offset = CBQRI_BC_ALLOC_CTL_OFF;
> +	int status;
> +	u64 reg;
> +
> +	lockdep_assert_held(&ctrl->lock);
> +
> +	if (ctrl->faulted)
> +		return -EIO;
> +
> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {

If a timeout in cbqri_wait_busy_flag() sets ctrl->faulted = true, will
runtime operations like cbqri_bc_alloc_op() ever execute again? Because
it returns early here when ctrl->faulted is true, the polling function
is never called again to clear the flag, which seems to permanently
disable the controller after a single timeout.

[ ... ]

> +static int cbqri_apply_bc_field(struct cbqri_controller *ctrl, u32 closid,
> +				void (*set)(struct cbqri_controller *, u64),
> +				u64 (*get)(struct cbqri_controller *),
> +				u64 val, bool *committed)
> +{
> +	int ret;
> +	u64 reg;
> +
> +	lockdep_assert_held(&ctrl->lock);
> +
> +	/* Load current RCID state so the unmodified field is preserved */
> +	ret = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> +	if (ret < 0)
> +		return ret;

Does this initial read need the same sentinel protection as the
verification read? If this READ_LIMIT silently fails, the staging
register might retain data from a previous operation (like a different
RCID). Since set() only modifies the target field, CONFIG_LIMIT could
commit the stale data in the unmodified field, causing cross-RCID state
corruption.

> +
> +	set(ctrl, val);
> +
> +	ret = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_CONFIG_LIMIT, closid);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * CONFIG_LIMIT committed. The per-CLOSID software cache must
> +	 * track hardware regardless of whether the verify below passes.
> +	 */
> +	if (committed)
> +		*committed = true;

[ ... ]

> +	reg = get(ctrl);
> +	if (reg != val) {
> +		pr_err_ratelimited("BC field verify mismatch (reg=0x%llx != val=%llu)\n",
> +				   reg, val);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

If the verification fails here and returns -EIO, *committed remains
true. When called from cbqri_apply_rbwb(), this causes the rejected
value to be cached in ctrl->rbwb_cache[closid]. If the requested value
was smaller than the previous one, could this cause the software cache
to underestimate the reserved capacity and allow future allocations to
exceed the MRBWB hardware limit?

[ ... ]

> +int cbqri_apply_mweight_config(struct cbqri_controller *ctrl, u32 closid,
> +			       u64 mweight)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctrl->lock);
> +	ret = cbqri_apply_bc_field(ctrl, closid,
> +				   cbqri_set_mweight, cbqri_get_mweight,
> +				   mweight, NULL);
> +	mutex_unlock(&ctrl->lock);
> +	return ret;
> +}

What happens if an arbitrarily large mweight is passed in? FIELD_MODIFY
silently truncates values larger than the 8-bit MWEIGHT_MASK. The
verification step in cbqri_apply_bc_field() will then compare the
truncated hardware value against the original untruncated val and
always fail, returning -EIO and logging a hardware failure instead of
returning -EINVAL. Should this have bounds checking similar to
cbqri_apply_rbwb()?

[ ... ]

> @@ -437,6 +664,83 @@ static int cbqri_probe_cc(struct cbqri_controller *ctrl)
>  	return 0;
>  }
>  
> +static int cbqri_probe_bc(struct cbqri_controller *ctrl)
> +{
> +	bool has_mon_at_code = false;
> +	int err, status;
> +	u64 reg;

[ ... ]

> +	/* Probe monitoring features */
> +	err = cbqri_probe_feature(ctrl, CBQRI_BC_MON_CTL_OFF,
> +				  CBQRI_BC_MON_CTL_OP_READ_COUNTER, &status,
> +				  &has_mon_at_code);

When probing features, cbqri_probe_feature() modifies the OP mask and
clears RCID_MASK, but it doesn't appear to clear EVT_ID_MASK:

drivers/resctrl/cbqri_devices.c:cbqri_probe_feature() {
    ...
    FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_OP_MASK, &reg, operation);
    reg &= ~CBQRI_CONTROL_REGISTERS_RCID_MASK;
    iowrite64(reg, ctrl->base + reg_offset);
    ...
}

If the hardware or firmware left an unsupported Event ID in those bits,
could the hardware reject the READ_COUNTER operation, causing the probe
to see STATUS != SUCCESS and incorrectly assume monitoring is not
supported?

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