linux-kernel - Re: [PATCH v2 5/7] drm/stm: ltdc: add devm_reset_control & platform_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Jul 2017 14:38:38 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Philippe CORNU <philippe.cornu@...com>
Cc:     Yannick Fertre <yannick.fertre@...com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Vincent Abriou <vincent.abriou@...com>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Fabien Dessenne <fabien.dessenne@...com>,
        Mickael Reulier <mickael.reulier@...com>,
        Gabriel Fernandez <gabriel.fernandez@...com>,
        Ludovic Barre <ludovic.barre@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>
Subject: Re: [PATCH v2 5/7] drm/stm: ltdc: add devm_reset_control &
 platform_get_ressource

Hi Philippe,

On Thu, 2017-07-20 at 14:05 +0200, Philippe CORNU wrote:
> Use devm_reset_control_get_exclusive to avoid resource leakage (based
> on patch "Convert drivers to explicit reset API" from Philipp Zabel).
> 
> Also use platform_get_resource, which is more usual and
> consistent with platform_get_irq called later.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@...com>
> Signed-off-by: Philippe CORNU <philippe.cornu@...com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
> Cc: Philipp Zabel <p.zabel@...gutronix.de>

Looking at the usage below, this driver only seems to care about the
reset deassertion before register use, so this could use the shared API.
Further, it seems that this reset is optional.

> ---
>  drivers/gpu/drm/stm/ltdc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 92e58ba..d826045 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -874,7 +874,7 @@ int ltdc_load(struct drm_device *ddev)
>  	struct drm_panel *panel;
>  	struct drm_crtc *crtc;
>  	struct reset_control *rstc;
> -	struct resource res;
> +	struct resource *res;
>  	int irq, ret, i;
>  
>  	DRM_DEBUG_DRIVER("\n");
> @@ -883,7 +883,7 @@ int ltdc_load(struct drm_device *ddev)
>  	if (ret)
>  		return ret;
>  
> -	rstc = of_reset_control_get(np, NULL);
> +	rstc = devm_reset_control_get_exclusive(dev, NULL);

I would suggest to change this to

-	rstc = of_reset_control_get(np, NULL);
+	rstc = devm_reset_control_get_optional_shared(dev, NULL);
+	if (IS_ERR(rstc))
+		return PTR_ERR(rstc);

>  	mutex_init(&ldev->err_lock);
>  
> @@ -898,13 +898,14 @@ int ltdc_load(struct drm_device *ddev)
>  		return -ENODEV;
>  	}
>  
> -	if (of_address_to_resource(np, 0, &res)) {
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
>  		DRM_ERROR("Unable to get resource\n");
>  		ret = -ENODEV;
>  		goto err;
>  	}
>  
> -	ldev->regs = devm_ioremap_resource(dev, &res);
> +	ldev->regs = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(ldev->regs)) {
>  		DRM_ERROR("Unable to get ltdc registers\n");
>  		ret = PTR_ERR(ldev->regs);

then below you can change:

-	if (!IS_ERR(rstc))
-		reset_control_deassert(rstc);
+	reset_control_deassert(rstc);

regards
Philipp