[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Libin Yang]

Hi Russell,


>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>Sent: Thursday, September 26, 2013 4:24 PM
>To: Uwe Kleine-König
>Cc: Libin Yang; Jonathan Corbet; Mauro Carvalho Chehab; linux-media at vger.kernel.org;
>linux-arm-kernel at lists.infradead.org; kernel at pengutronix.de
>Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
>
>On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-König wrote:
>> Hi Libin,
>>
>> On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
>> > In the clk enable and prepare function, we will check the NULL
>> > pointer. So it should be no problem.
>> I'm not sure what you mean here and unfortunately your quoting style
>> makes your statement appear without context.
>>
>> 	if (... && !IS_ERR(cam->mipi_clk)) {
>> 		if (cam->mipi_clk)
>> 			devm_clk_put(mcam->dev, cam->mipi_clk);
>> 		cam->mipi_clk = NULL;
>> 	}
>>
>> might work in your setup, but it's wrong usage of the clk API. There
>> is no reason NULL couldn't be a valid clk pointer. Moreover I cannot
>> find a place in that driver that calls prepare and/or enable for the mipi_clk.
>> (BTW, calling clk_get_rate on a disabled clk is another thing you
>> shouldn't do.)
>
>It's a bug for another reason.  Consider this:
>
>	clk = devm_clk_get(...);
>
>Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL then the devm
>API will allocate a tracking structure for the "allocated"
>clock.  If you then do:
>
>	if (!IS_ERR(clk)) {
>		if (clk)
>			devm_clk_put(clk);
>		clk = NULL;
>	}
>
>Then this structure won't get freed.  Next time you call devm_clk_get(), you'll allocate another
>tracking structure.  If the driver does this a lot, it will spawn lots of these tracking structures
>which will only get cleaned up when the device is unbound (possibly never.)
>
>So, what this driver is doing with its NULL checks against clocks is buggy, no two ways
>about it. 

[Libin] Yes, you are right. it will not release the clk tracking structure if it is NULL and may allocate again later. It is a bug.

Regards,
Libin