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

[Drew Fustini <fustini@kernel.org>]

On Mon, May 25, 2026 at 12:30:40AM +0000, sashiko-bot@kernel.org wrote:
> > +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 (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> > +		pr_err_ratelimited("BUSY timeout before starting operation\n");
> > +		return -EIO;
> > +	}
> > +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_OP_MASK, &reg, operation);
> > +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_RCID_MASK, &reg, rcid);
> > +	iowrite64(reg, ctrl->base + reg_offset);
> 
> [Severity: High]
> Does this operation inadvertently preserve the Access Type (AT) field left over
> by firmware or prior probe operations?

I'll match cbqri_cc_alloc_op() and clear AT.

> > +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];
> > +	u64 readback;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&ctrl->lock);
> > +
> > +	if (field == CBQRI_BC_FIELD_RBWB)
> > +		rbwb = val;
> > +	else
> > +		mweight = val;
> > +
> > +	cbqri_set_bc_bw_alloc(ctrl, rbwb, mweight);
> > +
> > +	ret = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_CONFIG_LIMIT, closid);
> 
> Could modifying the hardware staging register before verifying the controller
> is ready cause a silent corruption of the unmodified field?

Yes, I'll change it to wait for BUSY=0 before the staging write.

> > +/*
> > + * Read the Rbwb (reserved bandwidth blocks) for closid via READ_LIMIT.
> > + */
> > +int cbqri_read_rbwb(struct cbqri_controller *ctrl, u32 closid, u64 *rbwb_out)
> > +{
> > +	int err;
> > +
> > +	mutex_lock(&ctrl->lock);
> > +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> > +	if (err == 0)
> > +		*rbwb_out = cbqri_get_rbwb(ctrl);
> > +	mutex_unlock(&ctrl->lock);
> > +	return err;
> > +}
> > +
> > +/*
> > + * Read the Mweight (opportunistic weight) for closid via READ_LIMIT.
> > + */
> > +int cbqri_read_mweight(struct cbqri_controller *ctrl, u32 closid, u64 *mweight_out)
> > +{
> > +	int err;
> > +
> > +	mutex_lock(&ctrl->lock);
> > +	err = cbqri_bc_alloc_op(ctrl, CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT, closid);
> > +	if (err == 0)
> > +		*mweight_out = cbqri_get_mweight(ctrl);
> > +	mutex_unlock(&ctrl->lock);
> > +	return err;
> > +}
> 
> [Severity: High]
> Are these standalone read functions vulnerable to the same silent READ_LIMIT
> no-op hardware bug described in the commit message?
> 
> Since they execute a bare READ_LIMIT without the pre-write sentinel validation
> used in cbqri_apply_bc_field(), could they return stale data from a prior
> operation if the hardware bug triggers?

I'll fix this in v6 with sentinel.

-Drew