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

[Dan Carpenter <dan.carpenter@linaro.org>]

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.

> +
> +	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)/?

> +		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)/?

regards,
dan carpenter

> +		ret = scmi_telemetry_dev_register(&tdev->dev,
> +						  &ti->des_dev, name);
> +		if (ret)
> +			goto err;
> +
> +		if (de->name)
> +			sysfs_add_file_to_group(&tdev->dev.kobj,
> +						&dev_attr_name.attr, NULL);
> +		if (de->tstamp_support) {
> +			sysfs_add_file_to_group(&tdev->dev.kobj,
> +						&dev_attr_tstamp_exp.attr,
> +						NULL);
> +			sysfs_add_file_to_group(&tdev->dev.kobj,
> +						&dev_attr_tstamp_enable.attr,
> +						NULL);
> +		}
> +	}
> +
> +	dev_info(dev, "Found %d Telemetry DE resources.\n",
> +		 ti->info->num_de);
> +
> +	return 0;
> +
> +err:
> +	scmi_telemetry_dev_unregister(&ti->des_dev);
> +
> +	return ret;
> +}