| 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: <5f70a482-6e61-4817-afdb-d5db4747897a@quicinc.com>
Date: Mon, 16 Jun 2025 19:28:03 +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
在 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?
>
> --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