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