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

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

[Daniel Vetter]

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(-)

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;
-- 
2.24.1

[PATCH 40/52] drm/shmob: Drop explicit drm_mode_config_cleanup call

[Daniel Vetter]

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/shmobile/shmob_drm_drv.c | 2 --
 drivers/gpu/drm/shmobile/shmob_drm_kms.c | 6 +++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index b8c0930959c7..ae9d6b8d3ca8 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -192,7 +192,6 @@ static int shmob_drm_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(ddev);
 	drm_kms_helper_poll_fini(ddev);
-	drm_mode_config_cleanup(ddev);
 	drm_irq_uninstall(ddev);
 	drm_dev_put(ddev);
 
@@ -288,7 +287,6 @@ static int shmob_drm_probe(struct platform_device *pdev)
 	drm_irq_uninstall(ddev);
 err_modeset_cleanup:
 	drm_kms_helper_poll_fini(ddev);
-	drm_mode_config_cleanup(ddev);
 err_free_drm_dev:
 	drm_dev_put(ddev);
 
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_kms.c b/drivers/gpu/drm/shmobile/shmob_drm_kms.c
index c51197b6fd85..e6e34bb75ba0 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_kms.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_kms.c
@@ -126,7 +126,11 @@ static const struct drm_mode_config_funcs shmob_drm_mode_config_funcs = {
 
 int shmob_drm_modeset_init(struct shmob_drm_device *sdev)
 {
-	drm_mode_config_init(sdev->ddev);
+	int ret;
+
+	ret = drm_mode_config_init(sdev->ddev);
+	if (ret)
+		return ret;
 
 	shmob_drm_crtc_create(sdev);
 	shmob_drm_encoder_create(sdev);
-- 
2.24.1

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

[Geert Uytterhoeven]

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

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

[Daniel Vetter]

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



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

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

[Geert Uytterhoeven]

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

-- 
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

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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 40/52] drm/shmob: Drop explicit drm_mode_config_cleanup call

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Wed, Feb 19, 2020 at 11:21:10AM +0100, 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 ...

With the same comments as the one for the rcar-du patch taken into
account,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 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/shmobile/shmob_drm_drv.c | 2 --
>  drivers/gpu/drm/shmobile/shmob_drm_kms.c | 6 +++++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index b8c0930959c7..ae9d6b8d3ca8 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -192,7 +192,6 @@ static int shmob_drm_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(ddev);
>  	drm_kms_helper_poll_fini(ddev);
> -	drm_mode_config_cleanup(ddev);
>  	drm_irq_uninstall(ddev);
>  	drm_dev_put(ddev);
>  
> @@ -288,7 +287,6 @@ static int shmob_drm_probe(struct platform_device *pdev)
>  	drm_irq_uninstall(ddev);
>  err_modeset_cleanup:
>  	drm_kms_helper_poll_fini(ddev);
> -	drm_mode_config_cleanup(ddev);
>  err_free_drm_dev:
>  	drm_dev_put(ddev);
>  
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_kms.c b/drivers/gpu/drm/shmobile/shmob_drm_kms.c
> index c51197b6fd85..e6e34bb75ba0 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_kms.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_kms.c
> @@ -126,7 +126,11 @@ static const struct drm_mode_config_funcs shmob_drm_mode_config_funcs = {
>  
>  int shmob_drm_modeset_init(struct shmob_drm_device *sdev)
>  {
> -	drm_mode_config_init(sdev->ddev);
> +	int ret;
> +
> +	ret = drm_mode_config_init(sdev->ddev);
> +	if (ret)
> +		return ret;
>  
>  	shmob_drm_crtc_create(sdev);
>  	shmob_drm_encoder_create(sdev);

-- 
Regards,

Laurent Pinchart

Re: [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@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



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