Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to devm_drm_bridge_alloc() API

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Liu,

On Tue, 29 Apr 2025 10:10:55 +0800
Liu Ying <victor.liu@nxp.com> wrote:

> Hi,
> 
> On 04/25/2025, Luca Ceresoli wrote:
> > This is the new API for allocating DRM bridges.
> > 
> > This driver embeds an array of channels in the main struct, and each
> > channel embeds a drm_bridge. This prevents dynamic, refcount-based
> > deallocation of the bridges.
> > 
> > To make the new, dynamic bridge allocation possible:
> > 
> >  * change the array of channels into an array of channel pointers
> >  * allocate each channel using devm_drm_bridge_alloc()
> >  * adapt the code wherever using the channels
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

[...]

> > @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
> >  free_child:
> >  	of_node_put(child);
> >  
> > -	if (i == 1 && pc->ch[0].next_bridge)
> > -		drm_bridge_remove(&pc->ch[0].bridge);
> > +	if (i == 1 && pc->ch[0]->next_bridge)  
> 
> Since this patch makes pc->ch[0] and pc->ch[1] be allocated separately,
> pc->ch[0] could be NULL if channel0 is not available, hence a NULL pointer
> dereference here...

See below for this.

> > +		drm_bridge_remove(&pc->ch[0]->bridge);
> >  
> >  	pm_runtime_disable(dev);
> >  	return ret;
> > @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev)
> >  	int i;
> >  
> >  	for (i = 0; i < 2; i++) {
> > -		ch = &pc->ch[i];
> > +		ch = pc->ch[i];
> >  
> >  		if (!ch->is_available)  
> 
> ...and here too.

This is indeed a bug, I should have checked the pointer for being
non-NULL.

Looking at that more closely, I think the is_available flag can be
entirely removed now. The allocation itself (ch != NULL) now is
equivalent. Do you think my reasoning is correct?

Ouch! After writing the previous paragraph I realized you proposed this
a few lines below! OK, removing is_available. :)

[...]

> On top of this patch series, this issue doesn't happen if I apply the below
> change:

[...]

> @@ -351,7 +349,7 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>  free_child:
>         of_node_put(child);
>  
> -       if (i == 1 && pc->ch[0]->next_bridge)
> +       if (i == 1 && pc->ch[0])
>                 drm_bridge_remove(&pc->ch[0]->bridge);

Unrelated to this patch, but as I looked at it more in depth now, I'm
not sure this whole logic is robust, even in the original code.

The 'i == 1' check here seems to mean "if some error happened when
handling channel@1, that means channel@0 was successfully initialized,
so let's clean up channel 0".

However my understanding of the bindings is that device tree is allowed
to have the channel@1 node before the channel@0 node (or even channel@1
without channel@0, but that's less problematic here).

In such case (channel@1 before channel@0), this would happen:

 1. alloc and init ch[1], all OK
 2. alloc and init ch[0], an error happens
    (e.g. of_graph_get_remote_node() fails)

So we'd reach the free_child: label, and we should call
drm_bridge_remove() for ch[1]->bridge, but there's no code to do that.

To be robust in such a case, I think both channels need to be checked
independently, as the status of one does not imply the status of the
other. E.g.:

  for (i = 0; i < 2; i++)
      if (pc->ch[i] && pc->ch[i]->next_bridge)
          drm_bridge_remove(&pc->ch[i]->bridge);

(which is similar to what .remove() does after the changes discussed in
this thread, and which I have queued for v3)

What's your opinion? Do you think I missed anything?

Thanks for taking the time to dig into this!

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com