[Intel-gfx] [PATCH 37/52] drm/rcar-du: Drop explicit drm_mode_config_cleanup call

[Daniel Vetter]

On Wed, Feb 19, 2020 at 2:53 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Wed, Feb 19, 2020 at 11:21:07AM +0100, Daniel Vetter wrote:
> > It's right above the drm_dev_put().
>
> Could you mention in the commit message that the call can be dropped
> because drm_mode_config_init() uses the managed API to handle cleaning
> automatically, removing the need to do so in drivers ? Otherwise when
> someone will look at the commit later, without having the full context
> in mind, the reason why the call is dropped won't be immediately clear.
> With this fixed,

Yeah I need to add that, since that explains the need for checking the
return value of drm_mode_config_init.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> This also applies to similar patches for other drivers.
>
> > Aside: Another driver with a bit much devm_kzalloc, which should
> > probably use drmm_kzalloc instead ...
>
> I agree, but I'm not sure this should be part of the commit message :-)

I'm trying to use this patch series as an education campaign about the
dangers of devm_kzalloc. Hence why I tried to touch as many drivers as
feasible (the ones I've did not touched have even more fundamental
lifetime issues and would blow up simply by switching to drm_dev_put()
for some reason or another). You alredy understand this stuff, so it's
a bit redundant here for your driver ...

I'm not sure about the other devm_kzalloc in rcar-du, but the one in
rcar_du_encoder_init seems to contain a drm_encoder, and drm_encoder
is a userspace visible thing. The others would need careful analysis,
but as a defensive move I'd e.g. not devm_kzalloc your driver private
structure behind drm_device->dev_private. It can work, but just a bit
too risky imo and hard to review for correctness.
-Daniel

> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Cc: Kieran Bingham <kieran.bingham+renesas at ideasonboard.com>
> > Cc: linux-renesas-soc at vger.kernel.org
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 1 -
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 4 +++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > index 654e2dd08146..3e67cf70f040 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> > @@ -530,7 +530,6 @@ static int rcar_du_remove(struct platform_device *pdev)
> >       drm_dev_unregister(ddev);
> >
> >       drm_kms_helper_poll_fini(ddev);
> > -     drm_mode_config_cleanup(ddev);
> >
> >       drm_dev_put(ddev);
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index fcfd916227d1..dcdc1580b511 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -712,7 +712,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
> >       unsigned int i;
> >       int ret;
> >
> > -     drm_mode_config_init(dev);
> > +     ret = drm_mode_config_init(dev);
> > +     if (ret)
> > +             return ret;
> >
> >       dev->mode_config.min_width = 0;
> >       dev->mode_config.min_height = 0;
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch