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] [day] [month] [year] [list]

Message-ID: <jeixk4dmk53ubnujop3sp2lc6jffugjmzndmmqmyuft7uquibt@s2h2dgj7yj7h>
Date: Wed, 29 Jan 2025 13:23:57 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>
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 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.

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/


> 
> --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
> >>
> 

-- 
With best wishes
Dmitry