| Message ID | 1519070782-20834-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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). Care to look into that part of the problem, too? -Daniel > > drivers/gpu/drm/bridge/panel.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 6d99d4a..0a10be6 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -161,6 +161,10 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > if (!panel) > return ERR_PTR(-EINVAL); > > + if (WARN_ON(!panel->dev->driver) || > + !try_module_get(panel->dev->driver->owner)) > + return ERR_PTR(-ENODEV); > + > panel_bridge = devm_kzalloc(panel->dev, sizeof(*panel_bridge), > GFP_KERNEL); > if (!panel_bridge) > @@ -199,6 +203,9 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge) > panel_bridge = drm_bridge_to_panel_bridge(bridge); > > drm_bridge_remove(bridge); > + > + module_put(panel_bridge->panel->dev->driver->owner); > + > devm_kfree(panel_bridge->panel->dev, bridge); > } > EXPORT_SYMBOL(drm_panel_bridge_remove); > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
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. struct device_link was "recently" introduced to fix that issue. Thierry
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. Best regards, Jyri
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
On 20/02/18 14:03, Thierry Reding wrote: > 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. > Thanks for the hint. Adding the link indeed unbinds the master drm device when the panel is unloaded without any complaints. The annoying thing is that the master drm device does not probe again and bind when the supplier device becomes available again. However, forcing a manual bind brings the whole device back without a problem. Is there any trivial way to fix this? Best regards, Jyri > You might get away without the DL_FLAG_PM_RUNTIME because DRM should > handle that properly already.
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 6d99d4a..0a10be6 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -161,6 +161,10 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, if (!panel) return ERR_PTR(-EINVAL); + if (WARN_ON(!panel->dev->driver) || + !try_module_get(panel->dev->driver->owner)) + return ERR_PTR(-ENODEV); + panel_bridge = devm_kzalloc(panel->dev, sizeof(*panel_bridge), GFP_KERNEL); if (!panel_bridge) @@ -199,6 +203,9 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge) panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_bridge_remove(bridge); + + module_put(panel_bridge->panel->dev->driver->owner); + devm_kfree(panel_bridge->panel->dev, bridge); } EXPORT_SYMBOL(drm_panel_bridge_remove);
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. drivers/gpu/drm/bridge/panel.c | 7 +++++++ 1 file changed, 7 insertions(+)