[PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[Jean-Francois Moine]

At probe time, a clock device may not be ready when some other device
wants to use it. 

This patch lets the functions clk_get/devm_clk_get return a probe defer
when the clock is defined in the DT but not yet available.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 drivers/clk/clk.c    | 2 +-
 drivers/clk/clkdev.c | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a004769..582abc8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
 	struct of_clk_provider *provider;
-	struct clk *clk = ERR_PTR(-ENOENT);
+	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
 
 	/* Check if we have such a provider in our array */
 	mutex_lock(&of_clk_lock);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 442a313..f8b28d9 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 
 	if (dev) {
 		clk = of_clk_get_by_name(dev->of_node, con_id);
-		if (!IS_ERR(clk) && __clk_get(clk))
-			return clk;
+		if (!IS_ERR(clk)) {
+			if (__clk_get(clk))
+				return clk;
+		} else {
+			if (PTR_ERR(clk) == -EPROBE_DEFER)
+				return clk;
+		}
 	}
 
 	return clk_get_sys(dev_id, con_id);


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[Russell King - ARM Linux]

On Sat, Sep 21, 2013 at 12:00:06PM +0200, Jean-Francois Moine wrote:
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 442a313..f8b28d9 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
>  
>  	if (dev) {
>  		clk = of_clk_get_by_name(dev->of_node, con_id);
> -		if (!IS_ERR(clk) && __clk_get(clk))
> -			return clk;
> +		if (!IS_ERR(clk)) {
> +			if (__clk_get(clk))
> +				return clk;
> +		} else {
> +			if (PTR_ERR(clk) == -EPROBE_DEFER)
> +				return clk;
> +		}

Thanks for patching this file and bringing this code to my attention.
The original is fscked.  Calling __clk_get() outside of any lock is
a serious bug and leads to races.

Also, a failing __clk_get() probably should not drop through to
clk_get_sys(), but should instead do this:

		if (!IS_ERR(clk)) {
			if (!__clk_get(clk))
				clk = ERR_PTR(-ENOENT);
			return clk;
		}

since a _found_ clock but one which can't be used (due to the module
going away) is not a reason to drop through to clk_get_sys().

Which means your change should look like this:

		if (!IS_ERR(clk)) {
			if (!__clk_get(clk))
				clk = ERR_PTR(-ENOENT);
			return clk;
		}
		if (PTR_ERR(clk) == -EPROBE_DEFER)
			return clk;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[Russell King - ARM Linux]

On Sat, Sep 21, 2013 at 12:00:06PM +0200, Jean-Francois Moine wrote:
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a004769..582abc8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
>  	struct of_clk_provider *provider;
> -	struct clk *clk = ERR_PTR(-ENOENT);
> +	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
>  
>  	/* Check if we have such a provider in our array */
>  	mutex_lock(&of_clk_lock);
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 442a313..f8b28d9 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
>  
>  	if (dev) {
>  		clk = of_clk_get_by_name(dev->of_node, con_id);
> -		if (!IS_ERR(clk) && __clk_get(clk))
> -			return clk;
> +		if (!IS_ERR(clk)) {
> +			if (__clk_get(clk))
> +				return clk;
> +		} else {
> +			if (PTR_ERR(clk) == -EPROBE_DEFER)
> +				return clk;
> +		}

Another comment.  This is broken - it will never fall through if a
'dev' is given, because of_clk_get_by_name() will always return an
-EPROBE_DEFER error pointer in that case.

So this approach is unworkable.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[Russell King - ARM Linux]

On Sat, Sep 21, 2013 at 11:47:34AM +0100, Russell King - ARM Linux wrote:
> Another comment.  This is broken - it will never fall through if a
> 'dev' is given, because of_clk_get_by_name() will always return an
> -EPROBE_DEFER error pointer in that case.
> 
> So this approach is unworkable.

And another comment:

This whole approach is broken - while Mark is correct that we should
find a way to return -EPROBE_DEFER if a clock is specified, the solution
here does not allow that - it will return -EPROBE_DEFER whenever the
clock is not found _or_ is not specified.

This has the effect of making the external clock *non*optional for the
kirkwood-i2s driver, which is also completely broken.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] ASoC: kirkwood: clk: probe defer when clock not yet ready

[Jean-Francois Moine]

On Sat, 21 Sep 2013 11:47:34 +0100
Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

> On Sat, Sep 21, 2013 at 12:00:06PM +0200, Jean-Francois Moine wrote:
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index a004769..582abc8 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);
> >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> >  {
> >  	struct of_clk_provider *provider;
> > -	struct clk *clk = ERR_PTR(-ENOENT);
> > +	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
> >  
> >  	/* Check if we have such a provider in our array */
> >  	mutex_lock(&of_clk_lock);
> > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> > index 442a313..f8b28d9 100644
> > --- a/drivers/clk/clkdev.c
> > +++ b/drivers/clk/clkdev.c
> > @@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
> >  
> >  	if (dev) {
> >  		clk = of_clk_get_by_name(dev->of_node, con_id);
> > -		if (!IS_ERR(clk) && __clk_get(clk))
> > -			return clk;
> > +		if (!IS_ERR(clk)) {
> > +			if (__clk_get(clk))
> > +				return clk;
> > +		} else {
> > +			if (PTR_ERR(clk) == -EPROBE_DEFER)
> > +				return clk;
> > +		}
> 
> Another comment.  This is broken - it will never fall through if a
> 'dev' is given, because of_clk_get_by_name() will always return an
> -EPROBE_DEFER error pointer in that case.
> 
> So this approach is unworkable.

> And another comment:
> 
> This whole approach is broken - while Mark is correct that we should
> find a way to return -EPROBE_DEFER if a clock is specified, the solution
> here does not allow that - it will return -EPROBE_DEFER whenever the
> clock is not found _or_ is not specified.
> 
> This has the effect of making the external clock *non*optional for the
> kirkwood-i2s driver, which is also completely broken.

It works fine for me (I tried both cases, with and without the external
clock).

The scheme is as follows:

- kirkwood-i2s kirkwood_i2s_dev_probe() calls:

	priv->extclk = &pdev->dev, "extclk");

- devm_clk_get() calls:

	clk = clk_get(dev, id);

- clk_get() does:

	if (dev)			// this is the case
		clk = of_clk_get_by_name(dev->of_node, con_id);

- of_clk_get_by_name() does:

	if (name)			// this is the case
		index = of_property_match_string(np, "clock-names", name);

  Here, if the external clock is declared in the DT, index is >= 0.
  If there is no externel clock in the DT, index is < 0.

	clk = of_clk_get(np, index);
 	if (!IS_ERR(clk))
		break;
	else if (name && index >= 0) {
		pr_err("ERROR: could not get clock %s:%s(%i)\n",
 			np->full_name, name ? name : "", index);  
 		return clk;
 	}

- If the index is < 0, of_clk_get() immediately returns -EINVAL.

  So, kirkwood-i2s knows there is no external clock. DONE.

- otherwise, if the index is >= 0 (the external clock is declared in
  the DT), of_clk_get() gets the phandle (of_parse_phandle_with_args())
  and gets the clock (of_clk_get_from_provider()).

  This last function returns either the clock (device initialized),
  a -EPROBE_DEFER error (clock not yet initialized) or some other
  error (bad DT or clock error).

  So, kirkwood-i2s knows there is a declared external clock and it acts
  according to the return code (accept, defer or ignore). DONE.

Did I miss something?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html