| Message ID | 20200219102122.1607365-38-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | None | expand |
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
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
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
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 ?
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
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;
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
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;
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(-)