[PATCH 1/6] clk: Handle error pointers in clk_valid()

[Samuel Holland]

On 2/20/23 13:42, Michal Suchánek wrote:
> On Mon, Feb 20, 2023 at 10:57:17AM -0500, Sean Anderson wrote:
>>
>> On 2/20/23 05:46, Michal Suchánek wrote:
>>> On Sun, Feb 19, 2023 at 11:59:34PM -0600, Samuel Holland wrote:
>>>> Some clk uclass functions, such as devm_clk_get() and clk_get_parent(),
>>>> return error pointers. clk_valid() should not consider these pointers
>>>> to be valid.
>>>>
>>>> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
>>>> Signed-off-by: Samuel Holland <samuel at sholland.org>
>>>> ---
>>>>
>>>>   include/clk.h | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/clk.h b/include/clk.h
>>>> index d91285235f7..4acb878ec6d 100644
>>>> --- a/include/clk.h
>>>> +++ b/include/clk.h
>>>> @@ -671,7 +671,7 @@ static inline bool clk_dev_binded(struct clk *clk)
>>>>    */
>>>>   static inline bool clk_valid(struct clk *clk)
>>>>   {
>>>> -	return clk && !!clk->dev;
>>>> +	return clk && !IS_ERR(clk) && !!clk->dev;
>>>>   }
>>>>   int soc_clk_dump(void);
>>>
>>> Maybe it would be better to avoid the error pointers instead, there
>>> should not be that many places where they are returned.
>>
>> This not quite the right way to phrase this. Instead, I would ask: where
>> are places where the return value is not checked correctly? It's perfectly
> 
> Pretty much everywhere where clk_get_parent is used outside of core code
> absolutely no error checking is done.

There are 105 uses of ERR_PTR in drivers/clk, mostly in CCF clock
registration functions. There is also devm_clk_get() with about 30
callers. Those mostly seem to have reasonable error checking; the error
pointer ends up as the driver probe function's return value via PTR_ERR.

>> fine to pass NULL or uninitialized clocks to other clock functions, but it's
>> not OK to ignore errors.
> 
> Is there some documentation on what is the distinction, specifically?
> 
> If there isn't it's meaningless and NULL can be returned on error
> simplifying the code, thinking about the code, debugging, pretty much
> everything.

What should I do for v2? Keep this patch? Convert clk_get_parent() to
return NULL instead of ERR_PTR, and hope nobody uses clk_valid() on the
pointer returned from devm_clk_get()?

Regards,
Samuel