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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
