Re: [RFC PATCH v1 4/8] misc/arm-cla: Initialize devices on CPU bringup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Incomplete hardware reset in cla_op_reset_all() aborts on the first failure, leaving remaining accelerators running during teardown.
- [Medium] cla_reset_ts() incorrectly uses cla_op_regread() to configure the TSOFFOWNER register, clobbering the setup value.
- [Medium] dev->accelerators mask accumulates stale state across CPU hotplug events, causing false device failures.
- [Medium] Missing explicit clearing of the LRESP register in cla_dev_setup() despite the code comment.

Pre-existing issues:
- [High] Integer overflow in cla_op_access_reg() near the 32-bit boundary causes catastrophic underflow of nregs and out-of-bounds memory access.
--

--- Patch [4]: [RFC PATCH v1 4/8] misc/arm-cla: Initialize devices on CPU bringup ---
commit 9a23e1cd42d86f5042ad1e97cc30851dba6b1e3e
Author: Jean-Philippe Brucker <jpb@kernel.org>

misc/arm-cla: Initialize devices on CPU bringup

CLA registers are only accessible from the CPU local to the CLA, so perform
device initialization from the CPU hotplug online callback. Clear the DATA
registers, reset each attached accelerator and verify that it returns to the
expected idle state.

> diff --git a/drivers/misc/arm-cla/cla-init.c b/drivers/misc/arm-cla/cla-init.c
> --- a/drivers/misc/arm-cla/cla-init.c
> +++ b/drivers/misc/arm-cla/cla-init.c
[ ... ]
> +static int cla_reset_ts(struct cla_dev *dev, unsigned int accid)
> +{
[ ... ]
> +	reg = FIELD_PREP(CLA_TSOFFOWNER_PL, cla_kernel_pl);
> +	ret = cla_op_regread(dev, accid, CLA_REG_TSOFFOWNER, 1, &reg);

[Severity: Medium]
Does this code mistakenly call cla_op_regread() instead of cla_op_regwrite()
here in cla_reset_ts()? Doing so will clobber the newly prepared configuration
value with the current hardware state, meaning the kernel fails to claim
ownership of the TSOFFOWNER register. This could cause the subsequent writes
to CLA_REG_TSVOFF and CLA_REG_TSPOFF to be ignored.

> +	if (!ret) {
> +		reg = 0;
> +		ret = cla_op_regwrite(dev, accid, CLA_REG_TSVOFF, 1, &reg);
[ ... ]
> +static int cla_dev_setup(unsigned int cpu)
> +{
[ ... ]
> +	/* Clear DATA and LRESP_DATANZ */
> +	for (i = 0; i < CLA_NUM_DATA_REGS; i++)
> +		cla_reg_write(dev, CLA_REG_DATA(i), 0);

[Severity: Medium]
The comment above indicates this should clear DATA and LRESP_DATANZ, but is
there a missing cla_reg_write() for CLA_REG_LRESP here in cla_dev_setup()? If
left uncleared, stale response data from before the CPU was offlined might
remain and confuse userspace or the driver later.

> +
> +	/*
> +	 * Reset all accelerators. We restrict PLxCTRL to the accelerators that
> +	 * are attached and well behaved.
> +	 */
> +	for (accid = 0; accid < CLA_NUM_ACC; accid++) {
> +		ret = cla_dev_setup_accel(dev, accid);
> +		if (ret > 0)
> +			continue;
> +		else if (ret < 0)
> +			goto err;
> +
> +		dev->accelerators |= (1 << accid);

[Severity: Medium]
Could this accumulate stale state across CPU hotplug events?

Because dev->accelerators is never explicitly cleared to 0 at the start of
cla_dev_setup() or during teardown, if a CPU is offlined and an accelerator
subsequently fails to probe on the next online event, it will incorrectly
remain in the dev->accelerators mask.

During teardown, cla_op_reset_all() might then attempt to reset a non-existent
accelerator, fail, and mark the entire device as broken.

> +		plxctrl_val |= CLA_PLxCTRL_PREP(accid,
> +						FIELD_PREP(CLA_PLxCTRL_AVAIL, 1));
> +	}
[ ... ]
> diff --git a/drivers/misc/arm-cla/cla-ops.c b/drivers/misc/arm-cla/cla-ops.c
> --- a/drivers/misc/arm-cla/cla-ops.c
> +++ b/drivers/misc/arm-cla/cla-ops.c
[ ... ]
> +int cla_op_reset_all(struct cla_dev *dev)
> +{
> +	int ret;
> +	unsigned int accid;
> +
> +	cla_for_each_accid(dev, accid) {
> +		ret = cla_op_reset(dev, accid);
> +		if (ret)
> +			return ret < 0 ? ret : -ENODEV;

[Severity: High]
Does returning immediately on the first error leave the remaining attached
accelerators running?

When cla_op_reset_all() is called during CPU teardown, an early abort means
subsequent accelerators are never quiesced. As the CPU goes offline, these
active hardware accelerators could continue issuing DMA transactions or raising
interrupts, posing a risk of memory corruption.

> +	}
> +	return 0;
> +}
[ ... ]
>  static int cla_op_access_reg(struct cla_dev *dev, u8 op,
>  			     enum cla_launch_data_mode data_mode,
>  			     unsigned int accid, unsigned int regidx,
>  			     size_t nregs, u64 *regs)
>  {
[ ... ]
>  	/* 1 to 8 registers accessed at a time, within the same 8-reg group */
>  	while (nregs > 0) {
>  		unsigned int reg_group = ALIGN_DOWN(regidx, 8);
>  		unsigned int max_reg = min(regidx + nregs, reg_group + 8);
>  		unsigned int ndata = max_reg - regidx;

[Severity: High]
This isn't a bug introduced by this patch, but can the 32-bit addition of
reg_group + 8 wrap around to 0 in cla_op_access_reg()?

If a read is requested near the 32-bit boundary (e.g., regidx = 0xFFFFFFFC
and nregs = 2), reg_group becomes 0xFFFFFFF8. The calculation reg_group + 8
then wraps around to 0 due to 32-bit unsigned arithmetic.

The call to min(regidx + nregs, reg_group + 8) would evaluate to
min(0xFFFFFFFE, 0), returning 0. This causes ndata to become 0 - 0xFFFFFFFC,
which is 4. Subtracting this from the 64-bit size_t nregs causes it to
underflow to a huge value, leading to a massive out-of-bounds loop and kernel
memory corruption.

>  
>  		launch.ndata_m1 = ndata - 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717104759.123203-1-ryan.roberts@arm.com?part=4