On Sat, Mar 04, 2023 at 01:54:12PM -0600, Samuel Holland wrote: > 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.
Yes, there are too many. clk_valid should check both. Reviewed-by: Michal Suchánek <msucha...@suse.de> Thanks Michal