Re: [PATCH RFC v5 08/18] riscv_cbqri: Add capacity controller monitoring device ops

[Drew Fustini <fustini@kernel.org>]

On Mon, May 25, 2026 at 12:29:22AM +0000, sashiko-bot@kernel.org wrote:
> > +	/* Probe monitoring features */
> > +	err = cbqri_probe_feature(ctrl, CBQRI_CC_MON_CTL_OFF,
> > +				  CBQRI_CC_MON_CTL_OP_READ_COUNTER, &status,
> > +				  &has_mon_at_code);
> 
> [Severity: Medium]
> Is it safe to reuse cbqri_probe_feature() for the MON_CTL register here?
> 
> cbqri_probe_feature() attempts to verify Access Type (AT) support by writing
> to the AT field bits. However, the MON_CTL register does not define an AT
> field, which means this implicitly writes to reserved (WPRI) bits. Could
> this helper be updated to conditionally skip the AT probe sequence for
> monitoring registers?

WPRI tolerates the write so this is not a correctness issue today. But
to avoid confusion, I will split cbqri_probe_feature() so the AT probe
sequence is only run for alloc_ctl registers.

> Will this probe sequence falsely disable monitoring on hardware?
> 
> This invokes cbqri_probe_feature() using the READ_COUNTER operation, which
> zeroes out all other fields including EVT_ID. This sends a read command for
> the NONE event (EVT_ID=0) to an unconfigured slot, which the hardware may
> reject with an error status. If the hardware returns an error, this check
> will fail and disable monitoring. Could this probe instead use
> CONFIG_EVENT or handle expected failure codes?

The NONE event as is a valid configured state. But I will switch the
probe operation to CONFIG_EVENT with a probe-safe EVT_ID to remove the
ambiguity.

> > +/*
> > + * Pre-arm every MCID with the Occupancy event so a subsequent READ_COUNTER
> > + * just snapshots the live counter rather than re-configuring the slot.
> > + * Called once per CC during resctrl-side cpuhp online for the L3 monitoring
> > + * domain.
> > + */
> > +int cbqri_init_mon_counters(struct cbqri_controller *ctrl)
> > +{
> > +	int i, err;
> > +
> > +	for (i = 0; i < ctrl->mcid_count; i++) {
> > +		mutex_lock(&ctrl->lock);
> > +		err = cbqri_mon_op(ctrl, CBQRI_CC_MON_CTL_OFF,
> > +				   CBQRI_CC_MON_CTL_OP_CONFIG_EVENT,
> > +				   i, CBQRI_CC_EVT_ID_OCCUPANCY, NULL);
> 
> [Severity: Medium]
> Does mcid_count need bounds checking against the physical hardware limit?
> 
> The ctrl->mcid_count value is used as the loop boundary, but if firmware
> supplies an erroneously large value, this loop could block the CPU for an
> extended period during hotplug. 
> 
> Additionally, because the MCID field mask is 12 bits, any mcid_count larger
> than 4096 will be truncated in cbqri_mon_op(), causing logical MCIDs to
> incorrectly alias to the same physical hardware slot. Should an upper bound be
> enforced on mcid_count?

I'll add a check in v6.

> > +/* mon_ctl field masks (CC and BC share an identical OP/MCID/EVT_ID/STATUS layout) */
> > +#define CBQRI_MON_CTL_OP_MASK        GENMASK(4, 0)
> > +#define CBQRI_MON_CTL_MCID_MASK      GENMASK(19, 8)
> > +#define CBQRI_MON_CTL_EVT_ID_MASK    GENMASK(27, 20)
> 
> [Severity: High]
> Will using GENMASK() here inadvertently clear the upper 32 bits of the 64-bit
> control register on 32-bit architectures?
> 
> When used with FIELD_MODIFY(), ~(_mask) will evaluate to a 32-bit unsigned
> value which gets zero-extended to 64 bits. When this is bitwise ANDed with
> the 64-bit register variable, it will wipe out the upper 32 bits, corrupting
> the STATUS field before the register is written back. Should these 64-bit
> field masks be defined with GENMASK_ULL() instead?

Yes, I will chnage to GENMASK_ULL.

-Drew