[PATCH v7 3/4] misc: fastrpc: Cleanup the domain names
Bjorn Andersson
andersson at kernel.org
Wed Jul 16 19:28:20 UTC 2025
On Mon, Jul 14, 2025 at 11:11:32AM +0530, Ling Xu wrote:
> Currently the domain ids are added for each instance of domains, this is
> totally not scalable approach.
This sentence only makes sense for people in your team or participants
of some recent meeting or (private) mail thread of yours. When providing
you problem description [1], do so in a way that it makes sense to
people outside that bubble - and yourself next month.
[1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> Clean this mess and create domain ids for
> only domains not its instances.
>
Is the "mess" that the domain is part of the ioctl, or is the mess that
the names of the domains are defined in an array and you prefer them to
be listed out in code (fastrpc_get_domain_id())?
> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
> Signed-off-by: Ling Xu <quic_lxu5 at quicinc.com>
> ---
> drivers/misc/fastrpc.c | 50 ++++++++++++++++---------------------
> include/uapi/misc/fastrpc.h | 2 +-
> 2 files changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 378923594f02..85b6eb16b616 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -27,8 +27,6 @@
> #define MDSP_DOMAIN_ID (1)
> #define SDSP_DOMAIN_ID (2)
> #define CDSP_DOMAIN_ID (3)
> -#define CDSP1_DOMAIN_ID (4)
> -#define FASTRPC_DEV_MAX 5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> #define FASTRPC_MAX_SESSIONS 14
> #define FASTRPC_MAX_VMIDS 16
> #define FASTRPC_ALIGN 128
> @@ -106,8 +104,6 @@
>
> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>
> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> - "sdsp", "cdsp", "cdsp1" };
> struct fastrpc_phy_page {
> u64 addr; /* physical address */
> u64 size; /* size of contiguous region */
> @@ -1723,7 +1719,6 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> uint32_t attribute_id = cap->attribute_id;
> uint32_t *dsp_attributes;
> unsigned long flags;
> - uint32_t domain = cap->domain;
> int err;
>
> spin_lock_irqsave(&cctx->lock, flags);
> @@ -1741,7 +1736,7 @@ static int fastrpc_get_info_from_kernel(struct fastrpc_ioctl_capability *cap,
> err = fastrpc_get_info_from_dsp(fl, dsp_attributes, FASTRPC_MAX_DSP_ATTRIBUTES);
> if (err == DSP_UNSUPPORTED_API) {
> dev_info(&cctx->rpdev->dev,
> - "Warning: DSP capabilities not supported on domain: %d\n", domain);
> + "Warning: DSP capabilities not supported\n");
> kfree(dsp_attributes);
> return -EOPNOTSUPP;
> } else if (err) {
> @@ -1769,17 +1764,6 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> return -EFAULT;
>
> cap.capability = 0;
> - if (cap.domain >= FASTRPC_DEV_MAX) {
> - dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
> - cap.domain, err);
> - return -ECHRNG;
> - }
> -
> - /* Fastrpc Capablities does not support modem domain */
> - if (cap.domain == MDSP_DOMAIN_ID) {
> - dev_err(&fl->cctx->rpdev->dev, "Error: modem not supported %d\n", err);
> - return -ECHRNG;
> - }
>
> if (cap.attribute_id >= FASTRPC_MAX_DSP_ATTRIBUTES) {
> dev_err(&fl->cctx->rpdev->dev, "Error: invalid attribute: %d, err: %d\n",
> @@ -2255,6 +2239,20 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
> return err;
> }
>
> +static int fastrpc_get_domain_id(const char *domain)
> +{
> + if (!strncmp(domain, "adsp", 4))
> + return ADSP_DOMAIN_ID;
> + else if (!strncmp(domain, "cdsp", 4))
> + return CDSP_DOMAIN_ID;
> + else if (!strncmp(domain, "mdsp", 4))
> + return MDSP_DOMAIN_ID;
> + else if (!strncmp(domain, "sdsp", 4))
> + return SDSP_DOMAIN_ID;
> +
The removed code performs a string compare and you replace this with a
string prefix compare, but there's no motivation given to why this is
done.
I'm also wondering why cdsp1 is now in CDSP_DOMAIN_ID, is that
intentional? Was it wrong before? If so, that change should be done
alone and with a Fixes:
Regards,
Bjorn
> + return -EINVAL;
> +}
> +
> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> {
> struct device *rdev = &rpdev->dev;
> @@ -2272,15 +2270,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> return err;
> }
>
> - for (i = 0; i < FASTRPC_DEV_MAX; i++) {
> - if (!strcmp(domains[i], domain)) {
> - domain_id = i;
> - break;
> - }
> - }
> + domain_id = fastrpc_get_domain_id(domain);
>
> if (domain_id < 0) {
> - dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
> + dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
> return -EINVAL;
> }
>
> @@ -2330,21 +2323,20 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> case ADSP_DOMAIN_ID:
> case MDSP_DOMAIN_ID:
> case SDSP_DOMAIN_ID:
> - /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
> + /* Unsigned PD offloading is only supported on CDSP */
> data->unsigned_support = false;
> - err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
> + err = fastrpc_device_register(rdev, data, secure_dsp, domain);
> if (err)
> goto err_free_data;
> break;
> case CDSP_DOMAIN_ID:
> - case CDSP1_DOMAIN_ID:
> data->unsigned_support = true;
> /* Create both device nodes so that we can allow both Signed and Unsigned PD */
> - err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> + err = fastrpc_device_register(rdev, data, true, domain);
> if (err)
> goto err_free_data;
>
> - err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
> + err = fastrpc_device_register(rdev, data, false, domain);
> if (err)
> goto err_deregister_fdev;
> break;
> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..c6e2925f47e6 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -134,7 +134,7 @@ struct fastrpc_mem_unmap {
> };
>
> struct fastrpc_ioctl_capability {
> - __u32 domain;
> + __u32 unused; /* deprecated, ignored by the kernel */
> __u32 attribute_id;
> __u32 capability; /* dsp capability */
> __u32 reserved[4];
> --
> 2.34.1
>
More information about the dri-devel
mailing list