Re: [RFC PATCH v1 5/8] misc/arm-cla: Accelerator context save and restore

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

On Fri, Jul 17, 2026 at 10:58:10AM +0000, sashiko-bot@kernel.org wrote:
> > +	regstate_size = FIELD_GET(CLA_SRSTATE_0_REGSTATE, srstate[0]);
> > +	if (WARN_ON(*regstate_off + regstate_size * 8 > dev->iassizes))
> > +		return -ENOSPC;
> 
> [Severity: High]
> Does this bounds check correctly handle the units for *regstate_off?
> 
> *regstate_off is incremented by regstate_size (which is a register count),
> so it is also a register count.
> 
> If dev->iassizes is in bytes, this check improperly adds a register count
> (*regstate_off) to a byte count (regstate_size * 8) and compares it against
> a byte count.

Yes this should be WARN_ON((*regstate_off + regstate_size) * 8 > dev->iassizes)


> > +int cla_regs_switch_out(struct cla_dev *dev, struct cla_regs *regs,
> > +			bool save_regs)
> > +{
> [ ... ]
> > +	if (save_regs) {
> > +		for (i = 0; i < CLA_NUM_DATA_REGS; i++)
> > +			regs->data[i] = cla_reg_read(dev, CLA_REG_DATA(i));
> > +
> > +		cla_for_each_accid(dev, accid) {
> > +			ret = cla_regs_save_accel(dev, accid, regs, &regstate_off);
> > +			if (ret)
> > +				return ret;
> 
> [Severity: Medium]
> If cla_regs_save_accel() fails here, the function returns without clearing
> regs->accel_valid.
> 
> Since regs is allocated once per domain and reused, accel_valid might
> already be true from a previous successful save. Returning an error here
> leaves the structure partially overwritten (a mix of old and new accelerator
> states) but still marked as valid.
> 
> If the caller handles this switch-out failure non-fatally and retries the
> context later, will cla_regs_switch_in() restore this corrupted state to
> the hardware?

Switch-out failure is fatal, requires at least module reload


> > +		}
> > +
> > +		regs->accel_valid = true;
> > +	}
> [ ... ]
> > +struct cla_regs **cla_regs_alloc_domain(struct cla_domain *domain)
> > +{
> [ ... ]
> > +	for (i = 0; i < domain->nr_devs; i++) {
> > +		struct cla_regs *regs;
> > +		struct cla_dev *dev = domain->devs[i];
> > +
> > +		/*
> > +		 * The regs structures are only ever accessed from the CLA
> > +		 * device's CPU, so try to allocate them on the right NUMA node
> > +		 */
> > +		size = sizeof(*regs) + dev->iassizes;
> 
> [Severity: High]
> If dev->iassizes represents a register count rather than a byte count,
> should it be scaled by sizeof(u64) here?

iassizes is a byte count