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:

quoted

Hi Libin,

On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:

quoted

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