Re: [PATCH RFC] drm/bridge: panel: Add module_get/but calls to attached panel driver

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4295 bytes --]

On Tue, Feb 20, 2018 at 01:28:48PM +0200, Jyri Sarha wrote:
> On 20/02/18 12:34, Thierry Reding wrote:
> > On Mon, Feb 19, 2018 at 11:59:23PM +0100, Daniel Vetter wrote:
> >> On Mon, Feb 19, 2018 at 10:06:22PM +0200, Jyri Sarha wrote:
> >>> Currently there is no way for a master drm driver to protect against an
> >>> attached panel driver from being unloaded while it is in use. The
> >>> least we can do is to indicate the usage by incrementing the module
> >>> reference count.
> >>>
> >>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>> cc: eric@anholt.net
> >>> cc: laurent.pinchart@ideasonboard.com
> >>> ---
> >>> I do not see any module_get/put code in drm core. Is there is a reason
> >>> for that?
> >>>
> >>> There is two more alternative places for adding the module_get/put
> >>> code. One is puting it directly to drm_panel_attach() and
> >>> drm_panel_detach(). However, if the same module implements both the
> >>> master drm driver and the panel (like tilcdc does with its
> >>> tilcdc_panel.c), then attaching the panel will lock the module in for
> >>> no good reason. Still, this solution should work with drm bridges as I
> >>> do not see any reason why anybody would implement bridge drivers in
> >>> the same module with the master drm driver.
> >>>
> >>> The other place to put the code would in the master drm driver. But
> >>> for handling the situation with bridges would need the device pointer
> >>> in struct drm_bridge.
> >>
> >> I think this looks like a reasonable place to do this. Looking at the code
> >> we seem to have a similar issue with the bridge driver itself. I think
> >> we need to wire through the module owner stuff and add a try_modeul_get to
> >> of_drm_find_bridge (and any other helper used to find bridge instances).
> > 
> > I disagree. module_get() is only going to protect you from unloading a
> > module that's in use, but there are other ways to unbind a driver from
> > the device and cause subsequent mayhem.
> > 
> 
> Yes. This is not a full fix for the issue. But AFAIK at the moment there
> is no infrastructure to tear down the whole drm device grace fully when
> a bridge driver or panel is removed. So this should be considered as a
> partial remedy protecting user from accidentally crashing the drm driver
> by unloading the wrong module first. That is why I wrote "least we can do".
> 
> > struct device_link was "recently" introduced to fix that issue.
> > 
> 
> Unfortunately I do not have time to do full fix for the issue, but in
> any case I think this patch is a step to the right direction. The module
> reference count should anyway be increased at least to know that the
> driver code remains in memory, even if the device is not there any more.
> 
> Then again the display panels or bridges are hardly ever hot pluggable
> devices, so they do not normally vanish just like that, unless someone
> is being nasty and forcibly unbinding them. But yes, I know that is a
> bad excuse for broken behaviour.

I think device links are actually a lot easier to use and give you a
full solution for this. Essentially the only thing you need to do is add
a call to device_link_add() in the correct place.

That said, it seems to me like drm_panel_bridge_add() is not a good
place to add this, because the panel/bridge does not follow a typical
consumer/supplier model. It is rather a helper construct with a well-
defined lifetime.

What you do want to protect is the panel (the "parent" of the panel/
bridge) from going away unexpectedly. I think the best way to achieve
that is to make the link in drm_panel_attach() with something like
this:

	u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE;
	struct device *consumer = connector->dev->dev;

	link = device_link_add(consumer, panel->dev, flags);
	if (!link) {
		dev_err(panel->dev, "failed to link panel %s to %s\n",
			dev_name(panel->dev), dev_name(consumer));
		return -EINVAL;
	}

Ideally I think we'd link the panel to the connector rather than the
top-level DRM device, but we don't have a struct device * for that, so
this is probably as good as it gets for now.

You might get away without the DL_FLAG_PM_RUNTIME because DRM should
handle that properly already.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel