Re: [PATCH v2 07/15] accel/qda: Add memory manager for CB devices

On Mon, Aug 17, 2026 at 10:17:42AM +0530, Ekansh Gupta wrote:
> Introduce the QDA memory manager (qda_memory_manager) to track the
> IOMMU devices that back each compute context bank (CB).
> 
> Each CB device registered on the qda-compute-cb bus is wrapped in a
> qda_iommu_device descriptor recording the device pointer and its stream
> ID, and stored in the memory manager's registry. Later patches use this
> registry to resolve the IOMMU device a session should allocate from.
> 
> The registry is a plain array sized to the number of
> "qcom,fastrpc-compute-cb" nodes present in the device tree: the RPMsg
> probe counts those nodes and passes the count to qda_init_device(),
> which allocates the array in qda_memory_manager_init(). The memory
> manager is created before CB devices are populated and destroyed after
> they are torn down, so no dangling descriptors remain.
> 
> qda_cb_setup_device() is called immediately after a CB device is
> registered on the bus: it allocates the descriptor, registers it with
> the memory manager, and stores it as the CB device's driver data so
> that qda_destroy_cb_device() can unregister and free it during teardown.
> 
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> Changes in v2:
> - Replace the XArray with a plain array sized to the device tree's CB
>   node count instead of a fixed 16-entry table (Dmitry Baryshkov)
> - Fold the init_memory_manager()/cleanup_memory_manager() wrappers into
>   qda_init_device()/qda_deinit_device() (Dmitry Baryshkov)
> - Drop the pr_debug() calls (Dmitry Baryshkov)
> - Use goto labels to unwind probe failures instead of open-coding the
>   cleanup at each error site (Dmitry Baryshkov)
> ---
>  drivers/accel/qda/Makefile             |  1 +
>  drivers/accel/qda/qda_cb.c             | 39 ++++++++++++++
>  drivers/accel/qda/qda_drv.c            | 26 +++++++++
>  drivers/accel/qda/qda_drv.h            |  5 ++
>  drivers/accel/qda/qda_memory_manager.c | 98 ++++++++++++++++++++++++++++++++++
>  drivers/accel/qda/qda_memory_manager.h | 55 +++++++++++++++++++
>  drivers/accel/qda/qda_rpmsg.c          | 23 +++++++-
>  7 files changed, 246 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/accel/qda/Makefile b/drivers/accel/qda/Makefile
> index 143c9e4e789e..701fad5ffb50 100644
> --- a/drivers/accel/qda/Makefile
> +++ b/drivers/accel/qda/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_ACCEL_QDA)	:= qda.o
>  qda-y := \
>  	qda_cb.o \
>  	qda_drv.o \
> +	qda_memory_manager.o \
>  	qda_rpmsg.o
>  
>  obj-$(CONFIG_DRM_ACCEL_QDA_COMPUTE_BUS) += qda_compute_bus.o
> diff --git a/drivers/accel/qda/qda_cb.c b/drivers/accel/qda/qda_cb.c
> index 9c5ef32f351e..cd32d6df6318 100644
> --- a/drivers/accel/qda/qda_cb.c
> +++ b/drivers/accel/qda/qda_cb.c
> @@ -8,13 +8,40 @@
>  #include <linux/slab.h>
>  #include <drm/drm_print.h>
>  #include "qda_drv.h"
> +#include "qda_memory_manager.h"
>  #include "qda_cb.h"
>  
> +static int qda_cb_setup_device(struct qda_dev *qdev, struct device *cb_dev, u32 sid)
> +{
> +	struct qda_iommu_device *iommu_dev;
> +	int ret;
> +
> +	iommu_dev = kzalloc_obj(*iommu_dev);
> +	if (!iommu_dev)
> +		return -ENOMEM;
> +
> +	iommu_dev->dev = cb_dev;
> +	iommu_dev->qdev = qdev;
> +	iommu_dev->sid = sid;
> +
> +	ret = qda_memory_manager_register_device(qdev->iommu_mgr, iommu_dev);
> +	if (ret) {
> +		drm_err(&qdev->drm_dev, "Failed to register IOMMU device: %d\n", ret);
> +		kfree(iommu_dev);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(cb_dev, iommu_dev);
> +
> +	return 0;
> +}
> +
>  int qda_create_cb_device(struct qda_dev *qdev, struct device_node *cb_node)
>  {
>  	struct qda_cb_dev *entry;
>  	struct device *cb_dev;
>  	u32 sid = 0;
> +	int ret;
>  
>  	if (of_property_read_u32(cb_node, "reg", &sid))
>  		drm_info(&qdev->drm_dev, "No stream ID in DT node %pOFn, using 0\n", cb_node);
> @@ -27,6 +54,12 @@ int qda_create_cb_device(struct qda_dev *qdev, struct device_node *cb_node)
>  		return PTR_ERR(cb_dev);
>  	}
>  
> +	ret = qda_cb_setup_device(qdev, cb_dev, sid);
> +	if (ret) {
> +		device_unregister(cb_dev);
> +		return ret;
> +	}
> +
>  	entry = kzalloc_obj(*entry);
>  	if (!entry) {
>  		qda_destroy_cb_device(cb_dev);
> @@ -74,8 +107,14 @@ int qda_cb_populate(struct qda_dev *qdev, struct device_node *parent_node)
>  
>  void qda_destroy_cb_device(struct device *cb_dev)
>  {
> +	struct qda_iommu_device *iommu_dev;
>  	struct iommu_group *group;
>  
> +	iommu_dev = dev_get_drvdata(cb_dev);
> +	if (iommu_dev && iommu_dev->qdev->iommu_mgr)

Can either of them be NULL?

> +		qda_memory_manager_unregister_device(iommu_dev->qdev->iommu_mgr,
> +						     iommu_dev);
> +
>  	group = iommu_group_get(cb_dev);
>  	if (group) {
>  		iommu_group_remove_device(cb_dev);
> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c
> index 09b6d5ac390b..fdc909facd95 100644
> --- a/drivers/accel/qda/qda_drv.c
> +++ b/drivers/accel/qda/qda_drv.c
> @@ -56,6 +56,32 @@ struct qda_dev *qda_alloc_device(struct device *dev)
>  	return qdev;
>  }
>  
> +void qda_deinit_device(struct qda_dev *qdev)
> +{
> +	if (qdev->iommu_mgr) {

Why?

> +		qda_memory_manager_exit(qdev->iommu_mgr);
> +		kfree(qdev->iommu_mgr);
> +		qdev->iommu_mgr = NULL;

What for? If it's a teardown path, qdev will be destroyed soon.

> +	}
> +}
> +
> +int qda_init_device(struct qda_dev *qdev, int num_cbs)
> +{
> +	int ret;
> +
> +	qdev->iommu_mgr = kzalloc_obj(*qdev->iommu_mgr);

devm_kzalloc_obj()?

> +	if (!qdev->iommu_mgr)
> +		return -ENOMEM;
> +	ret = qda_memory_manager_init(qdev->iommu_mgr, num_cbs);
> +	if (ret) {
> +		drm_err(&qdev->drm_dev, "Failed to initialize memory manager: %d\n", ret);
> +		kfree(qdev->iommu_mgr);
> +		qdev->iommu_mgr = NULL;

And what is going to read this NULL?

> +	}
> +
> +	return ret;
> +}
> +
>  int qda_register_device(struct qda_dev *qdev)
>  {
>  	int ret;
> diff --git a/drivers/accel/qda/qda_drv.h b/drivers/accel/qda/qda_drv.h
> index 3bd41b9593fa..690a833d732b 100644
> --- a/drivers/accel/qda/qda_drv.h
> +++ b/drivers/accel/qda/qda_drv.h
> @@ -13,6 +13,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
> +#include "qda_memory_manager.h"
>  
>  /* Driver identification */
>  #define QDA_DRIVER_NAME "qda"
> @@ -40,6 +41,8 @@ struct qda_dev {
>  	struct device *dev;
>  	/** @cb_devs: Compute context-bank (CB) child devices */
>  	struct list_head cb_devs;
> +	/** @iommu_mgr: IOMMU/memory manager instance */
> +	struct qda_memory_manager *iommu_mgr;
>  	/** @dsp_name: Name of the DSP domain (e.g. "cdsp", "adsp") */
>  	const char *dsp_name;
>  };
> @@ -59,6 +62,8 @@ static inline struct qda_dev *qda_dev_from_drm(struct drm_device *dev)
>  struct qda_dev *qda_alloc_device(struct device *dev);
>  
>  /* Core device lifecycle */
> +int qda_init_device(struct qda_dev *qdev, int num_cbs);
> +void qda_deinit_device(struct qda_dev *qdev);
>  int qda_register_device(struct qda_dev *qdev);
>  
>  #endif /* __QDA_DRV_H__ */
> diff --git a/drivers/accel/qda/qda_memory_manager.c b/drivers/accel/qda/qda_memory_manager.c
> new file mode 100644
> index 000000000000..b1a80ee77c35
> --- /dev/null
> +++ b/drivers/accel/qda/qda_memory_manager.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> +
> +#include <linux/slab.h>
> +#include "qda_drv.h"
> +#include "qda_memory_manager.h"
> +
> +static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr)
> +{
> +	int i;
> +
> +	for (i = 0; i < mem_mgr->num_devices; i++) {
> +		struct qda_iommu_device *iommu_dev = mem_mgr->devices[i];
> +
> +		if (!iommu_dev)
> +			continue;
> +
> +		mem_mgr->devices[i] = NULL;
> +		kfree(iommu_dev);
> +	}
> +	mem_mgr->num_devices = 0;

What for?

> +}
> +
> +/**
> + * qda_memory_manager_register_device() - Register an IOMMU device
> + * @mem_mgr: Pointer to memory manager
> + * @iommu_dev: Pointer to IOMMU device to register
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr,
> +				       struct qda_iommu_device *iommu_dev)
> +{
> +	if (mem_mgr->num_devices >= mem_mgr->max_devices)
> +		return -ENOSPC;
> +
> +	iommu_dev->id = mem_mgr->num_devices;
> +	mem_mgr->devices[mem_mgr->num_devices++] = iommu_dev;
> +
> +	return 0;
> +}
> +
> +/**
> + * qda_memory_manager_unregister_device() - Unregister an IOMMU device
> + * @mem_mgr: Pointer to memory manager
> + * @iommu_dev: Pointer to IOMMU device to unregister
> + *
> + * Removes the device from the registry and frees it.
> + */
> +void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr,
> +					  struct qda_iommu_device *iommu_dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < mem_mgr->num_devices; i++) {
> +		if (mem_mgr->devices[i] == iommu_dev) {
> +			mem_mgr->devices[i] = NULL;
> +			break;
> +		}
> +	}
> +
> +	kfree(iommu_dev);
> +}
> +
> +/**
> + * qda_memory_manager_init() - Initialize the memory manager
> + * @mem_mgr: Pointer to memory manager structure to initialize
> + * @max_devices: Number of IOMMU context bank devices to make room for
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices)
> +{
> +	if (max_devices <= 0)
> +		return -EINVAL;
> +
> +	mem_mgr->devices = kcalloc(max_devices, sizeof(*mem_mgr->devices), GFP_KERNEL);
> +	if (!mem_mgr->devices)
> +		return -ENOMEM;
> +
> +	mem_mgr->num_devices = 0;
> +	mem_mgr->max_devices = max_devices;
> +
> +	return 0;
> +}
> +
> +/**
> + * qda_memory_manager_exit() - Clean up the memory manager
> + * @mem_mgr: Pointer to memory manager structure to clean up
> + */
> +void qda_memory_manager_exit(struct qda_memory_manager *mem_mgr)
> +{
> +	cleanup_all_memory_devices(mem_mgr);
> +
> +	kfree(mem_mgr->devices);
> +	mem_mgr->devices = NULL;
> +	mem_mgr->max_devices = 0;
> +}
> diff --git a/drivers/accel/qda/qda_memory_manager.h b/drivers/accel/qda/qda_memory_manager.h
> new file mode 100644
> index 000000000000..7e38c8a18284
> --- /dev/null
> +++ b/drivers/accel/qda/qda_memory_manager.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#ifndef __QDA_MEMORY_MANAGER_H__
> +#define __QDA_MEMORY_MANAGER_H__
> +
> +#include <linux/device.h>
> +
> +/* Forward declarations */
> +struct qda_dev;
> +
> +/**
> + * struct qda_iommu_device - IOMMU device instance for memory management
> + *
> + * Represents a single IOMMU-enabled device managed by the memory manager.
> + * Each device can be assigned to a specific process session.
> + */
> +struct qda_iommu_device {
> +	/** @dev: Pointer to the underlying device */
> +	struct device *dev;
> +	/** @qdev: Back-pointer to the parent QDA device */
> +	struct qda_dev *qdev;
> +	/** @id: Unique identifier assigned by the memory manager */
> +	u32 id;

What kind of identifier? What is going to use it? I don't see any
readers in this patch, please don't add useless data.

> +	/** @sid: Stream ID for IOMMU transactions */
> +	u32 sid;

Do you need to store it?

> +};
> +
> +/**
> + * struct qda_memory_manager - Central memory management coordinator
> + *
> + * Coordinates memory management across multiple IOMMU devices. Maintains
> + * a registry of devices in an array sized to the number of context banks
> + * described in the device tree.
> + */
> +struct qda_memory_manager {
> +	/** @devices: Array storing registered IOMMU devices */
> +	struct qda_iommu_device **devices;
> +	/** @num_devices: Number of registered IOMMU devices */
> +	int num_devices;

What for? Is devices array to be looped up to num_devices or
max_devices?

> +	/** @max_devices: Capacity of the @devices array */
> +	int max_devices;
> +};
> +
> +int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices);
> +void qda_memory_manager_exit(struct qda_memory_manager *mem_mgr);
> +
> +int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr,
> +				       struct qda_iommu_device *iommu_dev);
> +void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr,
> +					  struct qda_iommu_device *iommu_dev);
> +
> +#endif /* __QDA_MEMORY_MANAGER_H__ */
> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c
> index 723b3e61bf0a..64bf503106d9 100644
> --- a/drivers/accel/qda/qda_rpmsg.c
> +++ b/drivers/accel/qda/qda_rpmsg.c
> @@ -42,11 +42,25 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev)
>  	drm_dev_unplug(&qdev->drm_dev);
>  	qdev->rpdev = NULL;
>  	qda_cb_unpopulate(qdev);
> +	qda_deinit_device(qdev);
> +}
> +
> +static int qda_count_cb_nodes(struct device_node *parent_node)
> +{
> +	struct device_node *child;
> +	int count = 0;
> +
> +	for_each_child_of_node(parent_node, child)
> +		if (of_device_is_compatible(child, "qcom,fastrpc-compute-cb"))
> +			count++;
> +
> +	return count;
>  }
>  
>  static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>  {
>  	struct qda_dev *qdev;
> +	int num_cbs;
>  	int ret;
>  
>  	qdev = qda_rpmsg_alloc_and_init_qdev(rpdev);
> @@ -59,10 +73,15 @@ static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>  		return ret;
>  	}
>  
> -	ret = qda_cb_populate(qdev, rpdev->dev.of_node);
> +	num_cbs = qda_count_cb_nodes(rpdev->dev.of_node);
> +	ret = qda_init_device(qdev, num_cbs);
>  	if (ret)
>  		return ret;
>  
> +	ret = qda_cb_populate(qdev, rpdev->dev.of_node);
> +	if (ret)
> +		goto err_deinit;
> +
>  	ret = qda_register_device(qdev);
>  	if (ret)
>  		goto err_unpopulate;
> @@ -71,6 +90,8 @@ static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>  
>  err_unpopulate:
>  	qda_cb_unpopulate(qdev);
> +err_deinit:
> +	qda_deinit_device(qdev);
>  	return ret;
>  }
>  
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry