On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
<[email protected]> wrote:
I've stumbled across a of_node_get/put imbalance that happens when the
fixed rate clock is added and deleted using device tree. The cause is
that this driver calls of_clk_add_provider() when probed, but doesn't
call of_clk_del_provider() when removed.
It looks like a lot of clock drivers share that issue:
$ cd drivers/clk/
$ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
131
It should be a one line fix, but for many files.
I'm not a clock subsystem expert, so please let me know whether I'm
missing something here.
Thanks,
Alan
>
> Adds support for fixed-rate clock providers which have not been
> enabled via of_clk_init().
>
> This is required by Device trees overlays that introduce clocks
> providers.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/clk/clk-fixed-rate.c | 69 +++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index 2edb39342a02..e64ba2315880 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -15,6 +15,7 @@
> #include <linux/io.h>
> #include <linux/err.h>
> #include <linux/of.h>
> +#include <linux/platform_device.h>
>
> /*
> * DOC: basic fixed-rate clock that cannot gate
> @@ -160,15 +161,16 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
> /**
> * of_fixed_clk_setup() - Setup function for simple fixed rate clock
> */
> -void of_fixed_clk_setup(struct device_node *node)
> +struct clk *_of_fixed_clk_setup(struct device_node *node)
> {
> struct clk *clk;
> const char *clk_name = node->name;
> u32 rate;
> u32 accuracy = 0;
> + int ret;
>
> if (of_property_read_u32(node, "clock-frequency", &rate))
> - return;
> + return ERR_PTR(-EIO);
>
> of_property_read_u32(node, "clock-accuracy", &accuracy);
>
> @@ -176,9 +178,68 @@ void of_fixed_clk_setup(struct device_node *node)
>
> clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
> 0, rate, accuracy);
> - if (!IS_ERR(clk))
> - of_clk_add_provider(node, of_clk_src_simple_get, clk);
> + if (IS_ERR(clk))
> + return clk;
> +
> + ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> + if (ret) {
> + clk_unregister(clk);
> + return ERR_PTR(ret);
> + }
> +
> + return clk;
> +}
> +
> +void of_fixed_clk_setup(struct device_node *node)
> +{
> + _of_fixed_clk_setup(node);
> }
> EXPORT_SYMBOL_GPL(of_fixed_clk_setup);
> CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
> +
> +static int of_fixed_clk_remove(struct platform_device *pdev)
> +{
> + struct clk *clk = platform_get_drvdata(pdev);
> +
> + if (clk)
> + clk_unregister_fixed_rate(clk);
> +
> + return 0;
> +}
> +
> +static int of_fixed_clk_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> +
> + /*
> + * This function is not executed when of_fixed_clk_setup
> + * succeeded.
> + */
> +
> + clk = _of_fixed_clk_setup(pdev->dev.of_node);
> +
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + platform_set_drvdata(pdev, clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_fixed_clk_ids[] = {
> + { .compatible = "fixed-clock" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, of_fixed_clk_ids);
> +
> +static struct platform_driver of_fixed_clk_driver = {
> + .driver = {
> + .name = "of_fixed_clk",
> + .of_match_table = of_fixed_clk_ids,
> + },
> + .probe = of_fixed_clk_probe,
> + .remove = of_fixed_clk_remove,
> +};
> +
> +builtin_platform_driver(of_fixed_clk_driver);
> #endif
> --
> 2.8.1
>
Hi Alan
On Thu, Oct 18, 2018 at 9:21 PM Alan Tull <[email protected]> wrote:
>
> On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> <[email protected]> wrote:
>
> I've stumbled across a of_node_get/put imbalance that happens when the
> fixed rate clock is added and deleted using device tree. The cause is
> that this driver calls of_clk_add_provider() when probed, but doesn't
> call of_clk_del_provider() when removed.
>
> It looks like a lot of clock drivers share that issue:
>
> $ cd drivers/clk/
> $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> 131
>
> It should be a one line fix, but for many files.
>
> I'm not a clock subsystem expert, so please let me know whether I'm
> missing something here.
Not an expert either, but I believe that the affected drivers are much
less. We have to consider only the drivers that can be removed
git grep -l of_clk_add_provider | xargs grep -l platform_driver |
xargs grep -l remove | xargs grep -L of_clk_del_provider
drivers/clk/clk-fixed-factor.c
drivers/clk/clk-fixed-rate.c
drivers/clk/davinci/psc.c
drivers/clk/ti/adpll.c
drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c
Also there is something else that I do not undersand:
of_clk_add_hw_provider() is the same as of_clk_add_provider() ?
Maybe is a good idea to remove one of them and use the devm_of_add_hw_provider.
I can try to do that with coccinelle if you want.
Cheers
>
> Thanks,
> Alan
>
> >
> > Adds support for fixed-rate clock providers which have not been
> > enabled via of_clk_init().
> >
> > This is required by Device trees overlays that introduce clocks
> > providers.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > drivers/clk/clk-fixed-rate.c | 69 +++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 65 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> > index 2edb39342a02..e64ba2315880 100644
> > --- a/drivers/clk/clk-fixed-rate.c
> > +++ b/drivers/clk/clk-fixed-rate.c
> > @@ -15,6 +15,7 @@
> > #include <linux/io.h>
> > #include <linux/err.h>
> > #include <linux/of.h>
> > +#include <linux/platform_device.h>
> >
> > /*
> > * DOC: basic fixed-rate clock that cannot gate
> > @@ -160,15 +161,16 @@ EXPORT_SYMBOL_GPL(clk_hw_unregister_fixed_rate);
> > /**
> > * of_fixed_clk_setup() - Setup function for simple fixed rate clock
> > */
> > -void of_fixed_clk_setup(struct device_node *node)
> > +struct clk *_of_fixed_clk_setup(struct device_node *node)
> > {
> > struct clk *clk;
> > const char *clk_name = node->name;
> > u32 rate;
> > u32 accuracy = 0;
> > + int ret;
> >
> > if (of_property_read_u32(node, "clock-frequency", &rate))
> > - return;
> > + return ERR_PTR(-EIO);
> >
> > of_property_read_u32(node, "clock-accuracy", &accuracy);
> >
> > @@ -176,9 +178,68 @@ void of_fixed_clk_setup(struct device_node *node)
> >
> > clk = clk_register_fixed_rate_with_accuracy(NULL, clk_name, NULL,
> > 0, rate, accuracy);
> > - if (!IS_ERR(clk))
> > - of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > + if (IS_ERR(clk))
> > + return clk;
> > +
> > + ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> > + if (ret) {
> > + clk_unregister(clk);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + return clk;
> > +}
> > +
> > +void of_fixed_clk_setup(struct device_node *node)
> > +{
> > + _of_fixed_clk_setup(node);
> > }
> > EXPORT_SYMBOL_GPL(of_fixed_clk_setup);
> > CLK_OF_DECLARE(fixed_clk, "fixed-clock", of_fixed_clk_setup);
> > +
> > +static int of_fixed_clk_remove(struct platform_device *pdev)
> > +{
> > + struct clk *clk = platform_get_drvdata(pdev);
> > +
> > + if (clk)
> > + clk_unregister_fixed_rate(clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int of_fixed_clk_probe(struct platform_device *pdev)
> > +{
> > + struct clk *clk;
> > +
> > + /*
> > + * This function is not executed when of_fixed_clk_setup
> > + * succeeded.
> > + */
> > +
> > + clk = _of_fixed_clk_setup(pdev->dev.of_node);
> > +
> > + if (IS_ERR(clk))
> > + return PTR_ERR(clk);
> > +
> > + platform_set_drvdata(pdev, clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id of_fixed_clk_ids[] = {
> > + { .compatible = "fixed-clock" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_fixed_clk_ids);
> > +
> > +static struct platform_driver of_fixed_clk_driver = {
> > + .driver = {
> > + .name = "of_fixed_clk",
> > + .of_match_table = of_fixed_clk_ids,
> > + },
> > + .probe = of_fixed_clk_probe,
> > + .remove = of_fixed_clk_remove,
> > +};
> > +
> > +builtin_platform_driver(of_fixed_clk_driver);
> > #endif
> > --
> > 2.8.1
> >
--
Ricardo Ribalda
Quoting Ricardo Ribalda Delgado (2018-10-18 13:02:01)
> Hi Alan
>
>
> On Thu, Oct 18, 2018 at 9:21 PM Alan Tull <[email protected]> wrote:
> >
> > On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> > <[email protected]> wrote:
> >
> > I've stumbled across a of_node_get/put imbalance that happens when the
> > fixed rate clock is added and deleted using device tree. The cause is
> > that this driver calls of_clk_add_provider() when probed, but doesn't
> > call of_clk_del_provider() when removed.
> >
> > It looks like a lot of clock drivers share that issue:
> >
> > $ cd drivers/clk/
> > $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> > 131
> >
> > It should be a one line fix, but for many files.
> >
> > I'm not a clock subsystem expert, so please let me know whether I'm
> > missing something here.
>
> Not an expert either, but I believe that the affected drivers are much
> less. We have to consider only the drivers that can be removed
>
> git grep -l of_clk_add_provider | xargs grep -l platform_driver |
> xargs grep -l remove | xargs grep -L of_clk_del_provider
> drivers/clk/clk-fixed-factor.c
> drivers/clk/clk-fixed-rate.c
> drivers/clk/davinci/psc.c
> drivers/clk/ti/adpll.c
> drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c
>
>
> Also there is something else that I do not undersand:
> of_clk_add_hw_provider() is the same as of_clk_add_provider() ?
The difference is the hw provider API deals with struct clk_hw and the
other API deals with struct clk pointers. The preference is to use the
clk_hw based APIs.
Quoting Alan Tull (2018-10-18 12:20:58)
> On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> <[email protected]> wrote:
>
> I've stumbled across a of_node_get/put imbalance that happens when the
> fixed rate clock is added and deleted using device tree. The cause is
> that this driver calls of_clk_add_provider() when probed, but doesn't
> call of_clk_del_provider() when removed.
>
> It looks like a lot of clock drivers share that issue:
>
> $ cd drivers/clk/
> $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> 131
>
> It should be a one line fix, but for many files.
>
> I'm not a clock subsystem expert, so please let me know whether I'm
> missing something here.
>
Patches welcome. Please include Fixes: tags for backports. Probably
drivers don't care because clk devices are almost never removed. That
isn't to say it shouldn't be fixed, but just giving some background on
why nobody has fixed it.
On Thu, Oct 18, 2018 at 3:24 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Alan Tull (2018-10-18 12:20:58)
> > On Tue, Jul 5, 2016 at 11:45 AM Ricardo Ribalda Delgado
> > <[email protected]> wrote:
> >
> > I've stumbled across a of_node_get/put imbalance that happens when the
> > fixed rate clock is added and deleted using device tree. The cause is
> > that this driver calls of_clk_add_provider() when probed, but doesn't
> > call of_clk_del_provider() when removed.
> >
> > It looks like a lot of clock drivers share that issue:
> >
> > $ cd drivers/clk/
> > $ git grep -l of_clk_add_provider * | xargs grep -L of_clk_del_provider | wc -l
> > 131
> >
> > It should be a one line fix, but for many files.
> >
> > I'm not a clock subsystem expert, so please let me know whether I'm
> > missing something here.
> >
>
> Patches welcome. Please include Fixes: tags for backports. Probably
> drivers don't care because clk devices are almost never removed. That
> isn't to say it shouldn't be fixed, but just giving some background on
> why nobody has fixed it.
Thanks for the context. I ran into this while testing some devicetree
overlay code changes. My use is FPGAs where a clock may be added or
removed if the FPGA is reprogrammed.
Alan