Re: [PATCH v15 04/35] firmware: arm_scmi: Allow registration of unknown-size events/reports

[Jonathan Cameron]

On Mon,  9 Nov 2026 09:11:50 +0000
Cristian Marussi <cristian.marussi@xxxxxxx> wrote:

> Allow protocols to register events with build-time unknown sizes: such
> events can be declared zero-sized and let the core SCMI stack perform the
> needed safe-net boundary checks based on the configured transport size.
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> ---
> v14 --> v15
>  - better commenting

A few bits of feedback on the comments.

> v12 --> v13
>  - improve variable naming
> v10 --> v11
>  - account for ktime_t field when checking boundary
> v2 --> v3
>  - split out of previous patch on protocol notifier
>  - use max() instead of max_t()
> ---
>  drivers/firmware/arm_scmi/notify.c | 28 +++++++++++++++++++++++-----
>  drivers/firmware/arm_scmi/notify.h |  8 ++++++--
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index bc046f2f83a6..a30589c9b377 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -595,7 +595,16 @@ int scmi_notify(const struct scmi_handle *handle, u8 proto_id, u8 evt_id,
>  	if (!r_evt)
>  		return -EINVAL;
>  
> -	if (len > r_evt->evt->max_payld_sz) {
> +	/*
> +	 * Events with a zero max_payld_sz are sized to be of the maximum
> +	 * size allowed by the transport: no need to be size-checked here
> +	 * since the transport layer would have already dropped such
> +	 * over-sized messages.

'such' here to me refers to the first bit. That is, those that are "
sized to be the maxim size allowed by the transport.

I'd just drop 'such' and have
	 * since the transport layer would have already dropped over-sized
	 * messages.

as that avoids this potential confusion.

> +	 * Note that, though, while this protects from buffer overruns, it is

Drop 'though'. Doesn't add anything and leads to odd flow to the sentence

> +	 * no more possible at this layer of the stack to detect zero-sized

'not possible at this layer ..'

The 'no more' refers to code that isn't here if someone reads this code
once the patch is merged.  So if you want to refer to what changed, keep
it in the patch description not a comment.


> +	 * events corrupted by truncation.
> +	 */
> +	if (r_evt->evt->max_payld_sz && len > r_evt->evt->max_payld_sz) {
>  		dev_err(handle->dev, "discard badly sized message\n");
>  		return -EINVAL;
>  	}