[v2,04/21] drm/exynos: Fix potential unbalanced calls to pm_runtime_put
diff mbox series

Message ID 20190826152649.13820-5-boris.brezillon@collabora.com
State New
Headers show
Series
  • drm: Add support for bus-format negotiation
Related show

Commit Message

Boris Brezillon Aug. 26, 2019, 3:26 p.m. UTC
The encoder->enable() can't report errors and is expected to always
succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error
path (as currently done) we'll have unbalanced get/put calls when
encoder->disable() is called.

The situation is not ideal since drm_panel_{prepare,enable}() can
theoretically return an error (even if in practice I don't think any
panel driver does that). Putting a WARN_ON() is the best we can do,
unfortunately. Note that -ENOSYS is actually a valid case, it just
means the panel driver does not implement the hook.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v2:
* New patch
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

Boris Brezillon Oct. 11, 2019, 1:31 p.m. UTC | #1
Hi Inki,

On Mon, 26 Aug 2019 17:26:32 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> The encoder->enable() can't report errors and is expected to always
> succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error
> path (as currently done) we'll have unbalanced get/put calls when
> encoder->disable() is called.
> 
> The situation is not ideal since drm_panel_{prepare,enable}() can
> theoretically return an error (even if in practice I don't think any
> panel driver does that). Putting a WARN_ON() is the best we can do,
> unfortunately. Note that -ENOSYS is actually a valid case, it just
> means the panel driver does not implement the hook.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Did you have a chance to look at this patch 4 and 5 of this series? I'd
really like to get those 2 patches merged.

Thanks,

Boris

> ---
> Changes in v2:
> * New patch
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 8e655ae1fb0c..c555cecfe1f5 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1387,8 +1387,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>  
>  	if (dsi->panel) {
>  		ret = drm_panel_prepare(dsi->panel);
> -		if (ret < 0)
> -			goto err_put_sync;
> +		WARN_ON(ret && ret != -ENOSYS);
>  	} else {
>  		drm_bridge_pre_enable(dsi->out_bridge);
>  	}
> @@ -1398,22 +1397,13 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>  
>  	if (dsi->panel) {
>  		ret = drm_panel_enable(dsi->panel);
> -		if (ret < 0)
> -			goto err_display_disable;
> +		WARN_ON(ret && ret != -ENOSYS);
>  	} else {
>  		drm_bridge_enable(dsi->out_bridge);
>  	}
>  
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  	return;
> -
> -err_display_disable:
> -	exynos_dsi_set_display_enable(dsi, false);
> -	drm_panel_unprepare(dsi->panel);
> -
> -err_put_sync:
> -	dsi->state &= ~DSIM_STATE_ENABLED;
> -	pm_runtime_put(dsi->dev);
>  }
>  
>  static void exynos_dsi_disable(struct drm_encoder *encoder)
Andrzej Hajda Oct. 11, 2019, 1:54 p.m. UTC | #2
On 26.08.2019 17:26, Boris Brezillon wrote:
> The encoder->enable() can't report errors and is expected to always
> succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error
> path (as currently done) we'll have unbalanced get/put calls when
> encoder->disable() is called.


True


>
> The situation is not ideal since drm_panel_{prepare,enable}() can
> theoretically return an error (even if in practice I don't think any
> panel driver does that).


So why do you want to fix it at all, if you think return value is always
0 :) ?


git grep -p -A30 '_prepare' drivers/gpu/drm/panel/ shows that many of
them can return errors.


>  Putting a WARN_ON() is the best we can do,
> unfortunately.


I guess optimally we should use DRM_MODE_LINK_STATUS_BAD, but I am not
sure how exactly it should be handled.


> Note that -ENOSYS is actually a valid case, it just
> means the panel driver does not implement the hook.


It would be good then to fix it in panel framework, ie without hook
drm_panel_* function should return 0, ENOSYS makes no sense here.


Regards

Andrzej


>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v2:
> * New patch
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 8e655ae1fb0c..c555cecfe1f5 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1387,8 +1387,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>  
>  	if (dsi->panel) {
>  		ret = drm_panel_prepare(dsi->panel);
> -		if (ret < 0)
> -			goto err_put_sync;
> +		WARN_ON(ret && ret != -ENOSYS);
>  	} else {
>  		drm_bridge_pre_enable(dsi->out_bridge);
>  	}
> @@ -1398,22 +1397,13 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
>  
>  	if (dsi->panel) {
>  		ret = drm_panel_enable(dsi->panel);
> -		if (ret < 0)
> -			goto err_display_disable;
> +		WARN_ON(ret && ret != -ENOSYS);
>  	} else {
>  		drm_bridge_enable(dsi->out_bridge);
>  	}
>  
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  	return;
> -
> -err_display_disable:
> -	exynos_dsi_set_display_enable(dsi, false);
> -	drm_panel_unprepare(dsi->panel);
> -
> -err_put_sync:
> -	dsi->state &= ~DSIM_STATE_ENABLED;
> -	pm_runtime_put(dsi->dev);
>  }
>  
>  static void exynos_dsi_disable(struct drm_encoder *encoder)
Boris Brezillon Oct. 11, 2019, 2:20 p.m. UTC | #3
On Fri, 11 Oct 2019 15:54:53 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 26.08.2019 17:26, Boris Brezillon wrote:
> > The encoder->enable() can't report errors and is expected to always
> > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error
> > path (as currently done) we'll have unbalanced get/put calls when
> > encoder->disable() is called.  
> 
> 
> True
> 
> 
> >
> > The situation is not ideal since drm_panel_{prepare,enable}() can
> > theoretically return an error (even if in practice I don't think any
> > panel driver does that).  
> 
> 
> So why do you want to fix it at all, if you think return value is always
> 0 :) ?
> 
> 
> git grep -p -A30 '_prepare' drivers/gpu/drm/panel/ shows that many of
> them can return errors.

