Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456718 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from  returning NULL Stephen Boyd <sboyd@codeaurora.org> - 2016-08-04 23:00 +0200
    Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from  returning NULL Sylwester Nawrocki <s.nawrocki@samsung.com> - 2016-08-05 10:40 +0200
    Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from  returning NULL Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-07 19:10 +0200
    Re: [PATCH] clk: prevent __of_clk_get_hw_from_provider() from  returning NULL Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-08-10 20:10 +0200

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

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

[toc] | [next] | [standalone]

#1456949

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

[toc] | [prev] | [next] | [standalone]

#1457479

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

[toc] | [prev] | [next] | [standalone]

#1459311

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web