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

[Cristian Marussi <cristian.marussi@arm.com>]

On Mon, Oct 20, 2025 at 05:30:34PM +0100, Jonathan Cameron wrote:
> 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.

Ok..indeed all my babbling in teh cover wont be anywhere once merged.

> > 
> > 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.
> 

Ok.

> > +			   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.

Trying to please checkpatch while staying under 80 cols...BUT I know is NOT a
good reason for the above mess...

Thanks,
Cristian