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

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:

quoted

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

quoted

+
+	return sysfs_emit(buf, "%s\n", available);
+}

[ snip ]

quoted

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

quoted

+		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