On Tue, 7 Jan 2025 12:35:15 +0200 Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > 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. Exactly, just like drm_bridge_add/remove() do. > Just as an idea, it might be nice to add refcounting to bridges_show(), > so that we can easily verify that refcounting works correctly. Good point, cheap and useful, adding that too! Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
