Re: [PATCH] drm/panthor: snapshot firmware interface counts before loops

[Osama Abdelkader]

On Tue, Jul 28, 2026 at 04:17:14PM +0100, Liviu Dudau wrote:
> On Mon, Jul 20, 2026 at 03:55:18PM +0200, Osama Abdelkader wrote:
> > The firmware exposes the global group count and per-group stream count in
> > the shared control interface. These values are validated before being used
> > as loop bounds, but the memory is shared with the MCU firmware and can be
> > changed after validation.
> 
> Again, you're making statements that need to be proven here. How is MCU
> firmware going to change the content of the memory shared with the driver?
> 

Thanks for the review. yes, this was flagged by Sashiko while reviewing the 
previous patch, but I agree the commit message shouldn't present it as a proven
firmware mutation bug.

The useful part here is just making the validated count the same value used
as the loop bound. since It's an optimisation, should I send v2 without Fixes
and Cc stable tags?

> > 
> > Read each count once with READ_ONCE() and use the validated snapshot as the
> > loop bound. This keeps the loop bounds consistent with the validation and
> > prevents the compiler from reloading a firmware-controlled count during
> > iteration.
> 
> As an optimisation I'm fine with it, so:
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> 
> Best regards,
> Liviu
> 

Best regards,
Osama
> > 
> > Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/panthor/panthor_fw.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index e2fcbd639c3c..6e6da98d795e 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -959,6 +959,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> >  	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
> >  			   ((u64)csg_idx * glb_iface->control->group_stride);
> > +	u32 stream_num;
> >  	unsigned int i;
> >  
> >  	if (iface_offset > shared_section_sz ||
> > @@ -972,8 +973,8 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va,
> >  						 sizeof(*csg_iface->output));
> >  
> > -	if (csg_iface->control->stream_num < MIN_CS_PER_CSG ||
> > -	    csg_iface->control->stream_num > MAX_CS_PER_CSG)
> > +	stream_num = READ_ONCE(csg_iface->control->stream_num);
> > +	if (stream_num < MIN_CS_PER_CSG || stream_num > MAX_CS_PER_CSG)
> >  		return -EINVAL;
> >  
> >  	if (!csg_iface->input || !csg_iface->output) {
> > @@ -990,7 +991,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  		}
> >  	}
> >  
> > -	for (i = 0; i < csg_iface->control->stream_num; i++) {
> > +	for (i = 0; i < stream_num; i++) {
> >  		int ret = panthor_init_cs_iface(ptdev, csg_idx, i);
> >  
> >  		if (ret)
> > @@ -1015,6 +1016,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
> >  {
> >  	struct panthor_fw_global_iface *glb_iface = &ptdev->fw->iface.global;
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> > +	u32 group_num;
> >  	unsigned int i;
> >  
> >  	if (!ptdev->fw->shared_section->mem->kmap)
> > @@ -1034,17 +1036,17 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (glb_iface->control->group_num > MAX_CSGS ||
> > -	    glb_iface->control->group_num < MIN_CSGS) {
> > +	group_num = READ_ONCE(glb_iface->control->group_num);
> > +	if (group_num > MAX_CSGS || group_num < MIN_CSGS) {
> >  		drm_err(&ptdev->base, "Invalid number of control groups");
> >  		return -EINVAL;
> >  	}
> >  
> > -	for (i = 0; i < glb_iface->control->group_num; i++) {
> > +	for (i = 0; i < group_num; i++) {
> >  		int ret = panthor_init_csg_iface(ptdev, i);
> >  
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> >  	drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x",
> > -- 
> > 2.43.0
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