Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()
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 > > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()
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 --- 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: fix reference count leaks due to pm_runtime_get_sync()
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 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel