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

[Jean-Philippe Brucker <jpb@kernel.org>]

On Fri, Jul 17, 2026 at 10:58:47AM +0000, sashiko-bot@kernel.org wrote:
> > +	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.

Yes


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

No, DATANZ is tied to DATA


> > +
> > +	/*
> > +	 * 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.

The FW does not change the visible accelerators across CPUHP events


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

I wouldn't put too much effort into handling HW failures because they
normally don't happen at this level, but finishing the iteration
even in case of failure seems simple enough here.