Re: [PATCH v2 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

On Wed, Mar 24, 2021 at 10:39:52AM +0100, Daniel Vetter wrote:
> On Wed, Mar 24, 2021 at 04:15:37AM +0200, Laurent Pinchart wrote:
> > On Wed, Jan 20, 2021 at 06:38:03PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 20, 2021 at 6:12 PM Paul Cercueil wrote:
> > > > Le mer. 20 janv. 2021 à 17:03, Daniel Vetter a écrit :
> > > > > On Wed, Jan 20, 2021 at 1:35 PM Paul Cercueil 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.
> > 
> > Bridge removal is completely broken. If you unbind a bridge driver from
> > the device, the bridge will be unregistered and resources freed, without
> > the display driver knowing about this. The lifetime of the drm_bridge
> > structure itself isn't the only issue to be addressed here, it's broader
> > than that, and needs to consider that the display driver could be
> > calling the bridge operations concurrently to the removal.
> 
> So for the "unloading bridge should first unload display" problem that was
> supposed to get fixed with device links. There was at least a patch for
> that, and I Rafel from pm side did all the core changes to make it work.
> But it didn't land I think, so things keep on sucking.
> 
> Ofc the lifetime of the bridge structure is then an additional problem on
> top here.

There's a set of interesting problems. I don't think it's impossible,
but it will require someone with a good understanding of the problem (as
that person would really need to see the big picture, and take all use
cases into account), and a large amount of time and motivation.

> > We need a volunteer with enough motivation to solve this subsystem-wide
> > :-) In the meantime, whatever shortcut addresses immediate issues is
> > probably fine, as yak-shaving in this area would definitely not be
> > reasonable.
> 
> I guess drm/bridge keeps on disappointing :-/

I usually blame the x86 folks for not caring enough about bridges
initially, resulting in it being a second class citizen ;-)

> > > > >> v2: Cleanup connector only if it was created
> > > > >>
> > > > >> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> > > > >> Cc: <stable@xxxxxxxxxxxxxxx> # 4.12+
> > > > >> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > > > >> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > > > >> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > >> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> > > > >> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > >> Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > > >> ---
> > > > >>  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)

-- 
Regards,

Laurent Pinchart