On Mon, Dec 15, 2025 at 10:41:32AM +0100, Maxime Ripard wrote: > On Thu, Dec 11, 2025 at 06:47:37PM +0100, Luca Ceresoli wrote: > > Hi Maxime, > > > > On Mon Dec 1, 2025 at 5:51 PM CET, Maxime Ripard wrote: > > > On Mon, Nov 24, 2025 at 05:25:39PM +0100, Luca Ceresoli wrote: > > >> Hi Maxime, > > >> > > >> On Mon Nov 24, 2025 at 11:39 AM CET, Maxime Ripard wrote: > > >> > On Wed, Nov 19, 2025 at 02:05:37PM +0100, Luca Ceresoli wrote: > > >> >> Several drivers (about 20) follow the same pattern: > > >> >> > > >> >> 1. get a pointer to a bridge (typically the next bridge in the > > >> >> chain) by > > >> >> calling of_drm_find_bridge() > > >> >> 2. store the returned pointer in the private driver data, keep it > > >> >> until > > >> >> driver .remove > > >> >> 3. dereference the pointer at attach time and possibly at other times > > >> >> > > >> >> of_drm_find_bridge() is now deprecated because it does not increment > > >> >> the > > >> >> refcount and should be replaced with drm_of_find_bridge() + > > >> >> drm_bridge_put(). > > >> >> > > >> >> However some of those drivers have a complex code flow and adding a > > >> >> drm_bridge_put() call in all the appropriate locations is error-prone, > > >> >> leads to ugly and more complex code, and can lead to errors over time > > >> >> with > > >> >> code flow changes. > > >> >> > > >> >> To handle all those drivers in a straightforward way, add a devm > > >> >> variant of > > >> >> drm_of_find_bridge() that adds a devm action to invoke > > >> >> drm_bridge_put() > > >> >> when the said driver is removed. This allows all those drivers to put > > >> >> the > > >> >> reference automatically and safely with a one line change: > > >> >> > > >> >> - priv->next_bridge = of_drm_find_bridge(remote_np); > > >> >> + priv->next_bridge = devm_drm_of_find_bridge(dev, remote_np); > > >> >> > > >> >> Signed-off-by: Luca Ceresoli <[email protected]> > > >> >> > > >> >> --- > > >> >> drivers/gpu/drm/drm_bridge.c | 30 ++++++++++++++++++++++++++++++ > > >> >> include/drm/drm_bridge.h | 5 +++++ > > >> >> 2 files changed, 35 insertions(+) > > >> >> > > >> >> diff --git a/drivers/gpu/drm/drm_bridge.c > > >> >> b/drivers/gpu/drm/drm_bridge.c > > >> >> index 09ad825f9cb8..c7baafbe5695 100644 > > >> >> --- a/drivers/gpu/drm/drm_bridge.c > > >> >> +++ b/drivers/gpu/drm/drm_bridge.c > > >> >> @@ -1446,6 +1446,36 @@ struct drm_bridge *drm_of_find_bridge(struct > > >> >> device_node *np) > > >> >> } > > >> >> EXPORT_SYMBOL(drm_of_find_bridge); > > >> >> > > >> >> +/** > > >> >> + * devm_drm_of_find_bridge - find the bridge corresponding to the > > >> >> device > > >> >> + * node in the global bridge list and add a > > >> >> devm > > >> >> + * action to put it > > >> >> + * > > >> >> + * @dev: device requesting the bridge > > >> >> + * @np: device node > > >> >> + * > > >> >> + * On success the returned bridge refcount is incremented, and a devm > > >> >> + * action is added to call drm_bridge_put() when @dev is removed. So > > >> >> the > > >> >> + * caller does not have to put the returned bridge explicitly. > > >> >> + * > > >> >> + * RETURNS: > > >> >> + * drm_bridge control struct on success, NULL on failure > > >> >> + */ > > >> >> +struct drm_bridge *devm_drm_of_find_bridge(struct device *dev, > > >> >> struct device_node *np) > > >> >> +{ > > >> >> + struct drm_bridge *bridge = drm_of_find_bridge(np); > > >> >> + > > >> >> + if (bridge) { > > >> >> + int err = devm_add_action_or_reset(dev, > > >> >> drm_bridge_put_void, bridge); > > >> >> + > > >> >> + if (err) > > >> >> + return ERR_PTR(err); > > >> >> + } > > >> >> + > > >> >> + return bridge; > > >> >> +} > > >> >> +EXPORT_SYMBOL(devm_drm_of_find_bridge); > > >> > > > >> > That's inherently unsafe though, because even if the bridge is removed > > >> > other parts of DRM might still have a reference to it and could call > > >> > into it. > > >> > > > >> > We'd then have dropped our reference to the next bridge, which could > > >> > have been freed, and it's a use-after-free. > > >> > > >> I think you refer to this scenario: > > >> > > >> 1. pipeline: encoder --> bridge A --> bridge B --> bridge C > > >> 2. encoder takes a reference to bridge B > > >> using devm_drm_of_find_bridge() or other means > > >> 3. bridge B takes a next_bridge reference to bridge C > > >> using devm_drm_of_find_bridge() > > >> 4. encoder calls (bridge B)->foo(), which in turns references > > >> next_bridge, e.g.: > > >> > > >> b_foo() { > > >> bar(b->next_bridge); > > >> } > > >> > > >> If bridges B and C are removed, bridge C can be freed but B is still > > >> allocated because the encoder holds a ref. So when step 4 happens, 'b->c' > > >> would be a use-after-free (or NULL deref if b.remove cleared it, which is > > >> just as bad). > > > > > > Yep. > > > > > >> If I got you correctly, then I'm a bit surprised by your comment. This > > >> series is part of the first chapter of the hotplug work, which does not > > >> aim > > >> at fixing everything but rather at fixing one part: handle dynamic > > >> _allocation_ lifetime of drm_bridges by adding a refcount and > > >> drm_bridge_get/put(). > > >> > > >> Chapter 2 of the work is adding drm_bridge_enter/exit/unplug() [1] and > > >> other changes in order to avoid code of drivers of removed bridges to > > >> access fields they shouldn't. So the above example at point 4 would > > >> become: > > >> > > >> b_foo() { > > >> if (!drm_bridge_enter()) > > >> return; > > >> bar(b->c); > > >> drm_bridge_exit(); > > >> } > > >> > > >> And that avoids 'b->c' after bridge B is removed. > > >> > > >> Does that answer your remark? > > > > > > Not really. I wasn't really questionning your current focus, or the way > > > you laid out the current agenda or whatever. > > > > > > What I am questionning though is whether or not we want to introduce > > > something we will have to untangle soon, and even more so when we're not > > > mentioning it anywhere. > > > > > >> > It's more complicated than it sounds, because we only have access to > > >> > the > > >> > drm_device when the bridge is attached, so later than probe. > > >> > > > >> > I wonder if we shouldn't tie the lifetime of that reference to the > > >> > lifetime of the bridge itself, and we would give up the next_bridge > > >> > reference only when we're destroyed ourselves. > > >> > > >> I'm afraid I'm not following you, sorry. Do you refer to the time between > > >> the bridge removal (driver .remove) and the last bridge put (when > > >> deallocation happens)? > > >> > > >> In that time frame the struct drm_bridge is still allocated along with > > >> any > > >> next_bridge pointer it may contain, but the following bridge could have > > >> been deallocated. > > >> > > >> What do you mean by "give up the next_bridge"? > > > > > > What I was trying to say was that if we want to fix the problem you > > > illustrated about, we need to give up the reference at __drm_bridge_free > > > time. So each bridge having a reference to a bridge would need to do so > > > in its destroy hook. > > > > > > Since it's quite a common pattern, it would make sense to add a > > > next_bridge field to drm_bridge itself, so the core can do it > > > automatically in __drm_bridge_free if that pointer is !NULL. > > > > > > But... > > > > > >> > Storing a list of all the references we need to drop is going to be > > >> > intrusive though, so maybe the easiest way to do it would be to create > > >> > a > > >> > next_bridge field in drm_bridge, and only drop the reference stored > > >> > there? > > >> > > > >> > And possibly tie the whole thing together using a helper? > > >> > > > >> > Anyway, I'm not sure it should be a prerequisite to this series. I we > > >> > do > > >> > want to go the devm_drm_of_find_bridge route however, we should at > > >> > least > > >> > document that it's unsafe, and add a TODO entry to clean up the mess > > >> > later on. > > > > > > ... I *really* don't consider it something you need to work on right now. > > > > > >> Do you mean the drm variant is unsafe while the original > > >> (drm_of_find_bridge() in this series, might be renamed) is not? I > > >> don't see how that can happen. If the driver for bridge B were to use > > >> drm_of_find_bridge(), that driver would be responsible to > > >> drm_bridge_put(b->next_bridge) in its .remove() function or earlier. > > >> So the next_bridge pointing to bridge C would equally become subject > > >> to use-after-free. > > > > > > No, I was saying that both are equally unsafe. But we're adding a new, > > > broken, helper, and we don't mention anywhere that it is. So what I was > > > saying is mostly do we really want to introduce some more broken code > > > when we know it is. And if we do, we should be really clear about it. > > > > > >> devm does not make it worse, on the opposite it postpones the > > >> drm_bridge_put(next_bridge) as late as possible: just after > > >> b.remove(). > > > > > > Which doesn't really change anything, does it? I'd expect the window > > > between the remove and final drm_bridge_put to be much wider than the > > > execution time of remove itself. > > > > > >> One final, high-level thought about the various 'next_bridge' pointers > > >> that > > >> many bridge drivers have. Most of them do: > > >> > > >> 0. have a 'struct drm_bridge next_bridge *' in their private struct > > >> 1. take the next_bridge reference during probe or another startup phase > > >> 2. store it in their private driver struct > > >> 3. use it to call drm_bridge_attach > > >> 4. (pending) put the reference to it in their .remove or earlier > > >> > > >> I'm wondering whether we could let the DRM bridge core do it all, by > > >> removing items 0, 1, 2 and 4, and change 3 as: > > >> > > >> - drm_bridge_attach(encoder, me->next_bridge, &me->bridge, flags); > > >> + drm_of_bridge_attach(encoder, &me->bridge, dev->of_node, 1, -1, > > >> flags); > > >> > > >> where dev->of_node and the following integers are the same flags passed > > >> to > > >> devm_drm_of_get_bridge() and the like, i.e. the endpoint info needed to > > >> walk the DT graph and reach the next bridge. > > >> > > >> This would allow the core to take care of all locking and lifetime of the > > >> next bridge, and most (all?) bridges would never access any pointers to > > >> the > > >> next bridge. The idea is to let the core do the right thing in a single > > >> place instead of trying to make all drivers do the right thing (and > > >> touching dozen files when needing to touch the logic). > > >> > > >> That is more a long-term ideal than something I'd do right now, but > > >> having > > >> opinions would be very interesting. > > > > > > That was pretty much my point, yeah. > > > > > > Maxime > > > > Let me recap this discussion, because there are various aspects and I need > > to clarify by view on it. > > > > First: the problem you discuss is about drm_of_find_bridge() introduced in > > patch 1. The devm variant is just equally affected. > > > > You proposed adding a next_bridge field in struct drm_bridge so there is an > > automated, common call to drm_bridge_put() (and setting it to NULL). It > > would remove some burden on individual drivers of course, but I don't think > > it would solve the problem. In the same scenario we are discussing > > (i.e. encoder --> bridge A --> bridge B --> bridge C, then B+C get removed) > > B's next_bridge would be automatically put, but the encoder could still > > call B->foo(), which could still do B->next_bridge. > > I mean, yes, that's what we have drm_bridge_enter and _exit for. > B->next_bridge would still be valid, but we also still would have a > reference to C so I'm not sure what the big deal is? > > > Additionally, as a matter of fact there are currently drivers storing > > bridge pointers. The next_bridge is the most common case. Code using > > drm_bridge_connector_init() for example can store up to eight of them, but > > individual drivers are the hardest to hunt for. > > That's fine, we can have a custom destroy hook for those. > > > I can see these (potential) tools to handle this (not mutually exclusive): > > > > 1. remove drm_bridge pointers pointing to other bridges > > 2. check whether a bridge (say B) still exists before any dereference > > to B->another_bridge: that's drm_bridge_enter/exit() > > 3. let owners of bridge pointers be notified when a bridge is unplugged, > > so they can actively put their reference and clear their pointer > > > > For item 1, I think the drm_of_bridge_attach() idea quoted above would > > work, at least for the simple cases where bridge drivers use the > > next_bridge only for attach. A next_bridge pointer in struct drm_bridge is > > not even needed in that case, the pointer would be computed from OF when > > needed and not stored. I can do an experiment and send a first series, do > > you think it would be useful? > > > > For item 2, I still think it would just work for cases not (or not yet) > > covered by item 1. Given the same example as above: > > > > b_foo() { > > if (!drm_bridge_enter()) > > return; > > bar(b->c); > > drm_bridge_exit(); > > } > > > > drm_bridge_enter() will prevent doing anything on b->whatever, including > > any stale pointers. The idea is "I, bridge B, have been unplugged, my > > resources are not reliably usable, keep out!". No? > > Yes. > > > However item 2 won't prevent the caller from keeping the pointer for a long > > time, especially if b_foo() is a void function so it cannot return anything > > saying "I was unplugged, I'm no longer theree". > > Indeed. > > > And that's where item 3 (notifiers) might come in useful: upon > > receiving a bridge removal notification any driver or other code is in > > a position to actively put its reference to the bridge and clear its > > pointer. I had proposed something in [0], which can likely be > > improved, but it is useful in my current draft solution. > > > > I'd like to know your opinions about the above points, so we can > > progressively clarify the path forward. > > I largely agree with everything you said, but that wasn't my point at > the beginning of the discussion though. My point was that you're giving > away the reference too soon with devm_drm_of_find_bridge (or by doing it > in remove). > > > PS: I just realized about a fourth option: a revocable [1] (thanks Laurent > > who pointed me to it). That's basically a wrapper around a pointer that > > allows to guard from accessing it when the pointed object is gone. To me it > > looks somewhat equivalent to drm_bridge_enter/exit() but with very > > different APIs. I still haven't looked at the details. > > Yeah, a revocable pointer would work too. It will be a pain to implement > in C though.
I discussed this with Tzung-Bi at LPC, which gave me a more detailed view of the particular issue he was trying to solve. I believe that reference-counting is a better solution than the new revocable API for the cros-ec drivers. The revocable API may still have its place in the kernel, but I increasingly think that it may be a workaround for cases where introducing reference counting would be very difficult due to historical reasons. -- Regards, Laurent Pinchart
