On Fri, Jul 17, 2026 at 10:58:47AM +0000, [email protected] wrote:
> > + reg = FIELD_PREP(CLA_TSOFFOWNER_PL, cla_kernel_pl);
> > + ret = cla_op_regread(dev, accid, CLA_REG_TSOFFOWNER, 1, ®);
>
> [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, ®);
> [ ... ]
> > +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.
