Re: [PATCH v15 30/35] firmware: arm_scmi: Add System Telemetry driver

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon,  9 Nov 2026 09:12:16 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Add an SCMI driver to allow enumeration, configuration and data collection
> of SCMI Telemetry data.
> 
> The driver exposes one character device for each found SCMI instance and a
> set of related IOCTLs can be used to enumerate, configure and collect
> Telemetry resources.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Some comments inline.  I'm not even going to pretend to be
alert enough to claim this is a thorough review though.

Jonathan

> diff --git a/drivers/firmware/arm_scmi/scmi_system_telemetry.c b/drivers/firmware/arm_scmi/scmi_system_telemetry.c
> new file mode 100644
> index 000000000000..185c8e646628
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/scmi_system_telemetry.c

> +static long scmi_tlm_des_batch_read_ioctl(const struct scmi_tlm_instance *ti,
> +					  unsigned long arg)
> +{
> +	struct scmi_telemetry_de_sample *samples __free(kvfree) = NULL;

As near as possible to the constructor for an __free declarations.

> +	int ret, *states __free(kvfree) = NULL;

Definitely don't mix __free and non __free declarations.
Separate lines!

> +	const struct scmi_telemetry_res_info *rinfo;
> +	void __user *uptr = (void __user *)arg;
> +	struct scmi_tlm_batch batch = {};
> +	size_t batch_sz, states_sz;
> +
> +	rinfo = scmi_telemetry_res_info_get(ti->tsp);
> +	ret = scmi_tlm_batch_initialize(uptr, &batch, sizeof(*samples),
> +					rinfo->num_des, (void **)&samples,
> +					&batch_sz, &states, &states_sz);
> +	if (ret)
> +		return ret;
> +
> +	for (int i = 0; i < batch.num_items; i++) {
> +		int ret;
> +
> +		ret = ti->tsp->ops->de_data_read(ti->tsp->ph, &samples[i]);
> +		if (ret) {
> +			if (!states)
> +				return ret;
> +
> +			states[i] = ret;
> +			continue;
> +		}
> +	}
> +
> +	/* Return max batch size on zero query */
> +	if (!batch.num_items)
> +		batch.num_items = rinfo->num_des;
> +
> +	return scmi_tlm_batch_finalize(uptr, &batch, samples, states, batch_sz,
> +				       states_sz);
> +}
> +
> +static struct scmi_tlm_shmti_ctx *
> +scmi_tlm_shmti_ctx_alloc(struct scmi_tlm_instance *ti,
> +			 struct scmi_telemetry_shmti_info *shmti)
> +{
> +	struct scmi_tlm_shmti_ctx *ctx;
> +
> +	ctx = kzalloc_obj(*ctx);
> +	if (!ctx)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ctx->shmti = shmti;
> +	kref_init(&ctx->ref);
> +
> +	/* Get hold of the Telemetry instance when returning anon_fds */

This bit of the code isn't returning anon_fds???

> +	get_device(&ti->dev);
> +	ctx->ti = ti;
> +
> +	return ctx;
> +}

