| 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: <i6uwqhso4h4vih6fnvnb72byvvmftd4bca3llahlxhnvafnjp2@kxktdrna6qmc>
Date: Thu, 19 Jun 2025 00:08:43 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Ling Xu <quic_lxu5@...cinc.com>
Cc: 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, 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
Subject: Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc
On Wed, Jun 18, 2025 at 01:15:16PM +0800, Ling Xu wrote:
> 在 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",
You also need to modify fastrpc_ioctl_capability definition to drop the
domain field and also modify fastrpc_get_info_from_kernel().
--
With best wishes
Dmitry
Powered by blists - more mailing lists