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

[sashiko-bot@kernel.org]

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.

[ ... ]
> +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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-dfustini-atl-sc-cbqri-dt-v3-0-c9c1342fe3cf@kernel.org?part=4