On Wed, Feb 19, 2020 at 2:53 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> > > 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@xxxxxxxxx> > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > Cc: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> > > Cc: linux-renesas-soc@xxxxxxxxxxxxxxx > > --- > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
