From: Daniel Vetter <daniel@ffwll.ch> To: Paul Cercueil <paul@crapouillou.net> Cc: David Airlie <airlied@linux.ie>, Sam Ravnborg <sam@ravnborg.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, od@zcrc.me, dri-devel <dri-devel@lists.freedesktop.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, stable <stable@vger.kernel.org>, Andrzej Hajda <a.hajda@samsung.com>, Neil Armstrong <narmstrong@baylibre.com>, Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@siol.net> Subject: Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach Date: Wed, 20 Jan 2021 18:38:03 +0100 [thread overview] Message-ID: <CAKMK7uFHYPvJm46f-LXBO=nERGBBO3i_=YXZyAUi0ZXJFLmXVw@mail.gmail.com> (raw) In-Reply-To: <4YQ8NQ.HNQ7IMBKVEBV2@crapouillou.net> On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil <paul@crapouillou.net> wrote: > > > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter <daniel@ffwll.ch> a > écrit : > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> > >> If we don't call drm_connector_cleanup() manually in > >> panel_bridge_detach(), the connector will be cleaned up with the > >> other > >> DRM objects in the call to drm_mode_config_cleanup(). However, > >> since our > >> drm_connector is devm-allocated, by the time > >> drm_mode_config_cleanup() > >> will be called, our connector will be long gone. Therefore, the > >> connector must be cleaned up when the bridge is detached to avoid > >> use-after-free conditions. > > > > For -fixes this sounds ok, but for -next I think switching to drmm_ > > would be much better. > > The API would need to change to have access to the drm_device struct, > though. That would be quite a big patch, there are a few dozens source > files that use this API already. Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these. I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely. -Daniel > > Cheers, > -Paul > > > > >> v2: Cleanup connector only if it was created > >> > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper > >> from the lvds-encoder bridge.") > >> Cc: <stable@vger.kernel.org> # 4.12+ > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Cc: Neil Armstrong <narmstrong@baylibre.com> > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > >> Cc: Jonas Karlman <jonas@kwiboo.se> > >> Cc: Jernej Skrabec <jernej.skrabec@siol.net> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> drivers/gpu/drm/bridge/panel.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/panel.c > >> b/drivers/gpu/drm/bridge/panel.c > >> index 0ddc37551194..df86b0ee0549 100644 > >> --- a/drivers/gpu/drm/bridge/panel.c > >> +++ b/drivers/gpu/drm/bridge/panel.c > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge > >> *bridge, > >> > >> static void panel_bridge_detach(struct drm_bridge *bridge) > >> { > >> + struct panel_bridge *panel_bridge = > >> drm_bridge_to_panel_bridge(bridge); > >> + struct drm_connector *connector = &panel_bridge->connector; > >> + > >> + /* Cleanup the connector if we know it was initialized */ > >> + if (!!panel_bridge->connector.dev) > >> + drm_connector_cleanup(connector); > >> } > >> > >> static void panel_bridge_pre_enable(struct drm_bridge *bridge) > >> -- > >> 2.29.2 > >> > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Paul Cercueil <paul@crapouillou.net> Cc: Jernej Skrabec <jernej.skrabec@siol.net>, Neil Armstrong <narmstrong@baylibre.com>, David Airlie <airlied@linux.ie>, Jonas Karlman <jonas@kwiboo.se>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Andrzej Hajda <a.hajda@samsung.com>, od@zcrc.me, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, stable <stable@vger.kernel.org>, Sam Ravnborg <sam@ravnborg.org> Subject: Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach Date: Wed, 20 Jan 2021 18:38:03 +0100 [thread overview] Message-ID: <CAKMK7uFHYPvJm46f-LXBO=nERGBBO3i_=YXZyAUi0ZXJFLmXVw@mail.gmail.com> (raw) In-Reply-To: <4YQ8NQ.HNQ7IMBKVEBV2@crapouillou.net> On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil <paul@crapouillou.net> wrote: > > > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter <daniel@ffwll.ch> a > écrit : > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> > >> If we don't call drm_connector_cleanup() manually in > >> panel_bridge_detach(), the connector will be cleaned up with the > >> other > >> DRM objects in the call to drm_mode_config_cleanup(). However, > >> since our > >> drm_connector is devm-allocated, by the time > >> drm_mode_config_cleanup() > >> will be called, our connector will be long gone. Therefore, the > >> connector must be cleaned up when the bridge is detached to avoid > >> use-after-free conditions. > > > > For -fixes this sounds ok, but for -next I think switching to drmm_ > > would be much better. > > The API would need to change to have access to the drm_device struct, > though. That would be quite a big patch, there are a few dozens source > files that use this API already. Hm right pure drmm_ doesn't work for panel or bridge since it's usually a separate driver. But devm_ also doesn't work. I think what we need here is two-stage: first kmalloc the panel (or bridge, it's really the same) in the panel/bridge driver load. Then when we bind it to the drm_device we can tie it into the managed resources with drmm_add_action_or_reset. Passing the drm_device to the point where we allocate the panel/bridge doesn't work for these. I think minimally we need a FIXME here and ack from Laurent on how this should be solved at least, since panel bridge is used rather widely. -Daniel > > Cheers, > -Paul > > > > >> v2: Cleanup connector only if it was created > >> > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper > >> from the lvds-encoder bridge.") > >> Cc: <stable@vger.kernel.org> # 4.12+ > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Cc: Neil Armstrong <narmstrong@baylibre.com> > >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > >> Cc: Jonas Karlman <jonas@kwiboo.se> > >> Cc: Jernej Skrabec <jernej.skrabec@siol.net> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> drivers/gpu/drm/bridge/panel.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/panel.c > >> b/drivers/gpu/drm/bridge/panel.c > >> index 0ddc37551194..df86b0ee0549 100644 > >> --- a/drivers/gpu/drm/bridge/panel.c > >> +++ b/drivers/gpu/drm/bridge/panel.c > >> @@ -87,6 +87,12 @@ static int panel_bridge_attach(struct drm_bridge > >> *bridge, > >> > >> static void panel_bridge_detach(struct drm_bridge *bridge) > >> { > >> + struct panel_bridge *panel_bridge = > >> drm_bridge_to_panel_bridge(bridge); > >> + struct drm_connector *connector = &panel_bridge->connector; > >> + > >> + /* Cleanup the connector if we know it was initialized */ > >> + if (!!panel_bridge->connector.dev) > >> + drm_connector_cleanup(connector); > >> } > >> > >> static void panel_bridge_pre_enable(struct drm_bridge *bridge) > >> -- > >> 2.29.2 > >> > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-01-20 19:00 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-20 12:35 [PATCH v2 0/3] Fixes to bridge/panel and ingenic-drm Paul Cercueil 2021-01-20 12:35 ` Paul Cercueil 2021-01-20 12:35 ` [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach Paul Cercueil 2021-01-20 12:35 ` Paul Cercueil 2021-01-20 16:03 ` Daniel Vetter 2021-01-20 16:03 ` Daniel Vetter 2021-01-20 16:25 ` Paul Cercueil 2021-01-20 16:25 ` Paul Cercueil 2021-01-20 17:38 ` Daniel Vetter [this message] 2021-01-20 17:38 ` Daniel Vetter 2021-03-24 2:15 ` Laurent Pinchart 2021-03-24 2:15 ` Laurent Pinchart 2021-03-24 9:39 ` Daniel Vetter 2021-03-24 9:39 ` Daniel Vetter 2021-03-24 9:46 ` Laurent Pinchart 2021-03-24 9:46 ` Laurent Pinchart 2021-01-20 12:35 ` [PATCH v2 2/3] drm/ingenic: Register devm action to cleanup encoders Paul Cercueil 2021-01-20 12:35 ` Paul Cercueil 2021-01-20 13:01 ` Daniel Vetter 2021-01-20 13:01 ` Daniel Vetter 2021-01-20 13:21 ` Paul Cercueil 2021-01-20 13:21 ` Paul Cercueil 2021-01-20 14:04 ` Daniel Vetter 2021-01-20 14:04 ` Daniel Vetter 2021-01-20 15:55 ` Paul Cercueil 2021-01-20 15:55 ` Paul Cercueil 2021-01-20 12:35 ` [PATCH v2 3/3] drm/ingenic: Fix non-OSD mode Paul Cercueil 2021-01-20 12:35 ` Paul Cercueil 2021-01-20 16:26 ` Daniel Vetter 2021-01-20 16:26 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKMK7uFHYPvJm46f-LXBO=nERGBBO3i_=YXZyAUi0ZXJFLmXVw@mail.gmail.com' \ --to=daniel@ffwll.ch \ --cc=a.hajda@samsung.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=jernej.skrabec@siol.net \ --cc=jonas@kwiboo.se \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=narmstrong@baylibre.com \ --cc=od@zcrc.me \ --cc=paul@crapouillou.net \ --cc=sam@ravnborg.org \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.