Then I was wrong :-).

> 
> 
> >  Putting a WARN_ON() is the best we can do,
> > unfortunately.  
> 
> 
> I guess optimally we should use DRM_MODE_LINK_STATUS_BAD, but I am not
> sure how exactly it should be handled.

You mean call
drm_connector_set_link_status_property(DRM_MODE_LINK_STATUS_BAD) ?

> 
> 
> > Note that -ENOSYS is actually a valid case, it just
> > means the panel driver does not implement the hook.  
> 
> 
> It would be good then to fix it in panel framework, ie without hook
> drm_panel_* function should return 0, ENOSYS makes no sense here.

I'm fine with that. Thierry, Sam, any opinion?
Sam Ravnborg Oct. 12, 2019, 7:15 a.m. UTC | #4
Hi Boris/Andrzej.

> 
> > 
> > 
> > > Note that -ENOSYS is actually a valid case, it just
> > > means the panel driver does not implement the hook.  
> > 
> > 
> > It would be good then to fix it in panel framework, ie without hook
> > drm_panel_* function should return 0, ENOSYS makes no sense here.
> 
> I'm fine with that. Thierry, Sam, any opinion?

Agreed, I have following patch in my panel patch queue:

drm/drm_panel: no error when no callback

    The callbacks in drm_panel_funcs are optional, so do not
    return an error just because no callback is assigned.

    Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

If I get time this weekend I will rebase/test and send the
set of patches out.

	Sam
Boris Brezillon Oct. 22, 2019, 9:14 a.m. UTC | #5
On Sat, 12 Oct 2019 09:15:26 +0200
Sam Ravnborg <sam@ravnborg.org> wrote:

> Hi Boris/Andrzej.
> 
> >   
> > > 
> > >   
> > > > Note that -ENOSYS is actually a valid case, it just
> > > > means the panel driver does not implement the hook.    
> > > 
> > > 
> > > It would be good then to fix it in panel framework, ie without hook
> > > drm_panel_* function should return 0, ENOSYS makes no sense here.  
> > 
> > I'm fine with that. Thierry, Sam, any opinion?  
> 
> Agreed, I have following patch in my panel patch queue:
> 
> drm/drm_panel: no error when no callback
> 
>     The callbacks in drm_panel_funcs are optional, so do not
>     return an error just because no callback is assigned.
> 
>     Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> 
> If I get time this weekend I will rebase/test and send the
> set of patches out.

Any progress on that? Can I rebase/send it for you if you don't have
time?
Boris Brezillon Oct. 22, 2019, 5:23 p.m. UTC | #6
On Fri, 11 Oct 2019 15:54:53 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 26.08.2019 17:26, Boris Brezillon wrote:
> > The encoder->enable() can't report errors and is expected to always
> > succeed. If we call pm_runtime_put() in the exynos_dsi_enable() error
> > path (as currently done) we'll have unbalanced get/put calls when
> > encoder->disable() is called.  
> 
> 
> True

I just realized this is actually not the case, because the
DSIM_STATE_ENABLED flag is cleared in the error path, and
exynos_dsi_disable() bails out early when DSIM_STATE_ENABLED is not set.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 8e655ae1fb0c..c555cecfe1f5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1387,8 +1387,7 @@  static void exynos_dsi_enable(struct drm_encoder *encoder)
 
 	if (dsi->panel) {
 		ret = drm_panel_prepare(dsi->panel);
-		if (ret < 0)
-			goto err_put_sync;
+		WARN_ON(ret && ret != -ENOSYS);
 	} else {
 		drm_bridge_pre_enable(dsi->out_bridge);
 	}
@@ -1398,22 +1397,13 @@  static void exynos_dsi_enable(struct drm_encoder *encoder)
 
 	if (dsi->panel) {
 		ret = drm_panel_enable(dsi->panel);
-		if (ret < 0)
-			goto err_display_disable;
+		WARN_ON(ret && ret != -ENOSYS);
 	} else {
 		drm_bridge_enable(dsi->out_bridge);
 	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
 	return;
-
-err_display_disable:
-	exynos_dsi_set_display_enable(dsi, false);
-	drm_panel_unprepare(dsi->panel);
-
-err_put_sync:
-	dsi->state &= ~DSIM_STATE_ENABLED;
-	pm_runtime_put(dsi->dev);
 }
 
 static void exynos_dsi_disable(struct drm_encoder *encoder)