在 7/17/2025 3:28 AM, Bjorn Andersson 写道: > 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())? I already split this to 2 changes in latest patch, mess means we created domain ids for its instances. > >> Co-developed-by: Srinivas Kandagatla <srinivas.kandaga...@linaro.org> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandaga...@linaro.org> >> Signed-off-by: Ling Xu <quic_l...@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: > cdsp1 use cdsp0 daemon, they are two instances but one domain. In kernel, we just care about the domains. we just give a scalable approach without adding instances for any new dsp every time. > 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 >>
-- Thx and BRs, Ling Xu
