* [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync() @ 2020-06-14 2:40 Aditya Pakki 2020-06-14 13:46 ` Laurent Pinchart 0 siblings, 1 reply; 3+ messages in thread From: Aditya Pakki @ 2020-06-14 2:40 UTC (permalink / raw) To: pakki001 Cc: kjlu, wu000273, Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, dri-devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync() 2020-06-14 2:40 [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync() Aditya Pakki @ 2020-06-14 13:46 ` Laurent Pinchart 2020-06-16 12:10 ` Daniel Vetter 0 siblings, 1 reply; 3+ messages in thread From: Laurent Pinchart @ 2020-06-14 13:46 UTC (permalink / raw) To: Aditya Pakki Cc: kjlu, wu000273, Andrzej Hajda, Neil Armstrong, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, dri-devel, linux-kernel, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync() 2020-06-14 13:46 ` Laurent Pinchart @ 2020-06-16 12:10 ` Daniel Vetter 0 siblings, 0 replies; 3+ messages in thread From: Daniel Vetter @ 2020-06-16 12:10 UTC (permalink / raw) To: Laurent Pinchart, Greg KH Cc: Aditya Pakki, kjlu, wu000273, Andrzej Hajda, Neil Armstrong, Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter, dri-devel, linux-kernel, Rafael J. Wysocki 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-16 12:11 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 2:40 [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync() Aditya Pakki 2020-06-14 13:46 ` Laurent Pinchart 2020-06-16 12:10 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).