Re: [PATCH v3 4/8] riscv_cbqri: Add capacity controller probe and allocation device ops

[Drew Fustini <fustini@kernel.org>]

On Sun, Jun 28, 2026 at 09:28:09PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Unspecified evaluation order in `cbqri_readq` causes a torn read during hardware polling, leading to false AT capability detection.
> --
> 
> commit a52377aec62b1aa81e5439ad733c3839af6693c6
> Author: Drew Fustini <fustini@kernel.org>
> 
> riscv_cbqri: Add capacity controller probe and allocation device ops
> 
> Add support for the RISC-V CBQRI capacity controller. A platform driver
> passes a cbqri_controller_info descriptor together with the cache level
> to riscv_cbqri_register_cc_dt(), which probes the controller and adds it
> to the controller list.
> 
> > diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> > new file mode 100644
> > index 0000000000000..69df46e07df17
> > --- /dev/null
> > +++ b/drivers/resctrl/cbqri_devices.c
> [ ... ]
> > +static u64 cbqri_readq(void __iomem *addr)
> > +{
> > +	return (u64)readl(addr) | ((u64)readl(addr + 4) << 32);
> > +}
> 
> [Severity: Medium]
> Could the unspecified evaluation order here cause a torn read during hardware
> polling?
> 
> Since the C standard does not specify the evaluation order of the operands
> to the bitwise OR operator, the compiler may emit the lower 32-bit read
> before the upper 32-bit read.
> 
> During the polling loop, if the final iteration reads the lower 32 bits
> while the busy flag is still set, and then the hardware completes the
> operation and clears the busy flag before the upper 32-bit read executes,
> the loop will terminate and return this torn value.

A tear is theoretically possible, but it is harmless here because the
two halves carry independent fields. BUSY and STATUS both live in the
high word, so a single readl(addr + 4) reads both at once. AT lives in
the low word, which the poll never inspects. The poll consumes only BUSY
and STATUS, so a tear cannot change when it ends or the STATUS it
returns.

> [ ... ]
> > +static int cbqri_probe_feature(struct cbqri_controller *ctrl, int reg_offset,
> > +			       int operation, int *status, bool *access_type_supported)
> > +{
> [ ... ]
> > +		writel(lower_32_bits(reg), ctrl->base + reg_offset);
> > +		if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> > +			pr_err("BUSY timeout setting AT field\n");
> > +			return -EIO;
> > +		}
> > +
> > +		/*
> > +		 * If the AT field value has been reset to zero,
> > +		 * then the AT support is not present
> > +		 */
> > +		at = FIELD_GET(CBQRI_CONTROL_REGISTERS_AT_MASK, reg);
> > +		if (at == CBQRI_CONTROL_REGISTERS_AT_CODE)
> > +			*access_type_supported = true;
> 
> [Severity: Medium]
> Does the torn read from cbqri_readq() result in misdetecting capability
> support here?
> 
> If cbqri_wait_busy_flag() returns a torn value with a stale lower half, the
> driver may incorrectly detect Access Type support on hardware that does not
> support it.
> 
> This could break cache allocation isolation guarantees, as the driver will
> split capacity blocks into CODE and DATA masks, but the hardware will silently
> treat them as a single pool.

No. The AT read back is the result of the drivers's own AT=CODE write,
not a live hardware status. A tear cannot catch it mid-change. Per the
CBQRI spec, a controller without AT support makes AT read-only zero. The
write is dropped and AT reads 0, never transiently CODE. On hardware
that does support AT, the write takes effect and AT reads back CODE, so
that direction is correct too.

-Drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv