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 <sam...@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