Re: [PATCH 07/10] firmware: arm_scmi: Add System Telemetry ioctls support

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 25 Sep 2025 21:35:51 +0100
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Extend the filesystem based interface with special 'control' file that can
> be used to configure and retrieve SCMI Telemetry data in binary form using
> the alternative ioctls-based ABI described in uapi/linux/scmi.h.
Why you say alternative.  Why do you need both?

That's in the cover letter but I'd put something here as well.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../firmware/arm_scmi/scmi_system_telemetry.c | 402 ++++++++++++++++++
>  1 file changed, 402 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/scmi_system_telemetry.c b/drivers/firmware/arm_scmi/scmi_system_telemetry.c
> index 2fec465b0f33..f591aad10302 100644
> --- a/drivers/firmware/arm_scmi/scmi_system_telemetry.c
> +++ b/drivers/firmware/arm_scmi/scmi_system_telemetry.c

> +static long scmi_tlm_des_read_ioctl(const struct scmi_tlm_inode *tlmi,
> +				    unsigned long arg, bool single,
> +				    bool is_group)
> +{
> +	const struct scmi_tlm_setup *tsp = tlmi->tsp;
> +	void * __user uptr = (void * __user)arg;
> +	struct scmi_tlm_data_read bulk, *bulk_ptr;
> +	int ret, grp_id = SCMI_TLM_GRP_INVALID;
> +
> +	if (copy_from_user(&bulk, uptr, sizeof(bulk)))
> +		return -EFAULT;
> +
> +	bulk_ptr = kzalloc(struct_size(bulk_ptr, samples, bulk.num_samples),

__free() would help here.

> +			   GFP_KERNEL);
> +	if (!bulk_ptr)
> +		return -ENOMEM;
> +
> +	if (is_group) {
> +		const struct scmi_telemetry_group *grp = tlmi->priv;
> +
> +		grp_id = grp->info->id;
> +	}
> +
> +	bulk_ptr->num_samples = bulk.num_samples;
> +	if (!single)
> +		ret = tsp->ops->des_bulk_read(tsp->ph, grp_id,
> +					      &bulk_ptr->num_samples,
> +			  (struct scmi_telemetry_de_sample *)bulk_ptr->samples);
> +	else
> +		ret = tsp->ops->des_sample_get(tsp->ph, grp_id,
> +					       &bulk_ptr->num_samples,
> +			  (struct scmi_telemetry_de_sample *)bulk_ptr->samples);

That is very unusual code alignment.  Drag 2 lines above left to match one line above.

> +	if (ret)
> +		goto out;
> +
> +	if (copy_to_user(uptr, bulk_ptr, sizeof(*bulk_ptr) +
> +			 bulk_ptr->num_samples * sizeof(bulk_ptr->samples[0])))
> +		ret = -EFAULT;
> +
> +out:
> +	kfree(bulk_ptr);
> +
> +	return ret;
> +}