| Message ID | 52420664.2040604@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Sylwester, On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: > On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote: > > This patch series implements clock deregistration in the common clock > > framework. Comparing to v5 it only includes further corrections of NULL > > clock handling. > > [...] > > > clk: Provide not locked variant of of_clk_get_from_provider() > > clkdev: Fix race condition in clock lookup from device tree > > clk: Add common __clk_get(), __clk_put() implementations > > clk: Assign module owner of a clock being registered > > clk: Implement clk_unregister > > Hi Mike, Russell, > > Would you have any further comments/suggestions on this series ? > > I have inspected all callers of clk_register() and all should be fine > with regards to dereferencing dev->driver. The first argument to this > function is either NULL or clk_register() is being called in drivers' > probe() callback, which ensures dev->driver won't change due to holding > dev->mutex. > > 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. > ---------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<------------------ > > Alternatively an additional argument could be added to the clk_register*() > functions. Something like: > > ---------8<------------------ > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 27f8c42..ef934ac 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev, > struct clk_hw *hw) > > } > EXPORT_SYMBOL_GPL(__clk_register); > > -static int _clk_register(struct device *dev, struct clk_hw *hw, struct > clk *clk) > +static int _clk_register(struct device *dev, struct clk_hw *hw, struct > clk *clk, > + struct module *owner) > { > int i, ret; > > @@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev, > struct clk_hw *hw, struct clk *clk) > } > } > > + clk->owner = owner; > + > ret = __clk_init(dev, clk); > if (!ret) > return 0; > @@ -1892,7 +1895,7 @@ fail_name: > } > > /** > - * clk_register - allocate a new clock, register it and return an > opaque cookie > + * ___clk_register - allocate a new clock, register it and return an > opaque cookie > * @dev: device that is registering this clock > * @hw: link to hardware-specific clock data > * > @@ -1902,7 +1905,8 @@ fail_name: > * rest of the clock API. In the event of an error clk_register will > * return an error code; drivers must test for an error code after calling > * clk_register. > */ > > -struct clk *clk_register(struct device *dev, struct clk_hw *hw) > +struct clk *___clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner) > { > int ret; > struct clk *clk; > @@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev, > struct clk_hw *hw) > goto fail_out; > } > - ret = _clk_register(dev, hw, clk); > + ret = _clk_register(dev, hw, clk, owner); > if (!ret) > return clk; > > @@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev, > struct clk_hw *hw) > fail_out: > return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(clk_register); > +EXPORT_SYMBOL_GPL(___clk_register); > > /* > * Free memory allocated for a clock. > @@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev, > void *res) > * automatically clk_unregister()ed on driver detach. See > * clk_register() for more information. > */ > -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) > +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner) > { > struct clk *clk; > int ret; > @@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev, > struct clk_hw *hw) > if (!clk) > return ERR_PTR(-ENOMEM); > > - ret = _clk_register(dev, hw, clk); > + ret = _clk_register(dev, hw, clk, owner); > if (!ret) { > devres_add(dev, clk); > } else { > @@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev, > struct clk_hw *hw) > return clk; > } > -EXPORT_SYMBOL_GPL(devm_clk_register); > +EXPORT_SYMBOL_GPL(__devm_clk_register); > > static int devm_clk_match(struct device *dev, void *res, void *data) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 13623f3..c656ebf 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -31,6 +31,7 @@ > #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate > change */ > > struct clk_hw; > +struct module; > > /** > * struct clk_ops - Callback operations for hardware clocks; these are to > @@ -436,8 +437,21 @@ struct clk *clk_register_composite(struct device > *dev, const char *name, > * rest of the clock API. In the event of an error clk_register will > * return an error code; drivers must test for an error code after calling > * clk_register. > */ > -struct clk *clk_register(struct device *dev, struct clk_hw *hw); > -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw); > +struct clk *___clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner); > + > +static inline struct clk *clk_register(struct device *dev, struct > clk_hw *hw) > +{ > + return ___clk_register(dev, hw, THIS_MODULE); > +} > + > +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner); > +static inline struct clk *devm_clk_register(struct device *dev, > + struct clk_hw *hw) > +{ > + return __devm_clk_register(dev, hw, THIS_MODULE); > +} > > void clk_unregister(struct clk *clk); > void devm_clk_unregister(struct device *dev, struct clk *clk); > ---------8<------------------ > > Similarly it would need to be done for the remaining clk_register*() > functions, which have much longer arguments list. > > It's a bit messy, since there is already __clk_register() function with > double underscore prefix. Perhaps that could be renamed to something else so > all the functions taking struct module * parameter are prefixed with double > underscore. > > I would squash patches 3/5 and 4/5 in the next iteration, if it is decided > to keep using dev->driver->owner.
Hi Laurent, On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > 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. It doesn't introduce any new race conditions AFAICT. I doubt all these issues can be resolved in one single step. Currently the modular clock providers are seriously broken, there is no reference tracking and the clock consumers can easily get into a state where they have invalid references to clocks supplied by already unregistered drivers. With this patch series the clock consumer drivers will not crash thanks to the clock object reference counting. So the worst thing may happen is a clock left in an unexpected state. However there should be no problems with the v4l2-async API, the host driver in its de-initialization routine unregisters its sub-drivers (they should stop using the clock when notified of such an event), only then the host would unregister the clock (subsequently the sub-drivers get re-attached and put into deferred probing state). There may be issues when a sub-driver's file handle is opened while the host is about to de-initialize. But I doubt resolution of such problems belongs to the common clock framework. I have been trying to improve the situation in small steps, rather than waiting forever for a perfect solution. Do you perhaps have any ideas WRT to a "more generic solution" ? In general I have been trying to avoid using v4l2-clk and add what's missing in the common clock framework. Perhaps we should leave only the kref part in the __clk_get(), __clk_put() hooks and be taking reference to a clock in clk_prepare() and releasing it in clk_unprepare() ? This way circular reference would exist only between clk_prepare(), clk_unprepare() calls. Assuming a driver prepares clock in device's open() and unprepares in device close() handler perhaps it could all work better, without relying on the host to ensure the resource reference tracking. I'm not sure if that is not making too many assumptions for a generic API. Thanks for the feedback. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Sylwester Nawrocki (2013-09-24 14:38:44) > On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote: > > This patch series implements clock deregistration in the common clock > > framework. Comparing to v5 it only includes further corrections of NULL > > clock handling. > [...] > > clk: Provide not locked variant of of_clk_get_from_provider() > > clkdev: Fix race condition in clock lookup from device tree > > clk: Add common __clk_get(), __clk_put() implementations > > clk: Assign module owner of a clock being registered > > clk: Implement clk_unregister > > Hi Mike, Russell, > > Would you have any further comments/suggestions on this series ? I don't have any further comments. The changes look good to me and push things in the right direction. I'll wait for Laurent's feedback on the omap3isp driver implications though. Regards, Mike > > I have inspected all callers of clk_register() and all should be fine > with regards to dereferencing dev->driver. The first argument to this > function is either NULL or clk_register() is being called in drivers' > probe() callback, which ensures dev->driver won't change due to holding > dev->mutex. > > 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. > > ---------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<------------------ > > > Alternatively an additional argument could be added to the clk_register*() > functions. Something like: > > ---------8<------------------ > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 27f8c42..ef934ac 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev, > struct clk_hw *hw) > } > EXPORT_SYMBOL_GPL(__clk_register); > > -static int _clk_register(struct device *dev, struct clk_hw *hw, struct > clk *clk) > +static int _clk_register(struct device *dev, struct clk_hw *hw, struct > clk *clk, > + struct module *owner) > { > int i, ret; > > @@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev, > struct clk_hw *hw, struct clk *clk) > } > } > > + clk->owner = owner; > + > ret = __clk_init(dev, clk); > if (!ret) > return 0; > @@ -1892,7 +1895,7 @@ fail_name: > } > > /** > - * clk_register - allocate a new clock, register it and return an > opaque cookie > + * ___clk_register - allocate a new clock, register it and return an > opaque cookie > * @dev: device that is registering this clock > * @hw: link to hardware-specific clock data > * > @@ -1902,7 +1905,8 @@ fail_name: > * rest of the clock API. In the event of an error clk_register will > return an > * error code; drivers must test for an error code after calling > clk_register. > */ > -struct clk *clk_register(struct device *dev, struct clk_hw *hw) > +struct clk *___clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner) > { > int ret; > struct clk *clk; > @@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev, > struct clk_hw *hw) > goto fail_out; > } > > - ret = _clk_register(dev, hw, clk); > + ret = _clk_register(dev, hw, clk, owner); > if (!ret) > return clk; > > @@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev, > struct clk_hw *hw) > fail_out: > return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(clk_register); > +EXPORT_SYMBOL_GPL(___clk_register); > > /* > * Free memory allocated for a clock. > @@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev, > void *res) > * automatically clk_unregister()ed on driver detach. See > clk_register() for > * more information. > */ > -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) > +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner) > { > struct clk *clk; > int ret; > @@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev, > struct clk_hw *hw) > if (!clk) > return ERR_PTR(-ENOMEM); > > - ret = _clk_register(dev, hw, clk); > + ret = _clk_register(dev, hw, clk, owner); > if (!ret) { > devres_add(dev, clk); > } else { > @@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev, > struct clk_hw *hw) > > return clk; > } > -EXPORT_SYMBOL_GPL(devm_clk_register); > +EXPORT_SYMBOL_GPL(__devm_clk_register); > > static int devm_clk_match(struct device *dev, void *res, void *data) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 13623f3..c656ebf 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -31,6 +31,7 @@ > #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate > change */ > > struct clk_hw; > +struct module; > > /** > * struct clk_ops - Callback operations for hardware clocks; these are to > @@ -436,8 +437,21 @@ struct clk *clk_register_composite(struct device > *dev, const char *name, > * rest of the clock API. In the event of an error clk_register will > return an > * error code; drivers must test for an error code after calling > clk_register. > */ > -struct clk *clk_register(struct device *dev, struct clk_hw *hw); > -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw); > +struct clk *___clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner); > + > +static inline struct clk *clk_register(struct device *dev, struct > clk_hw *hw) > +{ > + return ___clk_register(dev, hw, THIS_MODULE); > +} > + > +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw, > + struct module *owner); > +static inline struct clk *devm_clk_register(struct device *dev, > + struct clk_hw *hw) > +{ > + return __devm_clk_register(dev, hw, THIS_MODULE); > +} > > void clk_unregister(struct clk *clk); > void devm_clk_unregister(struct device *dev, struct clk *clk); > > ---------8<------------------ > > Similarly it would need to be done for the remaining clk_register*() > functions, > which have much longer arguments list. > > It's a bit messy, since there is already __clk_register() function with > double > underscore prefix. Perhaps that could be renamed to something else so > all the > functions taking struct module * parameter are prefixed with double > underscore. > > > I would squash patches 3/5 and 4/5 in the next iteration, if it is decided > to keep using dev->driver->owner. > > -- > Thanks, > Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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<------------------ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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<------------------ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Hi Sylwester, On Wednesday 25 September 2013 22:51:08 Sylwester Nawrocki wrote: > On 09/25/2013 11:47 AM, Laurent Pinchart wrote: > > 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. > > It doesn't introduce any new race conditions AFAICT. I doubt all these > issues can be resolved in one single step. Currently the modular clock > providers are seriously broken, there is no reference tracking and the clock > consumers can easily get into a state where they have invalid references to > clocks supplied by already unregistered drivers. > > With this patch series the clock consumer drivers will not crash thanks > to the clock object reference counting. So the worst thing may happen is a > clock left in an unexpected state. > > However there should be no problems with the v4l2-async API, the host driver > in its de-initialization routine unregisters its sub-drivers (they should > stop using the clock when notified of such an event), only then the host > would unregister the clock (subsequently the sub-drivers get re-attached and > put into deferred probing state). That in itself is a workaround I believe. Unbinding/rebinding devices from/to drivers isn't something the v4l core should do. > There may be issues when a sub-driver's file handle is opened while the host > is about to de-initialize. But I doubt resolution of such problems belongs > to the common clock framework. I have been trying to improve the situation > in small steps, rather than waiting forever for a perfect solution. > > Do you perhaps have any ideas WRT to a "more generic solution" ? In general > I have been trying to avoid using v4l2-clk and add what's missing in the > common clock framework. > > Perhaps we should leave only the kref part in the __clk_get(), __clk_put() > hooks and be taking reference to a clock in clk_prepare() and releasing it > in clk_unprepare() ? This way circular reference would exist only between > clk_prepare(), clk_unprepare() calls. > > Assuming a driver prepares clock in device's open() and unprepares in device > close() handler perhaps it could all work better, without relying on the > host to ensure the resource reference tracking. I'm not sure if that is not > making too many assumptions for a generic API. This is indeed an architecture decision that goes beyond the boundaries of the clock framework. The question boils down to how we want to acquire/release and refcount resources. Should drivers acquire and release hotpluggable resources at probe and remove time respectively, or only when they need them ? Or should they acquire them at probe them and be notified when they should release them ? The first option adds an overhead but could help solving the circular dependency problem in a simpler way.
Hi Sylwester, On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: > On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote: > > This patch series implements clock deregistration in the common clock > > framework. Comparing to v5 it only includes further corrections of NULL > > clock handling. > > [...] > > > clk: Provide not locked variant of of_clk_get_from_provider() > > clkdev: Fix race condition in clock lookup from device tree > > clk: Add common __clk_get(), __clk_put() implementations > > clk: Assign module owner of a clock being registered > > clk: Implement clk_unregister > > Hi Mike, Russell, > > Would you have any further comments/suggestions on this series ? > > I have inspected all callers of clk_register() and all should be fine > with regards to dereferencing dev->driver. The first argument to this > function is either NULL or clk_register() is being called in drivers' > probe() callback, which ensures dev->driver won't change due to holding > dev->mutex. > > 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. > > ---------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); I don't think you've compile-tested this :-) > + > 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); This doesn't introduce any regression in the sense that it will trade a problem for another one, so I'm fine with it in the short. Could you add a small comment above the clk_register() call to explain why the first argument is NULL ? > > 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 */
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<------------------
Hi Laurent, On 10/28/2013 10:05 PM, Laurent Pinchart wrote: > On Tuesday 24 September 2013 23:38:44 Sylwester Nawrocki wrote: >> On 08/30/2013 04:53 PM, Sylwester Nawrocki wrote: >>> This patch series implements clock deregistration in the common clock >>> framework. Comparing to v5 it only includes further corrections of NULL >>> clock handling. [...] >> ---------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); > > I don't think you've compile-tested this :-) Thank you for the comments. Yeah, I messed up this, I thought this part got recompiled but it didn't. I've fixed this in the recently posted patch. >> + >> 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); > > This doesn't introduce any regression in the sense that it will trade a > problem for another one, so I'm fine with it in the short. Could you add a > small comment above the clk_register() call to explain why the first argument > is NULL ? I'm not entirely happy about doing something like that. Nevertheless I didn't hear so far any better proposals and I guess it could be treated as a short term modification while we're working on proper handling of those circular references. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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<------------------ Alternatively an additional argument could be added to the clk_register*() functions. Something like: ---------8<------------------ diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 27f8c42..ef934ac 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1837,7 +1837,8 @@ struct clk *__clk_register(struct device *dev, struct clk_hw *hw) } EXPORT_SYMBOL_GPL(__clk_register); -static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) +static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk, + struct module *owner) { int i, ret; @@ -1877,6 +1878,8 @@ static int _clk_register(struct device *dev, struct clk_hw *hw, struct clk *clk) } } + clk->owner = owner; + ret = __clk_init(dev, clk); if (!ret) return 0; @@ -1892,7 +1895,7 @@ fail_name: } /** - * clk_register - allocate a new clock, register it and return an opaque cookie + * ___clk_register - allocate a new clock, register it and return an opaque cookie * @dev: device that is registering this clock * @hw: link to hardware-specific clock data * @@ -1902,7 +1905,8 @@ fail_name: * rest of the clock API. In the event of an error clk_register will return an * error code; drivers must test for an error code after calling clk_register. */ -struct clk *clk_register(struct device *dev, struct clk_hw *hw) +struct clk *___clk_register(struct device *dev, struct clk_hw *hw, + struct module *owner) { int ret; struct clk *clk; @@ -1914,7 +1918,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) goto fail_out; } - ret = _clk_register(dev, hw, clk); + ret = _clk_register(dev, hw, clk, owner); if (!ret) return clk; @@ -1922,7 +1926,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) fail_out: return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(clk_register); +EXPORT_SYMBOL_GPL(___clk_register); /* * Free memory allocated for a clock. @@ -2040,7 +2044,8 @@ static void devm_clk_release(struct device *dev, void *res) * automatically clk_unregister()ed on driver detach. See clk_register() for * more information. */ -struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) +struct clk *__devm_clk_register(struct device *dev, struct clk_hw *hw, + struct module *owner) { struct clk *clk; int ret; @@ -2049,7 +2054,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) if (!clk) return ERR_PTR(-ENOMEM); - ret = _clk_register(dev, hw, clk); + ret = _clk_register(dev, hw, clk, owner); if (!ret) { devres_add(dev, clk); } else { @@ -2059,7 +2064,7 @@ struct clk *devm_clk_register(struct device *dev, struct clk_hw *hw) return clk; } -EXPORT_SYMBOL_GPL(devm_clk_register); +EXPORT_SYMBOL_GPL(__devm_clk_register); static int devm_clk_match(struct device *dev, void *res, void *data) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 13623f3..c656ebf 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -31,6 +31,7 @@ #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */ struct clk_hw; +struct module; /**