[37/52] drm/rcar-du: Drop explicit drm_mode_config_cleanup call
diff mbox series

Message ID 20200219102122.1607365-38-daniel.vetter@ffwll.ch
State New
Delegated to: Kieran Bingham
Headers show
Series
  • Untitled series #243903
Related show

Commit Message

Daniel Vetter Feb. 19, 2020, 10:21 a.m. UTC
It's right above the drm_dev_put().

Aside: Another driver with a bit much devm_kzalloc, which should
probably use drmm_kzalloc instead ...

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@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(-)

Comments

Geert Uytterhoeven Feb. 19, 2020, 10:30 a.m. UTC | #1
Hi Daniel,

On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It's right above the drm_dev_put().
>
> Aside: Another driver with a bit much devm_kzalloc, which should
> probably use drmm_kzalloc instead ...

What's drmm_kzalloc()?
The only references I can find are in this patch series.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Daniel Vetter Feb. 19, 2020, 10:56 a.m. UTC | #2
On Wed, Feb 19, 2020 at 11:30 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Daniel,
>
> On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > It's right above the drm_dev_put().
> >
> > Aside: Another driver with a bit much devm_kzalloc, which should
> > probably use drmm_kzalloc instead ...
>
> What's drmm_kzalloc()?
> The only references I can find are in this patch series.

Yup, it's all new. Read cover letter for reading instructions for the
entire patch series. I'm afraid the driver patches wont make much
sense without the context. None actually :-/
-Daniel

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Geert Uytterhoeven Feb. 19, 2020, 11:10 a.m. UTC | #3
Hi Daniel,

On Wed, Feb 19, 2020 at 11:57 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Feb 19, 2020 at 11:30 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > It's right above the drm_dev_put().
> > >
> > > Aside: Another driver with a bit much devm_kzalloc, which should
> > > probably use drmm_kzalloc instead ...
> >
> > What's drmm_kzalloc()?
> > The only references I can find are in this patch series.
>
> Yup, it's all new. Read cover letter for reading instructions for the
> entire patch series. I'm afraid the driver patches wont make much
> sense without the context. None actually :-/

IC, as the cover letter was sent only to dri-devel and intel-gfx, many
recipients of the patches won't have received it...
https://lore.kernel.org/dri-devel/20200219102122.1607365-1-daniel.vetter@ffwll.ch/

Gr{oetje,eeting}s,

                        Geert
Laurent Pinchart Feb. 19, 2020, 12:17 p.m. UTC | #4
Hi Daniel,

On Wed, Feb 19, 2020 at 12:10:18PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 19, 2020 at 11:57 AM Daniel Vetter wrote:
> > On Wed, Feb 19, 2020 at 11:30 AM Geert Uytterhoeven wrote:
> > > On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter wrote:
> > > > It's right above the drm_dev_put().
> > > >
> > > > Aside: Another driver with a bit much devm_kzalloc, which should
> > > > probably use drmm_kzalloc instead ...
> > >
> > > What's drmm_kzalloc()?
> > > The only references I can find are in this patch series.
> >
> > Yup, it's all new. Read cover letter for reading instructions for the
> > entire patch series. I'm afraid the driver patches wont make much
> > sense without the context. None actually :-/
> 
> IC, as the cover letter was sent only to dri-devel and intel-gfx, many
> recipients of the patches won't have received it...
> https://lore.kernel.org/dri-devel/20200219102122.1607365-1-daniel.vetter@ffwll.ch/

I was also going to mention that it would be nice to send the cover
letter to all recipients from the series, otherwise it's a bit painful.
Daniel, is this something that could be integrated in your workflow ?
Daniel Vetter Feb. 19, 2020, 12:40 p.m. UTC | #5
On Wed, Feb 19, 2020 at 02:17:27PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wed, Feb 19, 2020 at 12:10:18PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Feb 19, 2020 at 11:57 AM Daniel Vetter wrote:
> > > On Wed, Feb 19, 2020 at 11:30 AM Geert Uytterhoeven wrote:
> > > > On Wed, Feb 19, 2020 at 11:22 AM Daniel Vetter wrote:
> > > > > It's right above the drm_dev_put().
> > > > >
> > > > > Aside: Another driver with a bit much devm_kzalloc, which should
> > > > > probably use drmm_kzalloc instead ...
> > > >
> > > > What's drmm_kzalloc()?
> > > > The only references I can find are in this patch series.
> > >
> > > Yup, it's all new. Read cover letter for reading instructions for the
> > > entire patch series. I'm afraid the driver patches wont make much
> > > sense without the context. None actually :-/
> > 
> > IC, as the cover letter was sent only to dri-devel and intel-gfx, many
> > recipients of the patches won't have received it...
> > https://lore.kernel.org/dri-devel/20200219102122.1607365-1-daniel.vetter@ffwll.ch/
> 
> I was also going to mention that it would be nice to send the cover
> letter to all recipients from the series, otherwise it's a bit painful.
> Daniel, is this something that could be integrated in your workflow ?

No, the usual result of that if you do it is that mail servers scream at
you for too many recipients.

dri-devel is on lore.kernel.org now, with full historical backlog, so all
there.

https://lore.kernel.org/dri-devel/20200219102122.1607365-1-daniel.vetter@ffwll.ch/T/#t

Cheers, Daniel
Laurent Pinchart Feb. 19, 2020, 1:53 p.m. UTC | #6
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,

Reviewed-by: Laurent Pinchart <laurent.pinchart@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 :-)

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@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;
Daniel Vetter Feb. 19, 2020, 2:29 p.m. UTC | #7
On Wed, Feb 19, 2020 at 2:53 PM Laurent Pinchart
<laurent.pinchart@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@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@intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Cc: linux-renesas-soc@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

Patch
diff mbox series

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;