[PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()

[PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()

[Aditya Pakki]

On calling pm_runtime_get_sync() the reference count of the device
is incremented. In case of failure, decrement the
reference count before returning the error.

Signed-off-by: Aditya Pakki <pakki001@umn.edu>
---
 drivers/gpu/drm/bridge/cdns-dsi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
index 69c3892caee5..583cb8547106 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -788,8 +788,10 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	u32 tmp, reg_wakeup, div;
 	int nlanes;
 
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
+	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0)) {
+		pm_runtime_put(dsi->base.dev);
 		return;
+	}
 
 	mode = &bridge->encoder->crtc->state->adjusted_mode;
 	nlanes = output->dev->lanes;
@@ -1028,8 +1030,10 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
 	int ret, i, tx_len, rx_len;
 
 	ret = pm_runtime_get_sync(host->dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put(host->dev);
 		return ret;
+	}
 
 	cdns_dsi_init_link(dsi);
 
-- 
2.25.1

Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()

[Laurent Pinchart]

Hi Aditya,

(CC'ing Rafael)

Thank you for the patch.

On Sat, Jun 13, 2020 at 09:40:05PM -0500, Aditya Pakki wrote:
> On calling pm_runtime_get_sync() the reference count of the device
> is incremented. In case of failure, decrement the
> reference count before returning the error.
> 
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>

I've seen lots of similar patches recently. Instead of mass-patching the
drivers this way, shouldn't pm_runtime_get_sync() (and similar
functions) decrease the refcount on their failure path ? That would
require patching drivers that already handle this issue, but I believe
that would cause less churn, and more importantly, avoid the issue once
and for good for new code.

> ---
>  drivers/gpu/drm/bridge/cdns-dsi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> index 69c3892caee5..583cb8547106 100644
> --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -788,8 +788,10 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
>  	u32 tmp, reg_wakeup, div;
>  	int nlanes;
>  
> -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> +	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0)) {
> +		pm_runtime_put(dsi->base.dev);
>  		return;
> +	}
>  
>  	mode = &bridge->encoder->crtc->state->adjusted_mode;
>  	nlanes = output->dev->lanes;
> @@ -1028,8 +1030,10 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
>  	int ret, i, tx_len, rx_len;
>  
>  	ret = pm_runtime_get_sync(host->dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put(host->dev);
>  		return ret;
> +	}
>  
>  	cdns_dsi_init_link(dsi);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()

[Daniel Vetter]

On Sun, Jun 14, 2020 at 04:46:55PM +0300, Laurent Pinchart wrote:
> Hi Aditya,
> 
> (CC'ing Rafael)
> 
> Thank you for the patch.
> 
> On Sat, Jun 13, 2020 at 09:40:05PM -0500, Aditya Pakki wrote:
> > On calling pm_runtime_get_sync() the reference count of the device
> > is incremented. In case of failure, decrement the
> > reference count before returning the error.
> > 
> > Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> 
> I've seen lots of similar patches recently. Instead of mass-patching the
> drivers this way, shouldn't pm_runtime_get_sync() (and similar
> functions) decrease the refcount on their failure path ? That would
> require patching drivers that already handle this issue, but I believe
> that would cause less churn, and more importantly, avoid the issue once
> and for good for new code.

Yeah the current interface looks rather dodgy, generally drivers aren't
expected to clean up if the first function failed.

Rafael and Greg, what's your take on this here? See patch below and
Laurent's comment above.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/bridge/cdns-dsi.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
> > index 69c3892caee5..583cb8547106 100644
> > --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> > +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> > @@ -788,8 +788,10 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
> >  	u32 tmp, reg_wakeup, div;
> >  	int nlanes;
> >  
> > -	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> > +	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0)) {
> > +		pm_runtime_put(dsi->base.dev);
> >  		return;
> > +	}
> >  
> >  	mode = &bridge->encoder->crtc->state->adjusted_mode;
> >  	nlanes = output->dev->lanes;
> > @@ -1028,8 +1030,10 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
> >  	int ret, i, tx_len, rx_len;
> >  
> >  	ret = pm_runtime_get_sync(host->dev);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		pm_runtime_put(host->dev);
> >  		return ret;
> > +	}
> >  
> >  	cdns_dsi_init_link(dsi);
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch