| Message ID | 20180628075453.GA7793@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Matti Vaittinen (2018-06-28 00:54:53) > Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and > devm_clk_release_clkdev as a first styep to clean up drivers which are s/styep/step/ > leaking clkdev lookups at driver remove. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > > drivers/clk/clkdev.c | 165 +++++++++++++++++++++++++++++++++++++++++-------- > include/linux/clkdev.h | 8 +++ Also need to update the Documentation file at Documentation/driver-model/devres.txt > 2 files changed, 147 insertions(+), 26 deletions(-) > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 7513411140b6..4752a0004a6c 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl) > } > EXPORT_SYMBOL(clkdev_drop); > > -static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > +static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw, Don't rename this. > const char *con_id, > const char *dev_id, ...) > { > @@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > return cl; > } > > +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > + const char *con_id, const char *dev_id) > +{ > + struct clk_lookup *cl; > + > + /* > + * Since dev_id can be NULL, and NULL is handled specially, we must > + * pass it as either a NULL format string, or with "%s". > + */ > + if (dev_id) > + cl = do_clk_register_clkdev(hw, con_id, "%s", > + dev_id); > + else > + cl = do_clk_register_clkdev(hw, con_id, NULL); > + > + return cl; I think this is the same code as before? Try to minimize the diff so we can focus on what's really changing. > +} > + > /** > * clk_register_clkdev - register one clock lookup for a struct clk > * @clk: struct clk to associate with all clk_lookups [...] > + > +/** > + * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw > + * @dev: device this lookup is bound > + * @hw: struct clk_hw to associate with all clk_lookups > + * @con_id: connection ID string on device > + * @dev_id: format string describing device name > + * > + * con_id or dev_id may be NULL as a wildcard, just as in the rest of > + * clkdev. > + * > + * To make things easier for mass registration, we detect error clk_hws > + * from a previous clk_hw_register_*() call, and return the error code for > + * those. This is to permit this function to be called immediately > + * after clk_hw_register_*(). > + */ > +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, > + const char *con_id, const char *dev_id) > +{ > + struct clk_lookup **cl = NULL; Don't assign to NULL to just overwrite it later. > > if (IS_ERR(hw)) > return PTR_ERR(hw); Put another newline here. > + cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); > + if (cl) { > + *cl = __clk_register_clkdev(hw, con_id, dev_id); Why can't we just call clk_hw_register_clkdev()? Sure the IS_ERR() chain is duplicated, but that can be left out of the devm version and rely on the clk_hw_register_clkdev() to take care of it otherwise. > + if (*cl) > + devres_add(dev, cl); > + else > + devres_free(cl); > + } > + return (cl && *cl) ? 0 : -ENOMEM; > +} > +EXPORT_SYMBOL(devm_clk_hw_register_clkdev); > > - /* > - * Since dev_id can be NULL, and NULL is handled specially, we must > - * pass it as either a NULL format string, or with "%s". > - */ > - if (dev_id) > - cl = __clk_register_clkdev(hw, con_id, "%s", dev_id); > - else > - cl = __clk_register_clkdev(hw, con_id, NULL); > - > - return cl ? 0 : -ENOMEM; > +/** > + * devm_clk_register_clkdev - managed clk lookup registration for a struct clk > + * @dev: device this lookup is bound > + * @clk: struct clk to associate with all clk_lookups > + * @con_id: connection ID string on device > + * @dev_id: string describing device name > + * > + * con_id or dev_id may be NULL as a wildcard, just as in the rest of > + * clkdev. > + * > + * To make things easier for mass registration, we detect error clks > + * from a previous clk_register() call, and return the error code for > + * those. This is to permit this function to be called immediately > + * after clk_register(). > + */ > +int devm_clk_register_clkdev(struct device *dev, struct clk *clk, > + const char *con_id, const char *dev_id) I wouldn't even add this function, to encourage driver writers to migrate to clk_hw based registration functions and to avoid removing it later on. > +{ > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + return devm_clk_hw_register_clkdev(dev, __clk_get_hw(clk), con_id, > + dev_id); > } > -EXPORT_SYMBOL(clk_hw_register_clkdev); > +EXPORT_SYMBOL(devm_clk_register_clkdev); > diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h > index 4890ff033220..27ebeae8ae26 100644 > --- a/include/linux/clkdev.h > +++ b/include/linux/clkdev.h > @@ -52,4 +52,12 @@ int clk_add_alias(const char *, const char *, const char *, struct device *); > int clk_register_clkdev(struct clk *, const char *, const char *); > int clk_hw_register_clkdev(struct clk_hw *, const char *, const char *); > > +int devm_clk_register_clkdev(struct device *dev, struct clk *clk, > + const char *con_id, const char *dev_id); > +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, > + const char *con_id, const char *dev_id); > +void devm_clk_release_clkdev(struct device *dev, const char *con_id, > + const char *dev_id); > + > + > #endif
Hello all, Sorry for html email. I'm not used to send mails using gmail app. Trying to resend as plain text now... On 7/17/18, Matti Vaittinen <mazziesaccount@gmail.com> wrote: > ma 9. heinäk. 2018 klo 9.33 Stephen Boyd <sboyd@kernel.org> kirjoitti: > >> Quoting Matti Vaittinen (2018-06-28 00:54:53) >> > Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and >> > devm_clk_release_clkdev as a first styep to clean up drivers which are >> >> s/styep/step/ >> > > Snip... > > I'll respond to these comments when I am back from my summer vacation trip > after a week or two. > > Best regards > Matti Vaittinen > > >> >
Hello All, Sorry for longish delay but the exceptionally great summer in Finland has kept me away from computer... Now when I am back from my travels it's time to focus on patches again =) On Sun, Jul 08, 2018 at 11:33:44PM -0700, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-06-28 00:54:53) > > Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and > > devm_clk_release_clkdev as a first styep to clean up drivers which are > > s/styep/step/ Thanks. > > leaking clkdev lookups at driver remove. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > > > drivers/clk/clkdev.c | 165 +++++++++++++++++++++++++++++++++++++++++-------- > > include/linux/clkdev.h | 8 +++ > > Also need to update the Documentation file at > Documentation/driver-model/devres.txt Right. I'd better check that file then. Thanks for pointing it out. > > > 2 files changed, 147 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > > index 7513411140b6..4752a0004a6c 100644 > > --- a/drivers/clk/clkdev.c > > +++ b/drivers/clk/clkdev.c > > @@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl) > > } > > EXPORT_SYMBOL(clkdev_drop); > > > > -static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > > +static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw, > > Don't rename this. > I did rename this as I introduced new internal __clk_register_clkdev (see below) - which is utilized by the clk_register_clkdev, clk_hw_register_clkdev and devm_clk_hw_register_clkdev. This allowed me to cut off some duplicated code from clk_register_clkdev and clk_hw_register_clkdev. (Mainly the: /* * Since dev_id can be NULL, and NULL is handled specially, we must * pass it as either a NULL format string, or with "%s". */ if (dev_id) ... con_id, "%s", dev_id); else ... con_id, NULL); parameter selection for old __clk_register_clkdev (which I renamed to do_clk_register_clkdev). So I tried to reduce code by deciding this only in the new wrapper function __clk_register_clkdev. For me it was more obvioust that __clk_register_clkdev would be next internal layer for clk_register_clkdev. The old __clk_register_clkdev - which is now named as do_clk_register_clkdev is the final layer doing lookup and registration. > > const char *con_id, > > const char *dev_id, ...) > > { > > @@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > > return cl; > > } > > > > +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, > > + const char *con_id, const char *dev_id) > > +{ > > + struct clk_lookup *cl; > > + > > + /* > > + * Since dev_id can be NULL, and NULL is handled specially, we must > > + * pass it as either a NULL format string, or with "%s". > > + */ > > + if (dev_id) > > + cl = do_clk_register_clkdev(hw, con_id, "%s", > > + dev_id); > > + else > > + cl = do_clk_register_clkdev(hw, con_id, NULL); > > + > > + return cl; > > I think this is the same code as before? Try to minimize the diff so we > can focus on what's really changing. > This is code that earlier was duiplicated in both the clk_register_clkdev and clk_hw_register_clkdev. I cleaned the code duplication by adding this new __clk_register_clkdev function. > > +} > > + > > /** > > * clk_register_clkdev - register one clock lookup for a struct clk > > * @clk: struct clk to associate with all clk_lookups > [...] > > + > > +/** > > + * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw > > + * @dev: device this lookup is bound > > + * @hw: struct clk_hw to associate with all clk_lookups > > + * @con_id: connection ID string on device > > + * @dev_id: format string describing device name > > + * > > + * con_id or dev_id may be NULL as a wildcard, just as in the rest of > > + * clkdev. > > + * > > + * To make things easier for mass registration, we detect error clk_hws > > + * from a previous clk_hw_register_*() call, and return the error code for > > + * those. This is to permit this function to be called immediately > > + * after clk_hw_register_*(). > > + */ > > +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, > > + const char *con_id, const char *dev_id) > > +{ > > + struct clk_lookup **cl = NULL; > > Don't assign to NULL to just overwrite it later. Right. > > > > if (IS_ERR(hw)) > > return PTR_ERR(hw); > > Put another newline here. > Ok. > > + cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); > > + if (cl) { > > + *cl = __clk_register_clkdev(hw, con_id, dev_id); > > Why can't we just call clk_hw_register_clkdev()? Sure the IS_ERR() > chain is duplicated, but that can be left out of the devm version and > rely on the clk_hw_register_clkdev() to take care of it otherwise. > We could. But as I anyways introduced the new __clk_register_clkdev - in order to slighly simplify clk_register_clkdev and clk_hw_register_clkdev - it was convenient to not dublicate the IS_ERR chain and use the interal __clk_register_clkdev -variant. And actually, I was not sure if it is required to have some fast handling for the IS_ERR cases here and hence I thought it should be checked before devres_alloc. But if there is no need for priorizing this check - then I would remove IS_ERR checks from devm_clk_hw_register_clkdev and clk_hw_register_clkdev and do it only in the __clk_register_clkdev. Unfortunately we need to keep it in clk_register_clkdev because this must be checked before we do __clk_get_hw(clk). Anyways, that would further simplify this to something like (untested, not even compiled code below which is only meant to explain what I mean): static int __clk_register_clkdev(struct clk_hw *hw, struct clk_lookup **cl, const char *con_id, const char *dev_id) { if (IS_ERR(hw)) return PTR_ERR(hw); if (dev_id) *cl = do_clk_register_clkdev(hw, con_id, "%s", dev_id); else *cl = do_clk_register_clkdev(hw, con_id, NULL); return (*cl) ? 0 : -ENOMEM; int clk_register_clkdev(struct clk *clk, const char *con_id, const char *dev_id) { int rval; struct clk_lookup *cl; if (!IS_ERR(clk)) return __clk_register_clkdev(__clk_get_hw(clk), &cl, con_id, dev_id); return PTR_ERR(clk); } int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id, const char *dev_id) { int rval; struct clk_lookup *cl; return __clk_register_clkdev(hw, con_id, &cl, dev_id); } int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, const char *con_id, const char *dev_id) { struct clk_lookup **cl; int rval = -ENOMEM; if (IS_ERR(hw)) return PTR_ERR(hw); cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); if (cl) { rval = __clk_register_clkdev(hw, cl, con_id, dev_id); if (!rval) devres_add(dev, cl); else devres_free(cl); } return rval; } EXPORT_SYMBOL(devm_clk_hw_register_clkdev); or do you prefer that I do not touch the existing clk_register_clkdev and clk_hw_register_clkdev at all and only add devm_clk_hw_register_clkdev? If that's what you prefer we can go with it too. I just think doing the if (dev_id) ... con_id, "%s", dev_id); else ... con_id, NULL); selection only in one function makes this cleaner. > > +/** > > + * devm_clk_register_clkdev - managed clk lookup registration for a struct clk > > + * @dev: device this lookup is bound > > + * @clk: struct clk to associate with all clk_lookups > > + * @con_id: connection ID string on device > > + * @dev_id: string describing device name > > + * > > + * con_id or dev_id may be NULL as a wildcard, just as in the rest of > > + * clkdev. > > + * > > + * To make things easier for mass registration, we detect error clks > > + * from a previous clk_register() call, and return the error code for > > + * those. This is to permit this function to be called immediately > > + * after clk_register(). > > + */ > > +int devm_clk_register_clkdev(struct device *dev, struct clk *clk, > > + const char *con_id, const char *dev_id) > > I wouldn't even add this function, to encourage driver writers to > migrate to clk_hw based registration functions and to avoid removing it > later on. I can remove this. Best regards Matti Vaittinen
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 7513411140b6..4752a0004a6c 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl) } EXPORT_SYMBOL(clkdev_drop); -static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, +static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw, const char *con_id, const char *dev_id, ...) { @@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, return cl; } +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, + const char *con_id, const char *dev_id) +{ + struct clk_lookup *cl; + + /* + * Since dev_id can be NULL, and NULL is handled specially, we must + * pass it as either a NULL format string, or with "%s". + */ + if (dev_id) + cl = do_clk_register_clkdev(hw, con_id, "%s", + dev_id); + else + cl = do_clk_register_clkdev(hw, con_id, NULL); + + return cl; +} + /** * clk_register_clkdev - register one clock lookup for a struct clk * @clk: struct clk to associate with all clk_lookups @@ -421,22 +439,18 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw, int clk_register_clkdev(struct clk *clk, const char *con_id, const char *dev_id) { - struct clk_lookup *cl; - - if (IS_ERR(clk)) - return PTR_ERR(clk); + int rval = 0; - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s", - dev_id); - else - cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL); + if (IS_ERR(clk)) { + rval = PTR_ERR(clk); + } else { + struct clk_lookup *cl; - return cl ? 0 : -ENOMEM; + cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, dev_id); + if (!cl) + rval = -ENOMEM; + } + return rval; } EXPORT_SYMBOL(clk_register_clkdev); @@ -456,21 +470,120 @@ EXPORT_SYMBOL(clk_register_clkdev); */ int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id, const char *dev_id) +{ + int rval = 0; + + if (IS_ERR(hw)) { + rval = PTR_ERR(hw); + } else { + struct clk_lookup *cl; + + cl = __clk_register_clkdev(hw, con_id, dev_id); + if (!cl) + rval = -ENOMEM; + } + + return rval; +} +EXPORT_SYMBOL(clk_hw_register_clkdev); + +static void devm_clkdev_release(struct device *dev, void *res) +{ + clkdev_drop(*(struct clk_lookup **)res); +} + +static int devm_clk_match_clkdev(struct device *dev, void *res, void *data) +{ + struct clk_lookup **l = res; + + if (!l || !*l) { + WARN_ON(!l || !*l); + return 0; + } + + return *l == data; +} + +/** + * devm_clk_release_clkdev - Resource managed clkdev lookup release + * @dev: device this lookup is bound + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * Drop the clkdev lookup created with devm_clk_hw_register_clkdev or + * with devm_clk_register_clkdev. Normally this function will not need to be + * called and the resource management code will ensure that the resource is + * freed. + */ +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id) { struct clk_lookup *cl; + int rval; + + cl = clk_find(dev_id, con_id); + WARN_ON(!cl); + rval = devres_release(dev, devm_clkdev_release, + &devm_clk_match_clkdev, cl); + WARN_ON(rval); +} +EXPORT_SYMBOL(devm_clk_release_clkdev); + +/** + * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw + * @dev: device this lookup is bound + * @hw: struct clk_hw to associate with all clk_lookups + * @con_id: connection ID string on device + * @dev_id: format string describing device name + * + * con_id or dev_id may be NULL as a wildcard, just as in the rest of + * clkdev. + * + * To make things easier for mass registration, we detect error clk_hws + * from a previous clk_hw_register_*() call, and return the error code for + * those. This is to permit this function to be called immediately + * after clk_hw_register_*(). + */ +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id) +{ + struct clk_lookup **cl = NULL; if (IS_ERR(hw)) return PTR_ERR(hw); + cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL); + if (cl) { + *cl = __clk_register_clkdev(hw, con_id, dev_id); + if (*cl) + devres_add(dev, cl); + else + devres_free(cl); + } + return (cl && *cl) ? 0 : -ENOMEM; +} +EXPORT_SYMBOL(devm_clk_hw_register_clkdev); - /* - * Since dev_id can be NULL, and NULL is handled specially, we must - * pass it as either a NULL format string, or with "%s". - */ - if (dev_id) - cl = __clk_register_clkdev(hw, con_id, "%s", dev_id); - else - cl = __clk_register_clkdev(hw, con_id, NULL); - - return cl ? 0 : -ENOMEM; +/** + * devm_clk_register_clkdev - managed clk lookup registration for a struct clk + * @dev: device this lookup is bound + * @clk: struct clk to associate with all clk_lookups + * @con_id: connection ID string on device + * @dev_id: string describing device name + * + * con_id or dev_id may be NULL as a wildcard, just as in the rest of + * clkdev. + * + * To make things easier for mass registration, we detect error clks + * from a previous clk_register() call, and return the error code for + * those. This is to permit this function to be called immediately + * after clk_register(). + */ +int devm_clk_register_clkdev(struct device *dev, struct clk *clk, + const char *con_id, const char *dev_id) +{ + if (IS_ERR(clk)) + return PTR_ERR(clk); + return devm_clk_hw_register_clkdev(dev, __clk_get_hw(clk), con_id, + dev_id); } -EXPORT_SYMBOL(clk_hw_register_clkdev); +EXPORT_SYMBOL(devm_clk_register_clkdev); diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h index 4890ff033220..27ebeae8ae26 100644 --- a/include/linux/clkdev.h +++ b/include/linux/clkdev.h @@ -52,4 +52,12 @@ int clk_add_alias(const char *, const char *, const char *, struct device *); int clk_register_clkdev(struct clk *, const char *, const char *); int clk_hw_register_clkdev(struct clk_hw *, const char *, const char *); +int devm_clk_register_clkdev(struct device *dev, struct clk *clk, + const char *con_id, const char *dev_id); +int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw, + const char *con_id, const char *dev_id); +void devm_clk_release_clkdev(struct device *dev, const char *con_id, + const char *dev_id); + + #endif
Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and devm_clk_release_clkdev as a first styep to clean up drivers which are leaking clkdev lookups at driver remove. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- While searching for example on how clk drivers release clkdev at exit I found that many of those don't. Simple grep for clkdev under drivers/clk suggest that bunch of drivers which call clk_register_clkdev or clk_hw_register_clkdev never call clkdev_drop. In order to help cleaning this up I decided to add devm versions of clk_register_clkdev and clk_hw_register_clkdev. I hope this would allow simply converting bunch of calls to clk_register_clkdev and clk_hw_register_clkdev into managed versions without building further clean up logic. Please review this carefully. I have only performed some simple tests with my BD71837 driver. And as I have only limited understanding on clkdev lookups I may have done mistakes. Thanks! drivers/clk/clkdev.c | 165 +++++++++++++++++++++++++++++++++++++++++-------- include/linux/clkdev.h | 8 +++ 2 files changed, 147 insertions(+), 26 deletions(-)