On 05/07/2025, Luca Ceresoli wrote:

Hello Liu,

On Wed, 7 May 2025 10:10:53 +0800
Liu Ying [off-list ref] wrote:

quoted

On 05/07/2025, Luca Ceresoli wrote:

quoted

Hello Liu,  

Hi Luca,

quoted

thanks for your further feedback.

On Tue, 6 May 2025 10:24:18 +0800
Liu Ying [off-list ref] wrote:
  

quoted

On 04/30/2025, Luca Ceresoli wrote:  

quoted

Hello Liu,    

Hi Luca,
 

quoted

On Tue, 29 Apr 2025 10:10:55 +0800
Liu Ying [off-list ref] wrote:
    

quoted

Hi,

On 04/25/2025, Luca Ceresoli wrote:    

quoted

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>    

[...]
    

quoted

quoted

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

quoted

quoted

+		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. :)

[...]
    

quoted

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

[...]
    

quoted

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

The pixel combiner DT node would be added in imx8-ss-dc{0,1}.dtsi, please
see the case for imx8-ss-dc0.dtsi introduced by an in-flight patch[1].  As
channel@{0,1} child nodes always exist(DT overlay cannot effectively delete
any of them) and channel@0 always comes first, there is no problematic case.  

I'm not questioning what existing and future dts files (will) contain,
and surely I don't see a good reason someone would write channel@1
before channel@0.

My point is:

 - the bindings _allow_ channel1 before channel@0
 - the error management code after the free_child label won't work
   correctly if channel1 is before channel@0 in the device tree

IOW the driver is not robust against all legal device tree descriptions,
and it could be easily made robust using the example code in my
previous e-mail (quoted a few lines above).

If you agree about this I'll be happy to send a patch doing that change.
If you think I'm wrong, I won't fight a battle. This topic is
orthogonal to the change I'm introducing in this patch, and I can
continue the conversion independently from this discussion.  

I don't think it is necessary to do that change for now.  When someone
really comes across this issue, we may make the error management code
robust.

quoted

quoted

quoted

Thanks for taking the time to dig into this!    

After looking into this patch and patch 31(though I've already provided my A-b)
more closely, I think the imx8qxp_pc and imx8{qm,qxp}_ldb main structures
should have the same life time with the embedded DRM bridges, because for
example the clk_apb clock in struct imx8qxp_pc would be accessed by the
imx8qxp_pc_bridge_mode_set DRM bridge callback.  But, IIUC, your patches extend
the life time for the embedded channel/bridge structures only, but not for the
main structures.  What do you think ?  

I see you concern, but I'm sure the change I'm introducing is not
creating the problem you are concerned about.

The key aspect is that my patch is merely changing the lifetime of the
_allocation_ of the drm_bridge, not its usage. On drm_bridge_remove()
the bridge is removed from its encoder chain and it is completely not
reachable, both before and after my patch. With my patch it is not
freed immediately, but it's just a piece of "wasted" memory that is
still allocated until elsewhere in the kernel there are pointers to it,
to avoid use-after-free.

With this explanation, do you think my patch is correct (after fixing
the bug we already discussed of course)?  

I tend to say your patch is not correct because we'll eventually make sure
that removing a bridge module is safe when doing atomic commit,

I think your sentence can be rephrased as "your patch is correct with
the current code base where bridges are not (yet) removable, but there
will be a problem when they start to actually be removable".

Is my understanding correct? If it is, I agree on that sentence.

Nope, I meant your patch should align the life times of the main structures
and the DRM bridges, for the sake of the kinda long term goal - remove bridge
driver module safely when doing atomic commit.

The work to have removable bridges is massive and non-trivial, so it
will need to be tackled in steps. The grand plan [0] is:

 1. add refcounting to DRM bridges (struct drm_bridge)
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices 
 4. finish the hotplug bridge work, removing the "always-disconnected"
    connector, moving code to the core and potentially removing the
    hotplug-bridge itself (this needs to be clarified as points 1-3 are
    developed)

I'm busy with internal things these days and cannot look into the grand
plan and steps closely, sorry about that.

I am at step 1 right now. Removal during atomic updates is step 2,
ideas about how to implement that are already being discussed [1],
there's a practical plan proposed by Maxime with the goal of reaching
removable bridges without breaking things along the path.

[0] https://lore.kernel.org/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/ (local)
[1] https://lore.kernel.org/all/20250106-vigorous-talented-viper-fa49d9@houat/ (local)

quoted

which means
the main structures should have the same life time with the DRM bridges.

The word "lifetime" mean two things for bridges:

 * the time span during which memory is allocated for a struct
   drm_bridge (along with the embedding struct)

Note that with your patch set the imx8*-ldb drivers and this bridge driver
won't allocate the DRM bridge along with the embedding struct. This makes
me worry, because maybe these drivers are the only "special" ones in this
patch set and I don't want them to be "special" after your patch set is
applied.

 * the time span during which a DRM bridge is active/used/usable as
   part of a card
   - i.e. when it is part of an encoder chain
   - i.e. when drm_bridge_funcs callbacks can be called
   - i.e. from drm_bridge_add() to drm_bridge_remove()

These two lifetimes used to be nearly the same. Now the "memory
allocation lifetime" is extended, but the "bridge existence" is
unchanged: drm_bridge_add() to drm_bridge_remove() are called in the
same place and do the same things, so the bridge will stop being in any
encoder chain at the exact same time. now we are just keeping a piece of
memory allocated for a longer time.

Seen in another way, the events used to be:

 * probe:
   - allocate bridge
   - drm_bridge_add()

 * remove
   - drm_bridge_remove()
   - now the bridge is not used, it's just some dead memory [*]
   - kfree bridge (either in .remove() or just after by devm)

Now it becomes:

 * probe:
   - allocate bridge
   - drm_bridge_add()

 * remove
   - drm_bridge_remove()
   - now the bridge is not used, it's just some dead memory [*]
   - maybe some more time, possibly long, until the last put [*]
   - kfree bridge (by devm)

The duration of the [*] steps changes, but it's harmless because the
bridge is not used at all. No change except for memory allocation.

Luca

-- 
Regards,
Liu Ying