| Message ID | 1380049187-16036-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Tue, 24 Sep 2013 20:59:47 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The marvell-ccic does several things wrong or ineffectively in the clock > handling and it's usage of the devm_* stuff > > - it assumes clk_get doesn't return NULL > - it explicitly calls devm_clk_put instead just keeping the reference > during it's lifetime and let the driver core call it > - it calls kfree, gpio_free and free_irq for resources it requested > using devm_kzalloc, devm_gpio_request and devm_request_irq > respectively. > - it mixes devm_ and unmanaged resources which probably results in a > race condition during remove OK, all of that stuff was added this time around by Libin; my understanding of that particular hardware is ... minimal. The basic idea of the patch seems sound. I do note, though, that you've changed the behavior of the driver somewhat. The MIPI clock is current obtained at power-up time and released on power-down; you've moved it to probe time instead, and it's held for the lifetime of the driver. Perhaps that's even better, I don't know...Libin, what do you say on that? The free_irq() call is also removed by a patch previously submitted by Wei Yongjun. > This patch fixes all but the last issue in this list. This patch doesn't > introduce new reasons for not building, but there are already several > bulid problems. Care to report those? Thanks, jon
Hi Jonathan & Uwe, In the clk enable and prepare function, we will check the NULL pointer. So it should be no problem. For the mipi_clk, it is shared between other components, so we put the clk it we don't use it. For the free_irq, it's my fault. Out before patch really removed this code together with gpio free .... It missed the last part of the original patch. Regards, Libin >-----Original Message----- >From: Jonathan Corbet [mailto:corbet@lwn.net] >Sent: Wednesday, September 25, 2013 3:15 PM >To: Uwe Kleine-König >Cc: Mauro Carvalho Chehab; linux-media@vger.kernel.org; linux-arm- >kernel@lists.infradead.org; Russell King; kernel@pengutronix.de; Libin Yang >Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit) > >On Tue, 24 Sep 2013 20:59:47 +0200 >Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > >> The marvell-ccic does several things wrong or ineffectively in the >> clock handling and it's usage of the devm_* stuff >> >> - it assumes clk_get doesn't return NULL >> - it explicitly calls devm_clk_put instead just keeping the reference >> during it's lifetime and let the driver core call it >> - it calls kfree, gpio_free and free_irq for resources it requested >> using devm_kzalloc, devm_gpio_request and devm_request_irq >> respectively. >> - it mixes devm_ and unmanaged resources which probably results in a >> race condition during remove > >OK, all of that stuff was added this time around by Libin; my understanding of that particular >hardware is ... minimal. The basic idea of the patch seems sound. I do note, though, that >you've changed the behavior of the driver somewhat. The MIPI clock is current obtained at >power-up time and released on power-down; you've moved it to probe time instead, and it's >held for the lifetime of the driver. >Perhaps that's even better, I don't know...Libin, what do you say on that? > >The free_irq() call is also removed by a patch previously submitted by Wei Yongjun. > >> This patch fixes all but the last issue in this list. This patch >> doesn't introduce new reasons for not building, but there are already >> several bulid problems. > >Care to report those? > >Thanks, > >jon
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.) > For the mipi_clk, it is shared between other components, so we put the clk it we don't use it. There should be no problem if >1 driver holds a reference to the clock. It's clk_disable (+ maybe clk_unprepare) you should call when you don't need it any more. (But even having >1 driver enabling a clk isn't a problem ...) > >> This patch fixes all but the last issue in this list. This patch > >> doesn't introduce new reasons for not building, but there are already > >> several bulid problems. > > > >Care to report those? Sure: CC drivers/media/platform/marvell-ccic/mmp-driver.o drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_calc_dphy': drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct mmp_camera_platform_data' has no member named 'dphy3_algo' drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error: 'DPHY3_ALGO_PXA910' undeclared (first use in this function) drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each undeclared identifier is reported only once for each function it appears in drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk' drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk' drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error: 'DPHY3_ALGO_PXA2128' undeclared (first use in this function) drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk' drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk' drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe': drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_min' drivers/media/platform/marvell-ccic/mmp-driver.c:386:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_src' drivers/media/platform/marvell-ccic/mmp-driver.c:387:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_div' drivers/media/platform/marvell-ccic/mmp-driver.c:388:24: error: 'struct mmp_camera_platform_data' has no member named 'bus_type' drivers/media/platform/marvell-ccic/mmp-driver.c:389:20: error: 'struct mmp_camera_platform_data' has no member named 'dphy' drivers/media/platform/marvell-ccic/mmp-driver.c:391:20: error: 'struct mmp_camera_platform_data' has no member named 'lane' Best regards Uwe
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.
Hi Uwe, Thanks for your reviewing. Please see the comments below. >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. [Libin] Right. NULL could be a valid clk pointer. In the code, the clk will not be released if mipi_clk is NULL. Is below OK? + if (... && !IS_ERR(cam->mipi_clk)) { + devm_clk_put(mcam->dev, cam->mipi_clk); + cam->mipi_clk = NULL; + } Set cam->mipi_clk = NULL is let cam->mipi_clk to be the initial state just like after cam is allocated. >(BTW, calling clk_get_rate on a disabled clk is another thing you shouldn't do.) [Libin] Thanks for pointing it out. We enable the clk in other components. Yes, you are right. We should enable the clk explicitly here. > >> For the mipi_clk, it is shared between other components, so we put the clk it we don't use it. >There should be no problem if >1 driver holds a reference to the clock. It's clk_disable (+ >maybe clk_unprepare) you should call when you don't need it any more. (But even having >1 >driver enabling a clk isn't a problem ...) [Libin] So you mean we need not release the clk here and wait for devm to release it later? I will check it with my colleagues to see whether they are OK with this. > >> >> This patch fixes all but the last issue in this list. This patch >> >> doesn't introduce new reasons for not building, but there are >> >> already several bulid problems. >> > >> >Care to report those? >Sure: > > CC drivers/media/platform/marvell-ccic/mmp-driver.o > drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_calc_dphy': > drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct >mmp_camera_platform_data' has no member named 'dphy3_algo' > drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error: >'DPHY3_ALGO_PXA910' undeclared (first use in this function) > drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each undeclared >identifier is reported only once for each function it appears in > drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error: >'DPHY3_ALGO_PXA2128' undeclared (first use in this function) > drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct >mmp_camera_platform_data' has no member named 'lane_clk' > drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe': > drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_min' > drivers/media/platform/marvell-ccic/mmp-driver.c:386:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_src' > drivers/media/platform/marvell-ccic/mmp-driver.c:387:24: error: 'struct >mmp_camera_platform_data' has no member named 'mclk_div' > drivers/media/platform/marvell-ccic/mmp-driver.c:388:24: error: 'struct >mmp_camera_platform_data' has no member named 'bus_type' > drivers/media/platform/marvell-ccic/mmp-driver.c:389:20: error: 'struct >mmp_camera_platform_data' has no member named 'dphy' > drivers/media/platform/marvell-ccic/mmp-driver.c:391:20: error: 'struct >mmp_camera_platform_data' has no member named 'lane' > >Best regards >Uwe > >-- >Pengutronix e.K. | Uwe Kleine-König | >Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Russell, >-----Original Message----- >From: Russell King - ARM Linux [mailto:linux@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@vger.kernel.org; >linux-arm-kernel@lists.infradead.org; kernel@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
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c index b5a19af..ed16f81e 100644 --- a/drivers/media/platform/marvell-ccic/mmp-driver.c +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c @@ -143,7 +143,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam) struct mmp_camera_platform_data *pdata; if (mcam->bus_type == V4L2_MBUS_CSI2) { - cam->mipi_clk = devm_clk_get(mcam->dev, "mipi"); if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0)) return PTR_ERR(cam->mipi_clk); } @@ -186,12 +185,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam) gpio_set_value(pdata->sensor_power_gpio, 0); gpio_set_value(pdata->sensor_reset_gpio, 0); - if (mcam->bus_type == V4L2_MBUS_CSI2 && !IS_ERR(cam->mipi_clk)) { - if (cam->mipi_clk) - devm_clk_put(mcam->dev, cam->mipi_clk); - cam->mipi_clk = NULL; - } - mcam_clk_disable(mcam); } @@ -325,19 +318,6 @@ static irqreturn_t mmpcam_irq(int irq, void *data) return IRQ_RETVAL(handled); } -static void mcam_deinit_clk(struct mcam_camera *mcam) -{ - unsigned int i; - - for (i = 0; i < NR_MCAM_CLK; i++) { - if (!IS_ERR(mcam->clk[i])) { - if (mcam->clk[i]) - devm_clk_put(mcam->dev, mcam->clk[i]); - } - mcam->clk[i] = NULL; - } -} - static void mcam_init_clk(struct mcam_camera *mcam) { unsigned int i; @@ -371,7 +351,7 @@ static int mmpcam_probe(struct platform_device *pdev) if (cam == NULL) return -ENOMEM; cam->pdev = pdev; - cam->mipi_clk = NULL; + cam->mipi_clk = devm_clk_get(&pdev->dev, "mipi"); INIT_LIST_HEAD(&cam->devlist); mcam = &cam->mcam; @@ -442,6 +422,7 @@ static int mmpcam_probe(struct platform_device *pdev) /* * Power the device up and hand it off to the core. */ + ret = mmpcam_power_up(mcam); if (ret) goto out_deinit_clk; @@ -470,7 +451,6 @@ out_unregister: out_power_down: mmpcam_power_down(mcam); out_deinit_clk: - mcam_deinit_clk(mcam); return ret; } @@ -481,16 +461,11 @@ static int mmpcam_remove(struct mmp_camera *cam) struct mmp_camera_platform_data *pdata; mmpcam_remove_device(cam); - free_irq(cam->irq, mcam); mccic_shutdown(mcam); mmpcam_power_down(mcam); pdata = cam->pdev->dev.platform_data; - gpio_free(pdata->sensor_reset_gpio); - gpio_free(pdata->sensor_power_gpio); - mcam_deinit_clk(mcam); iounmap(cam->power_regs); iounmap(mcam->regs); - kfree(cam); return 0; }
The marvell-ccic does several things wrong or ineffectively in the clock handling and it's usage of the devm_* stuff - it assumes clk_get doesn't return NULL - it explicitly calls devm_clk_put instead just keeping the reference during it's lifetime and let the driver core call it - it calls kfree, gpio_free and free_irq for resources it requested using devm_kzalloc, devm_gpio_request and devm_request_irq respectively. - it mixes devm_ and unmanaged resources which probably results in a race condition during remove This patch fixes all but the last issue in this list. This patch doesn't introduce new reasons for not building, but there are already several bulid problems. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Cc: Libin Yang <lbyang@marvell.com> Changes since (implicit) v1: - really fix the third issue in the list. - drop whitespace noise hunk --- drivers/media/platform/marvell-ccic/mmp-driver.c | 29 ++---------------------- 1 file changed, 2 insertions(+), 27 deletions(-)