Re: [RFC PATCH 4/7] firmware: arm_scmi: Add System Telemetry driver

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

On Sat, Jun 21, 2025 at 12:27:03AM +0300, Dan Carpenter wrote:
> On Fri, Jun 20, 2025 at 08:28:10PM +0100, Cristian Marussi wrote:
> > +//TODO Review available interval show
> > +#define BUF_SZ	1024
> > +static inline ssize_t
> > +__available_update_show(char *buf,
> > +			const struct scmi_telemetry_update_interval *intervals)
> > +{
> > +	int len = 0, num_intervals = intervals->num;
> > +	char available[BUF_SZ];
> > +
> > +	for (int i = 0; i < num_intervals; i++) {
> > +		len += scnprintf(available + len, BUF_SZ - len, "%u ",
> > +				 intervals->update_intervals[i]);
> > +	}
> > +
> > +	available[len - 1] = '\0';
> 
> No need.  scnprintf() will already have put a NUL terminator there.
> Unless num_intervals <= 0 in which case this will corrupt memory.
> 

Yes, all of this routine has to be really reworked to avoid on the stack
allocation too...

> > +
> > +	return sysfs_emit(buf, "%s\n", available);
> > +}
> 
> [ snip ]
> 
> > +static int scmi_telemetry_groups_initialize(struct device *dev,
> > +					    struct scmi_tlm_instance *ti)
> > +{
> > +	int ret;
> > +
> > +	if (ti->info->num_groups == 0)
> > +		return 0;
> > +
> > +	ret = scmi_telemetry_dev_register(&ti->groups_dev, &ti->dev, "groups");
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (int i = 0; i < ti->info->num_groups; i++) {
> > +		const struct scmi_telemetry_group *grp = &ti->info->des_groups[i];
> > +		struct scmi_tlm_grp_dev *gdev;
> > +		char name[16];
> > +
> > +		gdev = devm_kzalloc(dev, sizeof(*gdev), GFP_KERNEL);
> > +		if (!gdev) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +
> > +		gdev->tsp = ti->tsp;
> > +		gdev->grp = grp;
> > +		gdev->dev.groups = scmi_grp_groups;
> > +
> > +		snprintf(name, 8, "%d", grp->id);
> 
> s/8/sizeof(name)/?

indeed...

> 
> > +		ret = scmi_telemetry_dev_register(&gdev->dev,
> > +						  &ti->groups_dev, name);
> > +		if (ret)
> > +			goto err;
> > +
> > +		if (ti->info->per_group_config_support) {
> > +			sysfs_add_file_to_group(&gdev->dev.kobj,
> > +						&dev_attr_grp_current_update.attr,
> > +						NULL);
> > +			sysfs_add_file_to_group(&gdev->dev.kobj,
> > +						&dev_attr_grp_intervals_discrete.attr,
> > +						NULL);
> > +			sysfs_add_file_to_group(&gdev->dev.kobj,
> > +						&dev_attr_grp_available_intervals.attr,
> > +						NULL);
> > +		}
> > +	}
> > +
> > +	dev_info(dev, "Found %d Telemetry GROUPS resources.\n",
> > +		 ti->info->num_groups);
> > +
> > +	return 0;
> > +
> > +err:
> > +	scmi_telemetry_dev_unregister(&ti->groups_dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_telemetry_des_initialize(struct device *dev,
> > +					 struct scmi_tlm_instance *ti)
> > +{
> > +	int ret;
> > +
> > +	ret = scmi_telemetry_dev_register(&ti->des_dev, &ti->dev, "des");
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (int i = 0; i < ti->info->num_de; i++) {
> > +		const struct scmi_telemetry_de *de = ti->info->des[i];
> > +		struct scmi_tlm_de_dev *tdev;
> > +		char name[16];
> > +
> > +		tdev = devm_kzalloc(dev, sizeof(*tdev), GFP_KERNEL);
> > +		if (!tdev) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +
> > +		tdev->tsp = ti->tsp;
> > +		tdev->de = de;
> > +		tdev->dev.groups = scmi_des_groups;
> > +
> > +		/*XXX What about of ID/name digits-length used ? */
> > +		snprintf(name, 8, "0x%04X", de->id);
> 
> s/8/sizeof(name)/?
> 

yes.

> regards,
> dan carpenter
>

Thanks,
Cristian