> +
> +static int scmi_tlm_shmti_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> +	unsigned long npages, expect, needed, req;
> +	struct scmi_tlm_shmti_ctx *ctx;
> +	phys_addr_t base;
> +	size_t map_sz;
> +	int ret, idx;
> +
> +	if (vma->vm_pgoff)
> +		return -EINVAL;
> +
> +	if (vma->vm_flags & (VM_WRITE | VM_EXEC))
> +		return -EPERM;
> +
> +	vm_flags_clear(vma, VM_MAYWRITE | VM_MAYEXEC);
> +
> +	/* Grab a ctx reference early on */
> +	ctx = scmi_tlm_shmti_ctx_get(filp->private_data);
> +	/*
> +	 * Note that any ctx_put potentially release the device and
> +	 * cleans the SRCU structure...no guards.

No guards is fine but maybe goto and a common cleanup block?

> +	 */
> +	idx = srcu_read_lock(&ctx->ti->dead_srcu);
> +	if (READ_ONCE(ctx->ti->dead)) {
> +		srcu_read_unlock(&ctx->ti->dead_srcu, idx);
> +		scmi_tlm_shmti_ctx_put(ctx);
> +		return -ENODEV;
> +	}
> +
> +	needed = ctx->shmti->len + ctx->shmti->offset;
> +	req = vma->vm_end - vma->vm_start;
> +	if (req < needed) {
> +		srcu_read_unlock(&ctx->ti->dead_srcu, idx);
> +		scmi_tlm_shmti_ctx_put(ctx);
> +		return -EINVAL;
> +	}
> +
> +	/* Align & reject oversized requests */
> +	base = ctx->shmti->phys & PAGE_MASK;
> +	expect = DIV_ROUND_UP(needed, PAGE_SIZE);
> +	npages = req >> PAGE_SHIFT;
> +	if (npages > expect) {
> +		srcu_read_unlock(&ctx->ti->dead_srcu, idx);
> +		scmi_tlm_shmti_ctx_put(ctx);
> +		return -EINVAL;
> +	}
> +
> +	map_sz = PAGE_ALIGN(needed);
> +	vma->vm_private_data = ctx;
> +	vma->vm_ops = &scmi_tlm_shmti_vm_ops;
> +	/* Set appropriate caching attributes */
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +
> +	/* Map physical address range into user space */
> +	ret = vm_iomap_memory(vma, base, map_sz);
> +	if (ret) {
> +		srcu_read_unlock(&ctx->ti->dead_srcu, idx);
> +		scmi_tlm_shmti_ctx_put(ctx);
> +		return ret;
> +	}
> +
> +	srcu_read_unlock(&ctx->ti->dead_srcu, idx);
> +
> +	return 0;
> +}

> +static void
> +scmi_tlm_allocate_anon_fds(struct scmi_tlm_instance *ti, unsigned int *num_shmtis,
> +			   struct scmi_tlm_shmti_info *shinfo,
> +			   struct file **filps)
> +{
> +	const struct scmi_telemetry_info *info = ti->info;
> +	struct device *dev = &ti->tsp->sdev->dev;
> +	int idx = 0;
> +
> +	for (int i = 0; i < *num_shmtis; i++) {
> +		struct scmi_telemetry_shmti_info *shmti = info->shmtis[i];
> +		struct scmi_tlm_shmti_ctx *ctx;
> +		struct file *filp;
> +		int fd;
> +
> +		fd = get_unused_fd_flags(O_CLOEXEC);

Bit marginal, but perhaps some __free magic for this.

> +		if (fd < 0) {
> +			dev_err(dev, "Failed GET_UNUSED_FD for SID: %u\n",
> +				shmti->sid);
> +			continue;
> +		}
> +
> +		ctx = scmi_tlm_shmti_ctx_alloc(ti, shmti);
> +		if (IS_ERR(ctx)) {
> +			dev_err(dev, "Failed CTX_ALLOC for SID: %u\n", shmti->sid);
> +			put_unused_fd(fd);
> +			continue;
> +		}
> +
> +		filp = anon_inode_getfile(SCMI_TLM_DRIVER, &scmi_tlm_shmti_fops,
> +					  ctx, O_RDONLY | O_CLOEXEC);
> +		if (IS_ERR(filp)) {
> +			scmi_tlm_shmti_ctx_put(ctx);

Bit fiddlier but maybe some __free magic for ctx as well.

> +			put_unused_fd(fd);
> +			dev_err(dev, "Failed ANON GET_FILE for SID: %u\n",
> +				shmti->sid);
> +			continue;
> +		}
> +
> +		shinfo[idx].sid = shmti->sid;
> +		shinfo[idx].fd = fd;
> +		shinfo[idx].len = shmti->len;
> +		shinfo[idx].offset = shmti->offset;
> +
> +		filps[idx++] = filp;
> +	}
> +
> +	/* Return effective initialized items */
> +	*num_shmtis = idx;
> +}

