| Message ID | 1376066046-16037-2-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sylwester Nawrocki (2013-08-09 09:34:05) > Add helper functions for the of_clk_providers list locking and > an unlocked variant of of_clk_get_from_provider(). > These functions are intended to be used in the clkdev to avoid > race condition in the device tree based clock look up in clk_get(). > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Looks good to me. Russell, Any objections? Regards, Mike > --- > drivers/clk/clk.c | 36 ++++++++++++++++++++++++++++-------- > include/linux/clk.h | 3 +++ > 2 files changed, 31 insertions(+), 8 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 0d4c982..8b8c152 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2143,7 +2143,18 @@ static const struct of_device_id __clk_of_table_sentinel > __used __section(__clk_of_table_end); > > static LIST_HEAD(of_clk_providers); > -static DEFINE_MUTEX(of_clk_lock); > +static DEFINE_MUTEX(of_clk_mutex); > + > +/* of_clk_provider list locking helpers */ > +void of_clk_lock(void) > +{ > + mutex_lock(&of_clk_mutex); > +} > + > +void of_clk_unlock(void) > +{ > + mutex_unlock(&of_clk_mutex); > +} > > struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, > void *data) > @@ -2187,9 +2198,9 @@ int of_clk_add_provider(struct device_node *np, > cp->data = data; > cp->get = clk_src_get; > > - mutex_lock(&of_clk_lock); > + mutex_lock(&of_clk_mutex); > list_add(&cp->link, &of_clk_providers); > - mutex_unlock(&of_clk_lock); > + mutex_unlock(&of_clk_mutex); > pr_info("Added clock from %s\n", np->full_name); > > return 0; > @@ -2204,7 +2215,7 @@ void of_clk_del_provider(struct device_node *np) > { > struct of_clk_provider *cp; > > - mutex_lock(&of_clk_lock); > + mutex_lock(&of_clk_mutex); > list_for_each_entry(cp, &of_clk_providers, link) { > if (cp->node == np) { > list_del(&cp->link); > @@ -2213,24 +2224,33 @@ void of_clk_del_provider(struct device_node *np) > break; > } > } > - mutex_unlock(&of_clk_lock); > + mutex_unlock(&of_clk_mutex); > } > EXPORT_SYMBOL_GPL(of_clk_del_provider); > > -struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) > +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec) > { > struct of_clk_provider *provider; > struct clk *clk = ERR_PTR(-ENOENT); > > /* Check if we have such a provider in our array */ > - mutex_lock(&of_clk_lock); > list_for_each_entry(provider, &of_clk_providers, link) { > if (provider->node == clkspec->np) > clk = provider->get(clkspec, provider->data); > if (!IS_ERR(clk)) > break; > } > - mutex_unlock(&of_clk_lock); > + > + return clk; > +} > + > +struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) > +{ > + struct clk *clk; > + > + mutex_lock(&of_clk_mutex); > + clk = __of_clk_get_from_provider(clkspec); > + mutex_unlock(&of_clk_mutex); > > return clk; > } > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 9a6d045..ea6822e 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -368,6 +368,9 @@ struct of_phandle_args; > struct clk *of_clk_get(struct device_node *np, int index); > struct clk *of_clk_get_by_name(struct device_node *np, const char *name); > struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); > +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec); > +void of_clk_lock(void); > +void of_clk_unlock(void); > #else > static inline struct clk *of_clk_get(struct device_node *np, int index) > { > -- > 1.7.9.5
On Mon, Aug 19, 2013 at 12:41:32PM -0700, Mike Turquette wrote: > Quoting Sylwester Nawrocki (2013-08-09 09:34:05) > > Add helper functions for the of_clk_providers list locking and > > an unlocked variant of of_clk_get_from_provider(). > > These functions are intended to be used in the clkdev to avoid > > race condition in the device tree based clock look up in clk_get(). > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Looks good to me. > > Russell, > > Any objections? Yes. > > diff --git a/include/linux/clk.h b/include/linux/clk.h > > index 9a6d045..ea6822e 100644 > > --- a/include/linux/clk.h > > +++ b/include/linux/clk.h > > @@ -368,6 +368,9 @@ struct of_phandle_args; > > struct clk *of_clk_get(struct device_node *np, int index); > > struct clk *of_clk_get_by_name(struct device_node *np, const char *name); > > struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); > > +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec); > > +void of_clk_lock(void); > > +void of_clk_unlock(void); Here's the thing. Yes, I know we have a directory called 'include' where header files live, but there is nothing layed down which requires all header files to be under a directory called 'include' - especially when it's stuff which should _not_ be shared with the rest of the kernel. In this case, these three functions are merely exported from drivers/clk/clk.c to drivers/clk/clkdev.c - which is in the very same directory. It is not intended that these functions be used for any other purpose. So, why not put them in a header file in drivers/clk/ which both these files can include? I *really* wish that people would get out of their mind that everything has to live in a header file in include/ or arch/arm/include/ and local includes are bad. Local includes are a very good thing, they aid in reducing the visibility of stuff which is not meant to be visible to the entire kernel. Just look at things like fs/mount.h to see what I mean. Or the various driver header files which contain definitions only for use by their associated drivers in the drivers/ subtree.
On 08/19/2013 09:50 PM, Russell King - ARM Linux wrote: > On Mon, Aug 19, 2013 at 12:41:32PM -0700, Mike Turquette wrote: >> Quoting Sylwester Nawrocki (2013-08-09 09:34:05) >>> Add helper functions for the of_clk_providers list locking and >>> an unlocked variant of of_clk_get_from_provider(). >>> These functions are intended to be used in the clkdev to avoid >>> race condition in the device tree based clock look up in clk_get(). >>> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> >> Looks good to me. >> >> Russell, >> >> Any objections? > > Yes. > >>> diff --git a/include/linux/clk.h b/include/linux/clk.h >>> index 9a6d045..ea6822e 100644 >>> --- a/include/linux/clk.h >>> +++ b/include/linux/clk.h >>> @@ -368,6 +368,9 @@ struct of_phandle_args; >>> struct clk *of_clk_get(struct device_node *np, int index); >>> struct clk *of_clk_get_by_name(struct device_node *np, const char *name); >>> struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); >>> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec); >>> +void of_clk_lock(void); >>> +void of_clk_unlock(void); > > Here's the thing. Yes, I know we have a directory called 'include' > where header files live, but there is nothing layed down which requires > all header files to be under a directory called 'include' - especially > when it's stuff which should _not_ be shared with the rest of the > kernel. > > In this case, these three functions are merely exported from > drivers/clk/clk.c to drivers/clk/clkdev.c - which is in the very same > directory. It is not intended that these functions be used for any > other purpose. > > So, why not put them in a header file in drivers/clk/ which both these > files can include? Yes, that's certainly a good idea. In fact it bothered me a bit that those functions are unnecessarily exposed like that. But I somehow missed a separate header could be simply added. Thanks for the suggestion. I'm going to post v2 of $subject series and the one adding clk_unregister() implementation including corrections for those couple issues pointed out in the reviews. Would appreciate to know you opinion on those and perhaps have your Ack so Mike could take whole series into his tree. > I *really* wish that people would get out of their mind that everything > has to live in a header file in include/ or arch/arm/include/ and local > includes are bad. Local includes are a very good thing, they aid in > reducing the visibility of stuff which is not meant to be visible to > the entire kernel. > > Just look at things like fs/mount.h to see what I mean. Or the various > driver header files which contain definitions only for use by their > associated drivers in the drivers/ subtree. -- Thanks, Sylwester
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0d4c982..8b8c152 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2143,7 +2143,18 @@ static const struct of_device_id __clk_of_table_sentinel __used __section(__clk_of_table_end); static LIST_HEAD(of_clk_providers); -static DEFINE_MUTEX(of_clk_lock); +static DEFINE_MUTEX(of_clk_mutex); + +/* of_clk_provider list locking helpers */ +void of_clk_lock(void) +{ + mutex_lock(&of_clk_mutex); +} + +void of_clk_unlock(void) +{ + mutex_unlock(&of_clk_mutex); +} struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, void *data) @@ -2187,9 +2198,9 @@ int of_clk_add_provider(struct device_node *np, cp->data = data; cp->get = clk_src_get; - mutex_lock(&of_clk_lock); + mutex_lock(&of_clk_mutex); list_add(&cp->link, &of_clk_providers); - mutex_unlock(&of_clk_lock); + mutex_unlock(&of_clk_mutex); pr_info("Added clock from %s\n", np->full_name); return 0; @@ -2204,7 +2215,7 @@ void of_clk_del_provider(struct device_node *np) { struct of_clk_provider *cp; - mutex_lock(&of_clk_lock); + mutex_lock(&of_clk_mutex); list_for_each_entry(cp, &of_clk_providers, link) { if (cp->node == np) { list_del(&cp->link); @@ -2213,24 +2224,33 @@ void of_clk_del_provider(struct device_node *np) break; } } - mutex_unlock(&of_clk_lock); + mutex_unlock(&of_clk_mutex); } EXPORT_SYMBOL_GPL(of_clk_del_provider); -struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec) { struct of_clk_provider *provider; struct clk *clk = ERR_PTR(-ENOENT); /* Check if we have such a provider in our array */ - mutex_lock(&of_clk_lock); list_for_each_entry(provider, &of_clk_providers, link) { if (provider->node == clkspec->np) clk = provider->get(clkspec, provider->data); if (!IS_ERR(clk)) break; } - mutex_unlock(&of_clk_lock); + + return clk; +} + +struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) +{ + struct clk *clk; + + mutex_lock(&of_clk_mutex); + clk = __of_clk_get_from_provider(clkspec); + mutex_unlock(&of_clk_mutex); return clk; } diff --git a/include/linux/clk.h b/include/linux/clk.h index 9a6d045..ea6822e 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -368,6 +368,9 @@ struct of_phandle_args; struct clk *of_clk_get(struct device_node *np, int index); struct clk *of_clk_get_by_name(struct device_node *np, const char *name); struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec); +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec); +void of_clk_lock(void); +void of_clk_unlock(void); #else static inline struct clk *of_clk_get(struct device_node *np, int index) {