linux-kernel - Re: [PATCH v2 5/5] misc: fastrpc: Modify context id mask to support polling mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <053dd6d6-9ca2-4723-8c21-da4093f00e03@quicinc.com>
Date: Mon, 10 Feb 2025 14:35:47 +0530
From: Ekansh Gupta <quic_ekangupt@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: <srinivas.kandagatla@...aro.org>, <linux-arm-msm@...r.kernel.org>,
        <gregkh@...uxfoundation.org>, <quic_bkumar@...cinc.com>,
        <linux-kernel@...r.kernel.org>, <quic_chennak@...cinc.com>,
        <dri-devel@...ts.freedesktop.org>, <arnd@...db.de>
Subject: Re: [PATCH v2 5/5] misc: fastrpc: Modify context id mask to support
 polling mode




On 1/29/2025 4:53 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 29, 2025 at 11:13:05AM +0530, Ekansh Gupta wrote:
>>
>>
>> On 1/29/2025 5:00 AM, Dmitry Baryshkov wrote:
>>> On Mon, Jan 27, 2025 at 10:12:39AM +0530, Ekansh Gupta wrote:
>>>> DSP needs last 4 bits of context id to be 0 for polling mode to be
>>>> supported as setting of last 8 is intended for async mode(not yet
>>>> supported on upstream driver) and setting these bits restrics
>>>> writing to poll memory from DSP. Modify context id mask to ensure
>>>> polling mode is supported.
>>> Shouldn't this commit come before the previous one?
>> Yes, I'll change the order in next series.
>>
>> Thanks for reviewing the changes.
> Please consider asking somebody for the internal review before sending
> patches. This should help you to catch such mistakes.
>
> Next, I keep on asking for a sensible userspace and testing suite. No,
> existing fastrpc doesn't pass that criteria. We have discussed that, but
> I see no changes in the development. The PR that you've linked in the
> cover letter contains a single commit, covering documentation, new
> IOCTL, CRC support, poll mode support, etc. What if the discussion ends
> up accepting the CRC support but declining the polling mode? Or vice
> versa, accepting poll mode but declining the CRC support? There is no
> easy way for us to review userspace impact, corresponding changes, etc.

We are working with our Legal team to push HexagonSDK publicly , that  will
have sample apps for all features supported by upstream driver and can be used
for testing.

I'll break down the PR to multiple meaningful commits based on the features
that are getting added.

>
> Last, but not least: I want to bring up Sima's response in one of the
> earlier discussions ([1]): "Yeah, if fastrpc just keeps growing the
> story will completely different."
>
> You are adding new IOCTL and new ivocation API. That totally sounds
> like "keeps growing", which returns us back to the uAPI question,
> drm_accel.h and the rest of the questions on the userspace, compiler,
> etc.
>
> [1] https://lore.kernel.org/dri-devel/Znk87-xCx8f3fIUL@phenom.ffwll.local/

Currently, we are upstreaming the features supported on DSP for publicly
available platforms. No features for future platforms are planned for FastRPC
driver.

We are also looking in having the driver under drivers/accel for any new features
that are planned in future platforms.

--ekansh

>
>
>> --ekansh
>>
>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
>>>> ---
>>>>  drivers/misc/fastrpc.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 257a741af115..ef56c793c564 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -40,7 +40,7 @@
>>>>  #define FASTRPC_INIT_HANDLE	1
>>>>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
>>>>  #define FASTRPC_MAX_STATIC_HANDLE (20)
>>>> -#define FASTRPC_CTXID_MASK (0xFF0)
>>>> +#define FASTRPC_CTXID_MASK (0xFF0000)
>>>>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>>>>  #define INIT_FILE_NAMELEN_MAX (128)
>>>>  #define FASTRPC_DEVICE_NAME	"fastrpc"
>>>> @@ -524,7 +524,7 @@ static void fastrpc_context_free(struct kref *ref)
>>>>  		fastrpc_buf_free(ctx->buf);
>>>>  
>>>>  	spin_lock_irqsave(&cctx->lock, flags);
>>>> -	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
>>>> +	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 16);
>>>>  	spin_unlock_irqrestore(&cctx->lock, flags);
>>>>  
>>>>  	kfree(ctx->maps);
>>>> @@ -664,7 +664,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>>>>  		spin_unlock_irqrestore(&cctx->lock, flags);
>>>>  		goto err_idr;
>>>>  	}
>>>> -	ctx->ctxid = ret << 4;
>>>> +	ctx->ctxid = ret << 16;
>>>>  	spin_unlock_irqrestore(&cctx->lock, flags);
>>>>  
>>>>  	kref_init(&ctx->refcount);
>>>> @@ -2675,7 +2675,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>>>>  	if (len < sizeof(*rsp))
>>>>  		return -EINVAL;
>>>>  
>>>> -	ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4);
>>>> +	ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 16);
>>>>  
>>>>  	spin_lock_irqsave(&cctx->lock, flags);
>>>>  	ctx = idr_find(&cctx->ctx_idr, ctxid);
>>>> -- 
>>>> 2.34.1
>>>>