| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36464d9c-9bbb-488e-85c0-7e5318a16bf8@quicinc.com>
Date: Wed, 18 Jun 2025 13:15:16 +0800
From: Ling Xu <quic_lxu5@...cinc.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
<andersson@...nel.org>, <konradybcio@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<amahesh@....qualcomm.com>, <arnd@...db.de>,
<gregkh@...uxfoundation.org>
CC: <quic_kuiw@...cinc.com>, <quic_ekangupt@...cinc.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
"Dmitry
Baryshkov" <dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc
在 6/16/2025 7:28 PM, Ling Xu 写道:
> 在 4/8/2025 4:14 PM, Srinivas Kandagatla 写道:
>>
>>
>> On 07/04/2025 10:13, Ling Xu wrote:
>>> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
>>>>
>>>>
>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>> GPDSP remoteprocs.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
>>>>> Signed-off-by: Ling Xu <quic_lxu5@...cinc.com>
>>>>> ---
>>>>> drivers/misc/fastrpc.c | 10 ++++++++--
>>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>> --- a/drivers/misc/fastrpc.c
>>>>> +++ b/drivers/misc/fastrpc.c
>>>>> @@ -28,7 +28,9 @@
>>>>> #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 GDSP0_DOMAIN_ID (5)
>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>
>>>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
>>>>
>>>> Domain ids are strictly for a domain not each instance.
>>>>
>>>>
>>>>> +#define FASTRPC_DEV_MAX 7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>>> #define FASTRPC_MAX_SESSIONS 14
>>>>> #define FASTRPC_MAX_VMIDS 16
>>>>> #define FASTRPC_ALIGN 128
>>>>> @@ -107,7 +109,9 @@
>>>>> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>>> - "sdsp", "cdsp", "cdsp1" };
>>>>> + "sdsp", "cdsp",
>>>>> + "cdsp1", "gdsp0",
>>>>> + "gdsp1" };
>>>>> struct fastrpc_phy_page {
>>>>> u64 addr; /* physical address */
>>>>> u64 size; /* size of contiguous region */
>>>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>> break;
>>>>> case CDSP_DOMAIN_ID:
>>>>> case CDSP1_DOMAIN_ID:
>>>>> + case GDSP0_DOMAIN_ID:
>>>>> + case GDSP1_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]);
>>>>
>>>>
>>>> Can you try this patch: only compile tested.
>>>>
>>>> ---------------------------------->cut<---------------------------------------
>>>> From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>>>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>>>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>>>
>>>> Currently the domain ids are added for each instance of domain, this is
>>>> totally not scalable approch.
>>>>
>>>> Clean this mess and create domain ids for only domains not its
>>>> instances.
>>>> This patch also moves the domain ids to uapi header as this is required
>>>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>>>
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>>>> ---
>>>> drivers/misc/fastrpc.c | 45 ++++++++++++++++++++-----------------
>>>> include/uapi/misc/fastrpc.h | 7 ++++++
>>>> 2 files changed, 32 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..b3932897a437 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -23,12 +23,6 @@
>>>> #include <uapi/misc/fastrpc.h>
>>>> #include <linux/of_reserved_mem.h>
>>>>
>>>> -#define ADSP_DOMAIN_ID (0)
>>>> -#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 +100,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 */
>>>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>>>> return -EFAULT;
>>>>
>>>> cap.capability = 0;
>>>> - if (cap.domain >= FASTRPC_DEV_MAX) {
>>>> + if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>>>> dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>>>> cap.domain, err);
>>>> return -ECHRNG;
>>>
>>> I tested this patch and saw one issue.
>>> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
>>
>>
>> Why is the userspace using something that is not uAPI?
>>
>> Why does it matter if its gdsp0 or gdsp1 for the userspace?
>> It should only matter if its gdsp domain or not.
>>
>
> Give an example here:
> In test example, user can use below API to query the notification capability of the specific domain_id,
> (actually this will not have any functional issue, but just return an error and lead wrong message):
> request_status_notifications_enable(domain_id, (void*)STATUS_CONTEXT, pd_status_notifier_callback)
>
> this will call ioctl_getdspinfo in fastrpc_ioctl.c:
> https://github.com/quic-lxu5/fastrpc/blob/8feccfd2eb46272ad1fabed195bfddb7fd680cbd/src/fastrpc_ioctl.c#L201
>
> code snip:
> FARF(ALWAYS, "ioctl_getdspinfo in ioctl.c domain:%d", domain);
> ioErr = ioctl(dev, FASTRPC_IOCTL_GET_DSP_INFO, &cap);
> FARF(ALWAYS, "done ioctl_getdspinfo in ioctl.c ioErr:%x", ioErr);
>
> and finally call fastrpc_get_dsp_info in fastrpc.c.
>
> if I use the patch you shared, it will report below error:
>
> UMD log:
> 2025-01-08T18:45:03.168718+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:201: ioctl_getdspinfo in ioctl.c domain:5
> 2025-01-08T18:45:03.169307+00:00 qcs9100-ride-sx calculator: log_config.c:396: file_watcher_thread starting for domain 5
> 2025-01-08T18:45:03.180355+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:203: done ioctl_getdspinfo in ioctl.c ioErr:ffffffff
>
> putty log:
> [ 1332.308444] qcom,fastrpc 20c00000.remoteproc:glink-edge.fastrpcglink-apps-dsp.-1.-1: Error: Invalid domain id:5, err:0
>
> Because on the user side, gdsp0 and gdsp1 will be distinguished to 5 and 6.
> so do you mean you want me to modify UMD code to transfer both gdsp0 and gdsp1 to gdsp just in ioctl_getdspinfo?
>>
There is no error message after removing these lines based on srini's patch, is this modification appropriate? If so, I will submit a new patch.
@@ -1771,17 +1763,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",
>> --srini
>>
>>
>>> For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.
>>>
>>> --Ling Xu
>>>
>>>> @@ -2255,6 +2247,24 @@ 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) == 0) {
>>>> + return ADSP_DOMAIN_ID;
>>>> + } else if (strncmp(domain, "cdsp", 4) == 0) {
>>>> + return CDSP_DOMAIN_ID;
>>>> + } else if (strncmp(domain, "mdsp", 4) ==0) {
>>>> + return MDSP_DOMAIN_ID;
>>>> + } else if (strncmp(domain, "sdsp", 4) ==0) {
>>>> + return SDSP_DOMAIN_ID;
>>>> + } else if (strncmp(domain, "gdsp", 4) ==0) {
>>>> + return GDSP_DOMAIN_ID;
>>>> + }
>>>> +
>>>> + return -EINVAL;
>>>> +
>>>> +}
>>>> +
>>>> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>> {
>>>> struct device *rdev = &rpdev->dev;
>>>> @@ -2272,15 +2282,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;
>>>> }
>>>>
>>>> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>> case SDSP_DOMAIN_ID:
>>>> /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>>>> 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 fdev_error;
>>>> break;
>>>> case CDSP_DOMAIN_ID:
>>>> - case CDSP1_DOMAIN_ID:
>>>> + case GDSP_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 fdev_error;
>>>>
>>>> - err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>>>> + err = fastrpc_device_register(rdev, data, false, domain);
>>>> if (err)
>>>> goto populate_error;
>>>> break;
>>>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>>>> index f33d914d8f46..89516abd258f 100644
>>>> --- a/include/uapi/misc/fastrpc.h
>>>> +++ b/include/uapi/misc/fastrpc.h
>>>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>>> __s32 reserved[5];
>>>> };
>>>>
>>>> +#define ADSP_DOMAIN_ID (0)
>>>> +#define MDSP_DOMAIN_ID (1)
>>>> +#define SDSP_DOMAIN_ID (2)
>>>> +#define CDSP_DOMAIN_ID (3)
>>>> +#define GDSP_DOMAIN_ID (4)
>>>> +
>>>> +#define FASTRPC_DOMAIN_MAX 4
>>>> struct fastrpc_ioctl_capability {
>>>> __u32 domain;
>>>> __u32 attribute_id;
>>>
>
--
Thx and BRs,
Ling Xu
Powered by blists - more mailing lists