> +
> +static long
> +scmi_tlm_shmtis_list_get_ioctl(struct scmi_tlm_instance *ti, unsigned long arg)
> +{
> +	struct scmi_tlm_shmti_info *shinfo __free(kvfree) = NULL;

Move down to where it's constructed.  cleanup.h talks about this.

> +	unsigned int max_shmtis = ti->info->base.num_shmtis;
> +	void __user *uptr = (void __user *)arg;
> +	struct scmi_tlm_batch batch = {};
> +	size_t batch_sz;
> +	int ret;
> +
> +	ret = scmi_tlm_batch_initialize(uptr, &batch, sizeof(*shinfo), max_shmtis,
> +					(void **)&shinfo, &batch_sz, NULL, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (batch.num_items == 0) {
> +		batch.num_items = max_shmtis;
> +
> +		return scmi_tlm_batch_finalize(uptr, &batch, shinfo, NULL,
> +					       batch_sz, 0);
> +	}
> +
> +	struct file **filps __free(kfree) = kzalloc_objs(*filps, batch.num_items);
> +	if (!filps)
> +		return -ENOMEM;
> +
> +	/* Passing the array as a param avoids dancing with uptr->items */
> +	scmi_tlm_allocate_anon_fds(ti, &batch.num_items, shinfo, filps);
> +	ret = scmi_tlm_batch_finalize(uptr, &batch, shinfo, NULL, batch_sz, 0);
> +	if (ret) {
> +		scmi_tlm_anon_fds_free(batch.num_items, shinfo, filps);
> +		return ret;
> +	}
> +
> +	scmi_tlm_anon_fds_install(batch.num_items, shinfo, filps);
> +
> +	return 0;
> +}

> +
> +static long
> +scmi_tlm_uuid_list_get_ioctl(const struct scmi_tlm_instance *ti,
> +			     unsigned long arg)
> +{
> +	struct scmi_tlm_uuid *uuids __free(kvfree) = NULL;

Move this down next to where the constructor is.  See cleanup.h guidance
on why they should be together.

> +	void __user *uptr = (void __user *)arg;
> +	struct scmi_tlm_batch batch = {};
> +	unsigned int num_uuids;
> +	uuid_t **cur_uuids;
> +	size_t batch_sz;
> +	int ret;
> +
> +	/*
> +	 * Note that UUID array can ONLY grow and it has NOT a known upper
> +	 * bound, so it does not matter if by the time we grab the cur_uuids
> +	 * via rcu_dereference() the array has grown in size and we end up
> +	 * here reading a stale/incomplete set of UUIDs, we will simply
> +	 * read the new UUIDs next-time: this simplifies allocation.
> +	 */
> +	num_uuids = smp_load_acquire(&ti->info->num_uuids);
> +	ret = scmi_tlm_batch_initialize(uptr, &batch, sizeof(*uuids),
> +					num_uuids, (void **)&uuids, &batch_sz,
> +					NULL, NULL);
> +	if (ret)
> +		return ret;
> +
> +	rcu_read_lock();
> +	cur_uuids = rcu_dereference(ti->info->uuids);
> +	for (int j = 0; j < batch.num_items; j++)
> +		export_uuid(uuids[j].bytes, cur_uuids[j]);
> +	rcu_read_unlock();
> +
> +	/* Return max batch size on zero query */
> +	if (!batch.num_items)
> +		batch.num_items = num_uuids;
> +
> +	return scmi_tlm_batch_finalize(uptr, &batch, uuids, NULL, batch_sz, 0);
> +}
> +
> +static int scmi_tlm_event_subscribe(struct scmi_tlm_instance *ti,
> +				    struct scmi_tlm_event *evt, void *key)
> +{
> +	struct scmi_tlm_events_session *session;
> +	struct scmi_tlm_registered_event *revt;
> +	struct eventfd_ctx *ctx;
> +	u32 cookie;
> +	int ret;
> +
> +	scoped_guard(mutex, &ti->sessions_mtx) {
> +		session = xa_load(&ti->sessions_xa, (unsigned long)key);
> +		if (!session) {
> +			/*
> +			 * Initialize a new session on first call.
> +			 * Once created successfully initialized sessions are
> +			 * freed only on final release.
> +			 */
> +			session = kzalloc_obj(*session);
> +			if (!session)
> +				return -ENOMEM;
> +
> +			/* Initialize inner with proper flags to support xa_alloc */
> +			xa_init_flags(&session->events_xa, XA_FLAGS_ALLOC);
> +			mutex_init(&session->events_mtx);
> +			ret = xa_insert(&ti->sessions_xa, (unsigned long)key,
> +					session, GFP_KERNEL);
> +			if (ret) {
> +				xa_destroy(&session->events_xa);
> +				kfree(session);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	ctx = eventfd_ctx_fdget(evt->efd);
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
> +
> +	revt = kzalloc_obj(*revt);
> +	if (!revt) {
> +		eventfd_ctx_put(ctx);
> +		return -ENOMEM;
> +	}
> +
> +	guard(mutex)(&session->events_mtx);
> +	revt->ctx = ctx;
> +	revt->type = evt->type;
> +	ret = xa_alloc(&session->events_xa, &cookie, revt,
> +		       scmi_evt_xa_limit, GFP_KERNEL);
> +	if (ret) {
> +		eventfd_ctx_put(revt->ctx);
> +		kfree(revt);
> +		return ret;
> +	}
> +
> +	ret = ti->tsp->ops->event_subscribe(ti->tsp->ph, revt->type, revt->ctx);
> +	if (ret) {
> +		xa_erase(&session->events_xa, cookie);
> +		eventfd_ctx_put(revt->ctx);
> +		kfree(revt);
> +		return ret;
Quite a bit of repetition in the error paths. Use a common goto
block to clean up.  For the guard stuff above, a helper is probably
the easiest way to avoid mixing that with cleanup via gotos down here.

> +	}
> +
> +	/* Return the allocated cookie */
> +	evt->cookie = cookie;
> +
> +	/* Get hold of the instance */
> +	get_device(&ti->dev);
> +
> +	return 0;
> +}

> +
> +static long scmi_tlm_unlocked_ioctl(struct file *filp, unsigned int cmd,
> +				    unsigned long arg)
> +{
> +	struct scmi_tlm_instance *ti = filp->private_data;
> +	bool writable = filp->f_mode & FMODE_WRITE;
> +
> +	guard(srcu)(&ti->dead_srcu);
> +	if (READ_ONCE(ti->dead))
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case SCMI_TLM_GET_ABI_INFO:
> +		return scmi_tlm_abi_info_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_CFG:
> +		return scmi_tlm_config_get_ioctl(ti, arg);
> +	case SCMI_TLM_SET_CFG:
> +		if (writable)
> +			return scmi_tlm_config_set_ioctl(ti, arg);
> +		break;
> +	case SCMI_TLM_GET_INTRVS:
> +		return scmi_tlm_intervals_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_DE_CFG:
> +		return scmi_tlm_de_config_get_ioctl(ti, arg);
> +	case SCMI_TLM_SET_DE_CFG:
> +		if (writable)
> +			return scmi_tlm_de_config_set_ioctl(ti, arg);
> +		break;
> +	case SCMI_TLM_GET_DE_INFO:
> +		return scmi_tlm_de_info_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_DE_LIST:
> +		return scmi_tlm_des_list_get_ioctl(ti, arg);
> +	case SCMI_TLM_DE_READ:
> +		return scmi_tlm_de_value_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_ALL_CFG:
> +		return scmi_tlm_all_de_config_get_ioctl(ti, arg);
> +	case SCMI_TLM_SET_ALL_CFG:
> +		if (writable)
> +			return scmi_tlm_all_de_config_set_ioctl(ti, arg);
> +		break;
> +	case SCMI_TLM_GET_GRP_LIST:
> +		return scmi_tlm_grps_list_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_GRP_INFO:
> +		return scmi_tlm_grp_info_get_ioctl(ti, arg);
> +	case SCMI_TLM_GET_GRP_DESC:
> +		return scmi_tlm_grp_desc_get_ioctl(ti, arg);
> +	case SCMI_TLM_SINGLE_READ:
> +		return scmi_tlm_des_read_ioctl(ti, arg, true);
> +	case SCMI_TLM_BULK_READ:
> +		return scmi_tlm_des_read_ioctl(ti, arg, false);
> +	case SCMI_TLM_BATCH_READ:
> +		return scmi_tlm_des_batch_read_ioctl(ti, arg);
> +	case SCMI_TLM_GET_SHMTI_LIST:
> +		return scmi_tlm_shmtis_list_get_ioctl(ti, arg);
> +	case SCMI_TLM_RESET:
> +		if (writable)
> +			return scmi_tlm_reset_ioctl(ti, arg);
> +		break;
> +	case SCMI_TLM_GET_UUID_LIST:
> +		return scmi_tlm_uuid_list_get_ioctl(ti, arg);
> +	case SCMI_TLM_EVENT_SUBSCRIBE:
> +		return scmi_tlm_event_subscribe_ioctl(ti, arg, filp);
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	return -EPERM;

I'd just return this where you break above.
Seems little benefit in coming down here to do so given they
are all error paths.

> +}

> +
> +static int scmi_telemetry_probe(struct scmi_device *sdev)
> +{
> +	const struct scmi_handle *handle = sdev->handle;
> +	struct scmi_protocol_handle *ph;
> +	struct scmi_tlm_instance *ti;
> +	unsigned int instance;
> +	const void *ops;
> +	int ret;
> +
> +	if (!handle)
> +		return -ENODEV;
> +
> +	instance = handle->id;
> +	if (instance >= SCMI_TLM_NUM_DEVS)
> +		return -ENOSPC;
> +
> +	ops = handle->devm_protocol_get(sdev, sdev->protocol_id, &ph);
> +	if (IS_ERR(ops))
> +		return dev_err_probe(&sdev->dev, PTR_ERR(ops),
> +				     "Cannot access protocol:0x%X\n",
> +				     sdev->protocol_id);
> +
> +	ti = scmi_tlm_instance_init(sdev, ops, ph, instance);
> +	if (IS_ERR(ti))
> +		return PTR_ERR(ti);
> +
> +	/*
> +	 * Setting the platform device as parent to enforce proper ordering
> +	 * at shutdown and to avoid the need for explicit get/put_device
> +	 */
> +	ti->dev.parent = &sdev->dev;
> +	ti->dev.devt = MKDEV(MAJOR(stlm_devt), MINOR(stlm_devt) + instance);
> +	ti->dev.class = &stlm_class;
> +	ti->dev.release = scmi_tlm_device_release;
> +	device_initialize(&ti->dev);
> +
> +	ret = dev_set_name(&ti->dev, "tlm_%d", ti->id);
> +	if (ret)
> +		goto err_dev;
> +
> +	cdev_init(&ti->cdev, &stlm_fops);
> +	ti->cdev.owner = THIS_MODULE;
> +
> +	ret = cdev_device_add(&ti->cdev, &ti->dev);
> +	if (ret)
> +		goto err_dev;
> +
> +	dev_set_drvdata(&sdev->dev, ti);
> +
> +	return 0;
> +
> +err_dev:
> +	put_device(&ti->dev);
> +
> +	return ret;
> +}
> +
> +static void scmi_telemetry_remove(struct scmi_device *sdev)
> +{
> +	struct scmi_tlm_events_session *session;
> +	struct scmi_tlm_instance *ti;
> +	unsigned long index;
> +
> +	ti = dev_get_drvdata(&sdev->dev);
> +	if (!ti)
> +		return;
> +
> +	/* Stop ongoing opens and wait on in-flight operation to drain */
> +	WRITE_ONCE(ti->dead, true);
> +	synchronize_srcu(&ti->dead_srcu);
> +
> +	/* Inhibit future opens */
> +	cdev_device_del(&ti->cdev, &ti->dev);
> +	/*
> +	 * Drop any residual session and related events if unbind happened whiel

while

> +	 * some event fds were still open.
> +	 */
> +	mutex_lock(&ti->sessions_mtx);
> +	xa_for_each(&ti->sessions_xa, index, session) {
> +		xa_erase(&ti->sessions_xa, index);
> +		/* Force any event unsubscribe...protocol is still alive */
> +		scmi_tlm_events_session_unsubscribe(ti, session);
> +	}
> +	mutex_unlock(&ti->sessions_mtx);
> +
> +	put_device(&ti->dev);
> +}

> +
> +static int __init scmi_telemetry_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&stlm_devt, 0, SCMI_TLM_NUM_DEVS,
> +				  SCMI_TLM_DRIVER);
> +	if (ret)
> +		return ret;
> +
> +	ret = class_register(&stlm_class);
> +	if (ret)
> +		goto err_class;
> +
> +	ret = scmi_register(&scmi_telemetry_driver);
> +	if (ret)
> +		goto err_scmi;
> +
> +	return 0;
> +
> +err_scmi:
> +	class_unregister(&stlm_class);
> +
> +err_class:
> +	unregister_chrdev_region(stlm_devt, SCMI_TLM_NUM_DEVS);
> +
> +	return ret;
> +}
> +module_init(scmi_telemetry_init);
> +
> +static void __exit scmi_telemetry_exit(void)
> +{
> +	scmi_unregister(&scmi_telemetry_driver);
> +	unregister_chrdev_region(stlm_devt, SCMI_TLM_NUM_DEVS);

This should be reverse order of init == same as error paths
at the end of init. Anything else needs some comments on why.

> +	class_unregister(&stlm_class);
> +}
> +module_exit(scmi_telemetry_exit);