* Re: [PATCH v6 0/5] clk: clock deregistration support [not found] ` <3160771.O1gFkR91vK@avalon> @ 2013-10-15 20:04 ` Sylwester Nawrocki 2013-10-28 19:54 ` Mike Turquette 2013-10-28 20:26 ` Laurent Pinchart 0 siblings, 2 replies; 4+ messages in thread From: Sylwester Nawrocki @ 2013-10-15 20:04 UTC (permalink / raw) To: Laurent Pinchart Cc: linux, mturquette, Sylwester Nawrocki, linux-arm-kernel, jiada_wang, kyungmin.park, myungjoo.ham, t.figa, g.liakhovetski, linux-kernel, linux-mips, linux-sh, LMML Hi, (adding linux-media mailing list at Cc) On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: [...] >> The only issue I found might be at the omap3isp driver, which provides >> clock to its sub-drivers and takes reference on the sub-driver modules. >> When sub-driver calls clk_get() all modules would get locked in memory, >> due to circular reference. One solution to that could be to pass NULL >> struct device pointer, as in the below patch. > > Doesn't that introduce race conditions ? If the sub-drivers require the clock, > they want to be sure that the clock won't disappear beyond their backs. I > agree that the circular dependency needs to be solved somehow, but we probably > need a more generic solution. The problem will become more widespread in the > future with DT-based device instantiation in both V4L2 and KMS. I'm wondering whether subsystems and drivers itself should be written so they deal with such dependencies they are aware of. There is similar situation in the regulator API, regulator_get() simply takes a reference on a module providing the regulator object. Before a "more generic solution" is available, what do you think about keeping obtaining a reference on a clock provider module in clk_get() and doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(), clk_put() in sub-driver whenever a clock is actively used, to avoid permanent circular reference ? -- Thanks, Sylwester >> ---------8<------------------ >> From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001 >> From: Sylwester Nawrocki<s.nawrocki@samsung.com> >> Date: Thu, 19 Sep 2013 23:52:04 +0200 >> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register() >> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com> >> --- >> drivers/media/platform/omap3isp/isp.c | 15 ++++++++++----- >> drivers/media/platform/omap3isp/isp.h | 1 + >> 2 files changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/platform/omap3isp/isp.c >> b/drivers/media/platform/omap3isp/isp.c >> index df3a0ec..d7f3c98 100644 >> --- a/drivers/media/platform/omap3isp/isp.c >> +++ b/drivers/media/platform/omap3isp/isp.c >> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp) >> struct clk_init_data init; >> unsigned int i; >> >> + for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) >> + isp->xclks[i] = ERR_PTR(-EINVAL); >> + >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { >> struct isp_xclk *xclk =&isp->xclks[i]; >> - struct clk *clk; >> >> xclk->isp = isp; >> xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B; >> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp) >> >> xclk->hw.init =&init; >> >> - clk = devm_clk_register(isp->dev,&xclk->hw); >> - if (IS_ERR(clk)) >> - return PTR_ERR(clk); >> + xclk->clk = clk_register(NULL,&xclk->hw); >> + if (IS_ERR(xclk->clk)) >> + return PTR_ERR(xclk->clk); >> >> if (pdata->xclks[i].con_id == NULL&& >> pdata->xclks[i].dev_id == NULL) >> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp) >> >> xclk->lookup->con_id = pdata->xclks[i].con_id; >> xclk->lookup->dev_id = pdata->xclks[i].dev_id; >> - xclk->lookup->clk = clk; >> + xclk->lookup->clk = xclk->clk; >> >> clkdev_add(xclk->lookup); >> } >> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp) >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { >> struct isp_xclk *xclk =&isp->xclks[i]; >> >> + if (!IS_ERR(xclk->clk)) >> + clk_unregister(xclk->clk); >> + >> if (xclk->lookup) >> clkdev_drop(xclk->lookup); >> } >> diff --git a/drivers/media/platform/omap3isp/isp.h >> b/drivers/media/platform/omap3isp/isp.h >> index cd3eff4..1498f2b 100644 >> --- a/drivers/media/platform/omap3isp/isp.h >> +++ b/drivers/media/platform/omap3isp/isp.h >> @@ -135,6 +135,7 @@ struct isp_xclk { >> struct isp_device *isp; >> struct clk_hw hw; >> struct clk_lookup *lookup; >> + struct clk *clk; >> enum isp_xclk_id id; >> >> spinlock_t lock; /* Protects enabled and divider */ >> ---------8<------------------ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6 0/5] clk: clock deregistration support 2013-10-15 20:04 ` [PATCH v6 0/5] clk: clock deregistration support Sylwester Nawrocki @ 2013-10-28 19:54 ` Mike Turquette 2013-10-28 21:06 ` Laurent Pinchart 2013-10-28 20:26 ` Laurent Pinchart 1 sibling, 1 reply; 4+ messages in thread From: Mike Turquette @ 2013-10-28 19:54 UTC (permalink / raw) To: Sylwester Nawrocki, Laurent Pinchart Cc: linux, Sylwester Nawrocki, linux-arm-kernel, jiada_wang, kyungmin.park, myungjoo.ham, t.figa, g.liakhovetski, linux-kernel, linux-mips, linux-sh, LMML Quoting Sylwester Nawrocki (2013-10-15 13:04:17) > Hi, > > (adding linux-media mailing list at Cc) > > On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: > [...] > >> The only issue I found might be at the omap3isp driver, which provides > >> clock to its sub-drivers and takes reference on the sub-driver modules. > >> When sub-driver calls clk_get() all modules would get locked in memory, > >> due to circular reference. One solution to that could be to pass NULL > >> struct device pointer, as in the below patch. > > > > Doesn't that introduce race conditions ? If the sub-drivers require the clock, > > they want to be sure that the clock won't disappear beyond their backs. I > > agree that the circular dependency needs to be solved somehow, but we probably > > need a more generic solution. The problem will become more widespread in the > > future with DT-based device instantiation in both V4L2 and KMS. > > I'm wondering whether subsystems and drivers itself should be written so > they deal with such dependencies they are aware of. > > There is similar situation in the regulator API, regulator_get() simply > takes a reference on a module providing the regulator object. > > Before a "more generic solution" is available, what do you think about > keeping obtaining a reference on a clock provider module in clk_get() and > doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(), > clk_put() in sub-driver whenever a clock is actively used, to avoid > permanent circular reference ? Laurent, Did you have any feedback on this proposal? I would like to merge these patches so that folks with clock driver modules can use them properly. We can fix up things in the core code as we figure them out. Regards, Mike > > -- > Thanks, > Sylwester > > >> ---------8<------------------ > >> From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 2001 > >> From: Sylwester Nawrocki<s.nawrocki@samsung.com> > >> Date: Thu, 19 Sep 2013 23:52:04 +0200 > >> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register() > >> > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com> > >> --- > >> drivers/media/platform/omap3isp/isp.c | 15 ++++++++++----- > >> drivers/media/platform/omap3isp/isp.h | 1 + > >> 2 files changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/media/platform/omap3isp/isp.c > >> b/drivers/media/platform/omap3isp/isp.c > >> index df3a0ec..d7f3c98 100644 > >> --- a/drivers/media/platform/omap3isp/isp.c > >> +++ b/drivers/media/platform/omap3isp/isp.c > >> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp) > >> struct clk_init_data init; > >> unsigned int i; > >> > >> + for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) > >> + isp->xclks[i] = ERR_PTR(-EINVAL); > >> + > >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { > >> struct isp_xclk *xclk =&isp->xclks[i]; > >> - struct clk *clk; > >> > >> xclk->isp = isp; > >> xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B; > >> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp) > >> > >> xclk->hw.init =&init; > >> > >> - clk = devm_clk_register(isp->dev,&xclk->hw); > >> - if (IS_ERR(clk)) > >> - return PTR_ERR(clk); > >> + xclk->clk = clk_register(NULL,&xclk->hw); > >> + if (IS_ERR(xclk->clk)) > >> + return PTR_ERR(xclk->clk); > >> > >> if (pdata->xclks[i].con_id == NULL&& > >> pdata->xclks[i].dev_id == NULL) > >> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp) > >> > >> xclk->lookup->con_id = pdata->xclks[i].con_id; > >> xclk->lookup->dev_id = pdata->xclks[i].dev_id; > >> - xclk->lookup->clk = clk; > >> + xclk->lookup->clk = xclk->clk; > >> > >> clkdev_add(xclk->lookup); > >> } > >> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device *isp) > >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { > >> struct isp_xclk *xclk =&isp->xclks[i]; > >> > >> + if (!IS_ERR(xclk->clk)) > >> + clk_unregister(xclk->clk); > >> + > >> if (xclk->lookup) > >> clkdev_drop(xclk->lookup); > >> } > >> diff --git a/drivers/media/platform/omap3isp/isp.h > >> b/drivers/media/platform/omap3isp/isp.h > >> index cd3eff4..1498f2b 100644 > >> --- a/drivers/media/platform/omap3isp/isp.h > >> +++ b/drivers/media/platform/omap3isp/isp.h > >> @@ -135,6 +135,7 @@ struct isp_xclk { > >> struct isp_device *isp; > >> struct clk_hw hw; > >> struct clk_lookup *lookup; > >> + struct clk *clk; > >> enum isp_xclk_id id; > >> > >> spinlock_t lock; /* Protects enabled and divider */ > >> ---------8<------------------ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6 0/5] clk: clock deregistration support 2013-10-28 19:54 ` Mike Turquette @ 2013-10-28 21:06 ` Laurent Pinchart 0 siblings, 0 replies; 4+ messages in thread From: Laurent Pinchart @ 2013-10-28 21:06 UTC (permalink / raw) To: Mike Turquette Cc: Sylwester Nawrocki, linux, Sylwester Nawrocki, linux-arm-kernel, jiada_wang, kyungmin.park, myungjoo.ham, t.figa, g.liakhovetski, linux-kernel, linux-mips, linux-sh, LMML On Monday 28 October 2013 12:54:01 Mike Turquette wrote: > Quoting Sylwester Nawrocki (2013-10-15 13:04:17) > > > Hi, > > > > (adding linux-media mailing list at Cc) > > > > On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > > > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: > > [...] > > > > >> The only issue I found might be at the omap3isp driver, which provides > > >> clock to its sub-drivers and takes reference on the sub-driver modules. > > >> When sub-driver calls clk_get() all modules would get locked in memory, > > >> due to circular reference. One solution to that could be to pass NULL > > >> struct device pointer, as in the below patch. > > > > > > Doesn't that introduce race conditions ? If the sub-drivers require the > > > clock, they want to be sure that the clock won't disappear beyond their > > > backs. I agree that the circular dependency needs to be solved somehow, > > > but we probably need a more generic solution. The problem will become > > > more widespread in the future with DT-based device instantiation in > > > both V4L2 and KMS. > > > > I'm wondering whether subsystems and drivers itself should be written so > > they deal with such dependencies they are aware of. > > > > There is similar situation in the regulator API, regulator_get() simply > > takes a reference on a module providing the regulator object. > > > > Before a "more generic solution" is available, what do you think about > > keeping obtaining a reference on a clock provider module in clk_get() and > > doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(), > > clk_put() in sub-driver whenever a clock is actively used, to avoid > > permanent circular reference ? > > Laurent, > > Did you have any feedback on this proposal? I would like to merge these > patches so that folks with clock driver modules can use them properly. > We can fix up things in the core code as we figure them out. I've just replied to Sylwester's original patch. I'm basically OK with the idea as a temporary solution. > > >> ---------8<------------------ > > >> > > >> From ca5963041aad67e31324cb5d4d5e2cfce1706d4f Mon Sep 17 00:00:00 > > >> 2001 > > >> > > >> From: Sylwester Nawrocki<s.nawrocki@samsung.com> > > >> Date: Thu, 19 Sep 2013 23:52:04 +0200 > > >> Subject: [PATCH] omap3isp: Pass NULL device pointer to clk_register() > > >> > > >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com> > > >> --- > > >> > > >> drivers/media/platform/omap3isp/isp.c | 15 ++++++++++----- > > >> drivers/media/platform/omap3isp/isp.h | 1 + > > >> 2 files changed, 11 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/drivers/media/platform/omap3isp/isp.c > > >> b/drivers/media/platform/omap3isp/isp.c > > >> index df3a0ec..d7f3c98 100644 > > >> --- a/drivers/media/platform/omap3isp/isp.c > > >> +++ b/drivers/media/platform/omap3isp/isp.c > > >> @@ -290,9 +290,11 @@ static int isp_xclk_init(struct isp_device *isp) > > >> > > >> struct clk_init_data init; > > >> unsigned int i; > > >> > > >> + for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) > > >> + isp->xclks[i] = ERR_PTR(-EINVAL); > > >> + > > >> > > >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { > > >> > > >> struct isp_xclk *xclk =&isp->xclks[i]; > > >> > > >> - struct clk *clk; > > >> > > >> xclk->isp = isp; > > >> xclk->id = i == 0 ? ISP_XCLK_A : ISP_XCLK_B; > > >> > > >> @@ -306,9 +308,9 @@ static int isp_xclk_init(struct isp_device *isp) > > >> > > >> xclk->hw.init =&init; > > >> > > >> - clk = devm_clk_register(isp->dev,&xclk->hw); > > >> - if (IS_ERR(clk)) > > >> - return PTR_ERR(clk); > > >> + xclk->clk = clk_register(NULL,&xclk->hw); > > >> + if (IS_ERR(xclk->clk)) > > >> + return PTR_ERR(xclk->clk); > > >> > > >> if (pdata->xclks[i].con_id == NULL&& > > >> pdata->xclks[i].dev_id == NULL) > > >> > > >> @@ -320,7 +322,7 @@ static int isp_xclk_init(struct isp_device *isp) > > >> > > >> xclk->lookup->con_id = pdata->xclks[i].con_id; > > >> xclk->lookup->dev_id = pdata->xclks[i].dev_id; > > >> > > >> - xclk->lookup->clk = clk; > > >> + xclk->lookup->clk = xclk->clk; > > >> > > >> clkdev_add(xclk->lookup); > > >> > > >> } > > >> > > >> @@ -335,6 +337,9 @@ static void isp_xclk_cleanup(struct isp_device > > >> *isp) > > >> > > >> for (i = 0; i< ARRAY_SIZE(isp->xclks); ++i) { > > >> > > >> struct isp_xclk *xclk =&isp->xclks[i]; > > >> > > >> + if (!IS_ERR(xclk->clk)) > > >> + clk_unregister(xclk->clk); > > >> + > > >> > > >> if (xclk->lookup) > > >> > > >> clkdev_drop(xclk->lookup); > > >> > > >> } > > >> > > >> diff --git a/drivers/media/platform/omap3isp/isp.h > > >> b/drivers/media/platform/omap3isp/isp.h > > >> index cd3eff4..1498f2b 100644 > > >> --- a/drivers/media/platform/omap3isp/isp.h > > >> +++ b/drivers/media/platform/omap3isp/isp.h > > >> @@ -135,6 +135,7 @@ struct isp_xclk { > > >> > > >> struct isp_device *isp; > > >> struct clk_hw hw; > > >> struct clk_lookup *lookup; > > >> > > >> + struct clk *clk; > > >> > > >> enum isp_xclk_id id; > > >> > > >> spinlock_t lock; /* Protects enabled and divider */ > > >> > > >> ---------8<------------------ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v6 0/5] clk: clock deregistration support 2013-10-15 20:04 ` [PATCH v6 0/5] clk: clock deregistration support Sylwester Nawrocki 2013-10-28 19:54 ` Mike Turquette @ 2013-10-28 20:26 ` Laurent Pinchart 1 sibling, 0 replies; 4+ messages in thread From: Laurent Pinchart @ 2013-10-28 20:26 UTC (permalink / raw) To: Sylwester Nawrocki Cc: linux, mturquette, Sylwester Nawrocki, linux-arm-kernel, jiada_wang, kyungmin.park, myungjoo.ham, t.figa, g.liakhovetski, linux-kernel, linux-mips, linux-sh, LMML Hi Sylwester, On Tuesday 15 October 2013 22:04:17 Sylwester Nawrocki wrote: > Hi, > > (adding linux-media mailing list at Cc) > > On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: > [...] > > >> The only issue I found might be at the omap3isp driver, which provides > >> clock to its sub-drivers and takes reference on the sub-driver modules. > >> When sub-driver calls clk_get() all modules would get locked in memory, > >> due to circular reference. One solution to that could be to pass NULL > >> struct device pointer, as in the below patch. > > > > Doesn't that introduce race conditions ? If the sub-drivers require the > > clock, they want to be sure that the clock won't disappear beyond their > > backs. I agree that the circular dependency needs to be solved somehow, > > but we probably need a more generic solution. The problem will become > > more widespread in the future with DT-based device instantiation in both > > V4L2 and KMS. > > I'm wondering whether subsystems and drivers itself should be written so > they deal with such dependencies they are aware of. > > There is similar situation in the regulator API, regulator_get() simply > takes a reference on a module providing the regulator object. > > Before a "more generic solution" is available, what do you think about > keeping obtaining a reference on a clock provider module in clk_get() and > doing clk_get(), clk_prepare_enable(), ..., clk_unprepare_disable(), > clk_put() in sub-driver whenever a clock is actively used, to avoid > permanent circular reference ? That's a workaround I can live with in the short term, as long as we work on a generic solution to this problem. It will bite us back in the not too distant future if we pretend to forget about it. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-10-28 21:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1377874402-2944-1-git-send-email-s.nawrocki@samsung.com>
[not found] ` <52420664.2040604@gmail.com>
[not found] ` <3160771.O1gFkR91vK@avalon>
2013-10-15 20:04 ` [PATCH v6 0/5] clk: clock deregistration support Sylwester Nawrocki
2013-10-28 19:54 ` Mike Turquette
2013-10-28 21:06 ` Laurent Pinchart
2013-10-28 20:26 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox