Re: [PATCH 1/6] clk: Handle error pointers in clk_valid()
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
> ---
>
> 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
Thanks
Michal
Re: [PATCH 1/6] clk: Handle error pointers in clk_valid()
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
---
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
Re: [PATCH 1/6] clk: Handle error pointers in clk_valid()
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
> > > ---
> > >
> > > 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.
> 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.
Thanks
Michal
Re: [PATCH 1/6] clk: Handle error pointers in clk_valid()
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
---
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
fine to pass NULL or uninitialized clocks to other clock functions, but it's
not OK to ignore errors.
--Sean
Re: [PATCH 1/6] clk: Handle error pointers in clk_valid()
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
> ---
>
> 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.
Thanks
Michal
[PATCH 1/6] clk: Handle error pointers in clk_valid()
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
---
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);
--
2.39.2
