[PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Masahiro Yamada]

The .get(_hw) callback of an OF clock provider can return a NULL
pointer in some cases.

For example, of_clk_src_onecell_get() returns NULL for index 1 of a
sparse array of clocks like follows:

  clk_num == 3
  idx 0: UART clk
  idx 1: NULL (no clk is allocated)
  idx 2: I2C clk

In such cases, clk_get() successfully returns NULL.

A problem is that most drivers only check IS_ERR(), like follows:

  clk = devm_clk_get(dev, NULL);
  if (IS_ERR(clk))
          return PTR_ERR(clk);

It carries on moving forward and will probably be hit by a different
error check with a different error message.

Let's make __of_clk_get_hw_from_provider() return ERR_PTR(-ENOENT)
if the .get(_hw) returns NULL.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 953643f..484acc2 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3117,7 +3117,7 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
 			hw = ERR_CAST(clk);
 	}
 
-	return hw;
+	return hw ?: ERR_PTR(-ENOENT);
 }
 
 struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
-- 
1.9.1

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Stephen Boyd]

On 07/19, Masahiro Yamada wrote:
> The .get(_hw) callback of an OF clock provider can return a NULL
> pointer in some cases.
> 
> For example, of_clk_src_onecell_get() returns NULL for index 1 of a
> sparse array of clocks like follows:
> 
>   clk_num == 3
>   idx 0: UART clk
>   idx 1: NULL (no clk is allocated)
>   idx 2: I2C clk
> 
> In such cases, clk_get() successfully returns NULL.
> 
> A problem is that most drivers only check IS_ERR(), like follows:
> 
>   clk = devm_clk_get(dev, NULL);
>   if (IS_ERR(clk))
>           return PTR_ERR(clk);
> 
> It carries on moving forward and will probably be hit by a different
> error check with a different error message.

NULL is a valid clk pointer, so we can't really do anything here
besides rely on driver authors to do the right thing.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Sylwester Nawrocki]

On 08/04/2016 10:57 PM, Stephen Boyd wrote:
> On 07/19, Masahiro Yamada wrote:
>> > The .get(_hw) callback of an OF clock provider can return a NULL
>> > pointer in some cases.
>> > 
>> > For example, of_clk_src_onecell_get() returns NULL for index 1 of a
>> > sparse array of clocks like follows:
>> > 
>> >   clk_num == 3
>> >   idx 0: UART clk
>> >   idx 1: NULL (no clk is allocated)
>> >   idx 2: I2C clk
>> > 
>> > In such cases, clk_get() successfully returns NULL.

I remember running into same issue before, we have addressed
it by initializing the array of clocks with some errno value,
e.g ERR_PTR(-ENOENT), so there is no chance to get NULL from
the array - either a valid clk pointer or an ERR_PTR() value.

--
Thanks,
Sylwester

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Masahiro Yamada]

Hi Stephen,


2016-08-05 5:57 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 07/19, Masahiro Yamada wrote:
>> The .get(_hw) callback of an OF clock provider can return a NULL
>> pointer in some cases.
>>
>> For example, of_clk_src_onecell_get() returns NULL for index 1 of a
>> sparse array of clocks like follows:
>>
>>   clk_num == 3
>>   idx 0: UART clk
>>   idx 1: NULL (no clk is allocated)
>>   idx 2: I2C clk
>>
>> In such cases, clk_get() successfully returns NULL.
>>
>> A problem is that most drivers only check IS_ERR(), like follows:
>>
>>   clk = devm_clk_get(dev, NULL);
>>   if (IS_ERR(clk))
>>           return PTR_ERR(clk);
>>
>> It carries on moving forward and will probably be hit by a different
>> error check with a different error message.
>
> NULL is a valid clk pointer, so we can't really do anything here
> besides rely on driver authors to do the right thing.

Please let me clearer, just in case.
The "driver" means clk provider, not consumer.  Correct?

So, clock providers should be responsible for not returning NULL,
for example, by filling blank entries with ERR_PTR(-ENOENT).



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Masahiro Yamada]

2016-08-05 5:57 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 07/19, Masahiro Yamada wrote:
>> The .get(_hw) callback of an OF clock provider can return a NULL
>> pointer in some cases.
>>
>> For example, of_clk_src_onecell_get() returns NULL for index 1 of a
>> sparse array of clocks like follows:
>>
>>   clk_num == 3
>>   idx 0: UART clk
>>   idx 1: NULL (no clk is allocated)
>>   idx 2: I2C clk
>>
>> In such cases, clk_get() successfully returns NULL.
>>
>> A problem is that most drivers only check IS_ERR(), like follows:
>>
>>   clk = devm_clk_get(dev, NULL);
>>   if (IS_ERR(clk))
>>           return PTR_ERR(clk);
>>
>> It carries on moving forward and will probably be hit by a different
>> error check with a different error message.
>
> NULL is a valid clk pointer, so we can't really do anything here
> besides rely on driver authors to do the right thing.


I still do not understand this.


I think clk_get() should return > 0 pointer on success,
error-pointer on failure.

I have no idea when NULL is useful as a return value of clk_get().




-- 
Best Regards
Masahiro Yamada

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Stephen Boyd]

On 08/10, Masahiro Yamada wrote:
> 2016-08-05 5:57 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> > On 07/19, Masahiro Yamada wrote:
> >> The .get(_hw) callback of an OF clock provider can return a NULL
> >> pointer in some cases.
> >>
> >> For example, of_clk_src_onecell_get() returns NULL for index 1 of a
> >> sparse array of clocks like follows:
> >>
> >>   clk_num == 3
> >>   idx 0: UART clk
> >>   idx 1: NULL (no clk is allocated)
> >>   idx 2: I2C clk
> >>
> >> In such cases, clk_get() successfully returns NULL.
> >>
> >> A problem is that most drivers only check IS_ERR(), like follows:
> >>
> >>   clk = devm_clk_get(dev, NULL);
> >>   if (IS_ERR(clk))
> >>           return PTR_ERR(clk);
> >>
> >> It carries on moving forward and will probably be hit by a different
> >> error check with a different error message.
> >
> > NULL is a valid clk pointer, so we can't really do anything here
> > besides rely on driver authors to do the right thing.
> 
> 
> I still do not understand this.
> 
> 
> I think clk_get() should return > 0 pointer on success,
> error-pointer on failure.

Russell King has repeatedly stated that NULL is a valid return
value from clk_get(). I'm sure we can find numerous such
statements on the arm mailing list.

> 
> I have no idea when NULL is useful as a return value of clk_get().
> 

Perhaps the provider wants to avoid allocating anything for some
clk id and have the clk consumer API do nothing in this case?
Consumers can be unaware of this fact by having the provider
return NULL so things like clk_prepare/enable become nops. Of
course, we can't make things like clk_get_rate() useful in this
case, but at least we can have on/off simplified.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Masahiro Yamada]

Hi.

(+CC Russell, perhaps he may have some comments on this topic.)

2016-08-11 8:23 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 08/10, Masahiro Yamada wrote:
>> 2016-08-05 5:57 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
>> > On 07/19, Masahiro Yamada wrote:
>> >> The .get(_hw) callback of an OF clock provider can return a NULL
>> >> pointer in some cases.
>> >>
>> >> For example, of_clk_src_onecell_get() returns NULL for index 1 of a
>> >> sparse array of clocks like follows:
>> >>
>> >>   clk_num == 3
>> >>   idx 0: UART clk
>> >>   idx 1: NULL (no clk is allocated)
>> >>   idx 2: I2C clk
>> >>
>> >> In such cases, clk_get() successfully returns NULL.
>> >>
>> >> A problem is that most drivers only check IS_ERR(), like follows:
>> >>
>> >>   clk = devm_clk_get(dev, NULL);
>> >>   if (IS_ERR(clk))
>> >>           return PTR_ERR(clk);
>> >>
>> >> It carries on moving forward and will probably be hit by a different
>> >> error check with a different error message.
>> >
>> > NULL is a valid clk pointer, so we can't really do anything here
>> > besides rely on driver authors to do the right thing.
>>
>>
>> I still do not understand this.
>>
>>
>> I think clk_get() should return > 0 pointer on success,
>> error-pointer on failure.
>
> Russell King has repeatedly stated that NULL is a valid return
> value from clk_get(). I'm sure we can find numerous such
> statements on the arm mailing list.
>
>>
>> I have no idea when NULL is useful as a return value of clk_get().
>>
>
> Perhaps the provider wants to avoid allocating anything for some
> clk id and have the clk consumer API do nothing in this case?

Hmm.

I was thinking in which case we want to do this.
Perhaps, when we add the "clocks" property to DT first,
but clock driver is not implemented yet.  So clk provider
just want to return NULL to make the consumer to skip the clk set-up ??


Another possibility:
I noticed we have a stub in include/linux/clk.h,
which just returns NULL if CONFIG_HAVE_CLK is not defined.



> Consumers can be unaware of this fact by having the provider
> return NULL so things like clk_prepare/enable become nops. Of
> course, we can't make things like clk_get_rate() useful in this
> case, but at least we can have on/off simplified.

At least, this seems useful for architectures without CONFIG_HAVE_CLK.


Another case where it might be useful is,
if "clocks" property is missing in DT,
clk_get() can return NULL to make the clock optional.
Then, clk consumer can skip clk_prepare_enable() and continue probing.

But, actually, clk_get returns ERR_PTR(-ENOENT) if "clocks" property
is missing.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from returning NULL

[Russell King - ARM Linux]

On Fri, Aug 12, 2016 at 03:59:51PM +0900, Masahiro Yamada wrote:
> But, actually, clk_get returns ERR_PTR(-ENOENT) if "clocks" property
> is missing.

Probably because we don't know if that's a case of a partially
converted platform (which doesn't yet have its clk stuff in DT),
so we need to fall back to the table-driven code, or if it really
has no clocks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.