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:

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.