On Mon, Jan 06, 2025 at 03:49:48PM +0100, Maxime Ripard wrote: > On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote: > > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > Most of these comments affect your earlier patches, but let's work on > > > the API-level view. > > > > > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote: > > > if (IS_ERR(priv)) > > > return ERR_PTR(priv); > > > bridge = &priv->bridge; > > > > > > ... > > > > > > drm_bridge_add(bridge); > > > } > > > > > > Would work just as well. > > > > > > I also don't think we need explicit (at least for the common case) > > > drm_bridge_get and drm_bridge_put calls for bridge users. > > > drm_bridge_attach and drm_bridge_detach can get/put the reference > > > directly. > > > > As I wrote previously, I think drm_bridge_attach() might be too late for that. > > It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge > > should increment the refcount, possibly adding a devres action to put > > the reference. > > We probably need both. drm_bridge_attach adds the bridge pointer to the > encoder bridge_chain list, so if we had something like > > bridge = drm_of_find_bridge(); > drm_bridge_attach(encoder, bridge); > drm_bridge_put(bridge); > > We could have a dangling pointer. Yes... So, both drm_bridge_attach and drm_of_find_bridge() should take the refcount. Just as an idea, it might be nice to add refcounting to bridges_show(), so that we can easily verify that refcounting works correctly. -- With best wishes Dmitry
