Hi Russell, >-----Original Message----- >From: Russell King - ARM Linux [mailto:linux@xxxxxxxxxxxxxxxx] >Sent: Thursday, September 26, 2013 4:24 PM >To: Uwe Kleine-König >Cc: Libin Yang; Jonathan Corbet; Mauro Carvalho Chehab; linux-media@xxxxxxxxxxxxxxx; >linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx >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 -- 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
