linux-kernel - Re: [PATCH 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk

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]

Message-ID: <Pine.LNX.4.64.1112061045150.10715@axis700.grange>
Date:	Tue, 6 Dec 2011 10:49:12 +0100 (CET)
From:	Guennadi Liakhovetski <g.liakhovetski@....de>
To:	Josh Wu <josh.wu@...el.com>
cc:	linux-media@...r.kernel.org, nicolas.ferre@...el.com,
	linux@....linux.org.uk, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare()
 functions

Hi Josh

Thanks for the patch, but I'll ask you to fix the same thing in it, that 
I've fixed for you in the first patch in this series:

On Wed, 30 Nov 2011, Josh Wu wrote:

> Signed-off-by: Josh Wu <josh.wu@...el.com>
> ---
>  drivers/media/video/atmel-isi.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> index ea4eef4..5da4381 100644
> --- a/drivers/media/video/atmel-isi.c
> +++ b/drivers/media/video/atmel-isi.c

[snip]

> @@ -978,10 +986,14 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
>  		goto err_clk_get;
>  	}
>  
> +	ret = clk_prepare(isi->mck);
> +	if (ret)
> +		goto err_set_mck_rate;
> +
>  	/* Set ISI_MCK's frequency, it should be faster than pixel clock */
>  	ret = clk_set_rate(isi->mck, pdata->mck_hz);
>  	if (ret < 0)
> -		goto err_set_mck_rate;
> +		goto err_unprepare_mck;
>  
>  	isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>  				sizeof(struct fbd) * MAX_BUFFER_NUM,
> @@ -1058,11 +1070,14 @@ err_alloc_ctx:
>  			isi->p_fb_descriptors,
>  			isi->fb_descriptors_phys);
>  err_alloc_descriptors:
> +err_unprepare_mck:
> +	clk_unprepare(isi->mck);
>  err_set_mck_rate:
>  	clk_put(isi->mck);
>  err_clk_get:
>  	kfree(isi);
>  err_alloc_isi:
> +	clk_unprepare(pclk);
>  	clk_put(pclk);
>  
>  	return ret;

Please, use error label names consistently. As you can see, currently the 
driver uses the convention

	ret = do_something();
	if (ret < 0)
		goto err_do_something;

i.e., the label is called after the operation, that has failed, not after 
the clean up step, that the control now has to jump to. Please, update 
your patch to also use this convention.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/