| Message ID | 20200915233832.19769-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2a32dbdc2c7db5463483fa01fb220fd1b770c6bc |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Put reference to VSP device | expand |
Hi Laurent, On 16/09/2020 00:38, Laurent Pinchart wrote: > The reference to the VSP device acquired with of_find_device_by_node() > in rcar_du_vsp_init() is never released. Fix it with a drmm action, > which gets run both in the probe error path and in the remove path. > > Fixes: 6d62ef3ac30b ("drm: rcar-du: Expose the VSP1 compositor through KMS planes") > Reported-by: Yu Kuai <yukuai3@huawei.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Looks nice and clean! Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > index f1a81c9b184d..fa09b3ae8b9d 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > @@ -13,6 +13,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_vblank.h> > > @@ -341,6 +342,13 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > }; > > +static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res) > +{ > + struct rcar_du_vsp *vsp = res; > + > + put_device(vsp->vsp); Ugh the asymmetry of the put_device is a bit annoying, because it's not initially clear that the of_find_device_by_node() call 'gets' a reference. (Or at least not until you find: https://lore.kernel.org/patchwork/patch/731284/) It is stated in the commit message though so that's fine, and although I thought perhaps a comment here might be useful to declare that it releases the reference taken by of_find_device_by_node(), I'm not sure it even adds that much value ... so either way. > +} > + > int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > unsigned int crtcs) > { > @@ -357,6 +365,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > > vsp->vsp = &pdev->dev; > > + ret = drmm_add_action(rcdu->ddev, rcar_du_vsp_cleanup, vsp); > + if (ret < 0) > + return ret; > + > ret = vsp1_du_init(vsp->vsp); > if (ret < 0) > return ret; >
Hi Kieran, On Wed, Sep 16, 2020 at 11:26:36AM +0100, Kieran Bingham wrote: > On 16/09/2020 00:38, Laurent Pinchart wrote: > > The reference to the VSP device acquired with of_find_device_by_node() > > in rcar_du_vsp_init() is never released. Fix it with a drmm action, > > which gets run both in the probe error path and in the remove path. > > > > Fixes: 6d62ef3ac30b ("drm: rcar-du: Expose the VSP1 compositor through KMS planes") > > Reported-by: Yu Kuai <yukuai3@huawei.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Looks nice and clean! > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > --- > > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > index f1a81c9b184d..fa09b3ae8b9d 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > > @@ -13,6 +13,7 @@ > > #include <drm/drm_fourcc.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_plane_helper.h> > > #include <drm/drm_vblank.h> > > > > @@ -341,6 +342,13 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { > > .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, > > }; > > > > +static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res) > > +{ > > + struct rcar_du_vsp *vsp = res; > > + > > + put_device(vsp->vsp); > > Ugh the asymmetry of the put_device is a bit annoying, because it's not > initially clear that the of_find_device_by_node() call 'gets' a reference. > > (Or at least not until you find: > https://lore.kernel.org/patchwork/patch/731284/) > > It is stated in the commit message though so that's fine, and although I > thought perhaps a comment here might be useful to declare that it > releases the reference taken by of_find_device_by_node(), I'm not sure > it even adds that much value ... so either way. I think that the fact that drmm_add_action() is called right after of_find_device_by_node() makes this explicit enough. > > +} > > + > > int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > > unsigned int crtcs) > > { > > @@ -357,6 +365,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, > > > > vsp->vsp = &pdev->dev; > > > > + ret = drmm_add_action(rcdu->ddev, rcar_du_vsp_cleanup, vsp); > > + if (ret < 0) > > + return ret; > > + > > ret = vsp1_du_init(vsp->vsp); > > if (ret < 0) > > return ret; > > >
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index f1a81c9b184d..fa09b3ae8b9d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -13,6 +13,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_vblank.h> @@ -341,6 +342,13 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, }; +static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res) +{ + struct rcar_du_vsp *vsp = res; + + put_device(vsp->vsp); +} + int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, unsigned int crtcs) { @@ -357,6 +365,10 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, vsp->vsp = &pdev->dev; + ret = drmm_add_action(rcdu->ddev, rcar_du_vsp_cleanup, vsp); + if (ret < 0) + return ret; + ret = vsp1_du_init(vsp->vsp); if (ret < 0) return ret;
The reference to the VSP device acquired with of_find_device_by_node() in rcar_du_vsp_init() is never released. Fix it with a drmm action, which gets run both in the probe error path and in the remove path. Fixes: 6d62ef3ac30b ("drm: rcar-du: Expose the VSP1 compositor through KMS planes") Reported-by: Yu Kuai <yukuai3@huawei.com> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)