Re: [PATCH v4 30/31] accel: ethosu: Require active weight streams setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel: ethosu: skip inactive streams in buffer_size() when length was set
--

commit 11fbe055c6b0770ecac837f4b9d39668ea40786d
Author: Rob Herring (Arm) <robh@kernel.org>

accel: ethosu: Require active weight streams setup

This commit requires every active weight or scale stream to be explicitly
configured or disabled with a zero length, deriving the active stream count
from the NPU configuration on U85 and from NPU_SET_PARALLEL_MODE on U65.

> diff --git a/drivers/accel/ethosu/ethosu_gem.c b/drivers/accel/ethosu/ethosu_gem.c
> index 1935440fb815b..21458aacfcad6 100644
> --- a/drivers/accel/ethosu/ethosu_gem.c
> +++ b/drivers/accel/ethosu/ethosu_gem.c

[ ... ]

> @@ -767,19 +768,45 @@ static int calc_sizes_resize(struct drm_device *ddev,
>  	return 0;
>  }
>  
> +static int weight_streams(struct ethosu_device *edev, struct cmd_state *st,
> +			  u8 *streams)
> +{
> +	if (ethosu_is_u65(edev)) {
> +		if (FIELD_GET(CONFIG_MACS_PER_CC_MASK, edev->npu_info.config) ==
> +		    NPU_MACS_PER_CC_256) {
> +			*streams = 1;
> +			return 0;
> +		}
> +
> +		if (!cmd_state_reg_is_set(st, NPU_SET_PARALLEL_MODE))
> +			return -EINVAL;
> +
> +		*streams = st->parallel_mode + 1;
> +	} else {
> +		*streams = 1 << FIELD_GET(CONFIG_NUM_WD_MASK,
> +					   edev->npu_info.config);
> +	}
> +
> +	return 0;
> +}
> +
>  static int buffer_size(struct ethosu_validated_cmdstream_info *info,
>  		       struct cmd_state *st, struct buffer *buf, s8 region,
> -		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool optional)
> +		       u16 region_cmd, u16 base_cmd, u16 length_cmd, bool can_omit)
>  {
>  	u64 end;
>  	bool base_set = cmd_state_reg_is_set(st, base_cmd);
>  	bool length_set = cmd_state_reg_is_set(st, length_cmd);
>  
> -	if (optional && !base_set && !length_set)
> +	if (can_omit && !length_set)
>  		return 0;

[Severity: Medium]
Should buffer_size() return 0 whenever can_omit is true, regardless of
whether length_set is true?

On Ethos-U65 with 512 MACs/cc, weight_streams() computes the active stream
count per operation from st->parallel_mode (*streams = st->parallel_mode + 1).

If an operation in a command stream runs with NPU_SET_PARALLEL_MODE set to 1
(streams == 2), it configures NPU_SET_WEIGHT1_BASE / NPU_SET_WEIGHT1_LENGTH
and NPU_SET_SCALE1_BASE / NPU_SET_SCALE1_LENGTH. In
ethosu_gem_cmdstream_copy_and_validate(), cmd_state_set_reg() sets the
corresponding bits in the cumulative st.cmd1 bitmap (which is never cleared
during the command stream) and populates st.weight[1] and st.scale[1].

If a subsequent operation in the same command stream switches to
NPU_SET_PARALLEL_MODE = 0 (streams == 1) and changes NPU_SET_WEIGHT_REGION or
NPU_SET_SCALE_REGION (for example, to SRAM or a smaller buffer), stream 1 is
inactive and calc_sizes() passes can_omit = (streams < 2) as true:

calc_sizes()
	buffer_size(info, st, &st->weight[1], st->weight[0].region,
		    NPU_SET_WEIGHT_REGION, NPU_SET_WEIGHT1_BASE,
		    NPU_SET_WEIGHT1_LENGTH, streams < 2)

	buffer_size(info, st, &st->scale[1], st->scale[0].region,
		    NPU_SET_SCALE_REGION, NPU_SET_SCALE1_BASE,
		    NPU_SET_SCALE1_LENGTH, streams < 2)

Because length_set remains true in st.cmd1 from the earlier operation,
buffer_size() will not return early at "if (can_omit && !length_set)". Will
this charge the stale st->weight[1] and st->scale[1] bounds against the new
operation's st->weight[0].region and st->scale[0].region, inflating
info->region_size[region] and causing valid job submissions to be rejected in
ethosu_ioctl_submit_job()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-ethosu-fixes-v4-0-4aa25e22578e@kernel.org?part=30