Re: [PATCH v3 18/22] accel: ethosu: Validate accumulator input

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Persistent accumulator state causes spurious validation failures for subsequent operations.
- [Medium] Unconditional accumulator format validation rejects valid command streams on U85.
--

commit 1aba4b3d50bf7ffb97c4f8a708b27672c76cbb73
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Validate accumulator input
    
The U85 ACC_FORMAT command can select IFM2 as the accumulator
input. This is used by null-pool operations and can also be used
by convolution. Track this selection and validate the IFM2 feature
map against the OFM extent before submitting the operation.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index f4bd31018e564..632a2352491a0 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -522,6 +523,32 @@ static int feat_matrix_size(struct ethosu_device *edev,
>  					  max_len);
>  }
>  
> +static int
> +calc_acc_input_size(struct drm_device *ddev,
> +		    struct ethosu_validated_cmdstream_info *info,
> +		    struct cmd_state *st)
> +{
> +	struct ethosu_device *edev = to_ethosu_device(ddev);
> +	u64 len;
> +	int ret;
> +
> +	if (!ethosu_is_u65(edev) &&
> +	    !cmd_state_reg_is_set(st, NPU_SET_ACC_FORMAT))
> +		return -EINVAL;

[Severity: Medium]
Does this unconditional check reject valid command streams on U85?

If a user submits a U85 command stream containing only operations that do not
use the accumulator (such as pure elementwise or resize operations), the
compiler may optimize away the unnecessary NPU_SET_ACC_FORMAT register. By
requiring it unconditionally here, the driver will reject these valid command
streams with -EINVAL, preventing userspace from executing certain models.

> +
> +	if (!st->acc_input_ifm2)
> +		return 0;
> +
> +	/* The accumulator has one input value for each OFM element. */
> +	ret = feat_matrix_size(edev, info, st, &st->ifm2,
> +			       FEAT_MATRIX_IFM2, st->ofm.width,
> +			       st->ofm.height[2], st->ofm.depth, false, &len);

[Severity: High]
Can this persistent state cause spurious validation failures for subsequent
operations?

When a user submits a command stream where an operation (like POOL) sets
NPU_SET_ACC_FORMAT to use IFM2, st->acc_input_ifm2 becomes true. If a
subsequent operation (like ELEMENTWISE or RESIZE) ignores the accumulator, the
compiler does not emit a new format register, leaving st->acc_input_ifm2 as
true.

This will validate the previous operation's IFM2 state against the new
operation's OFM dimensions in calc_acc_input_size(), leading to a dimension
mismatch and artificially inflated memory requirements or an -EINVAL rejection.

> +	dev_dbg(ddev->dev, "ACC IFM2:%d:0x%llx-0x%llx\n",
> +		st->ifm2.region, st->ifm2.base[0], len);
> +
> +	return ret;
> +}
> +

[ ... ]

> @@ -692,6 +722,9 @@ static int calc_sizes_elemwise(struct drm_device *ddev,
>  			       true, &len);
>  	dev_dbg(ddev->dev, "op %d: OFM:%d:0x%llx-0x%llx\n",
>  		op, st->ofm.region, st->ofm.base[0], len);
> +	if (ret)
> +		return ret;
> +	ret = calc_acc_input_size(ddev, info, st);

[Severity: High]
Since elementwise operations do not use the accumulator, does calling
calc_acc_input_size() here inadvertently apply stale accumulator state from
previous operations?

>  	if (ret)
>  		return ret;
>  	if (!feat_matrix_chained(edev, &st->ofm))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-ethosu-fixes-v3-0-490fe215286f@kernel.org?part=18