[PATCH 0/6] clk: uclass fixes and improvements

[PATCH 0/6] clk: uclass fixes and improvements

[Samuel Holland]

This series fixes a few issues found while writing a clk driver for a
new SoC (Bouffalo Lab BL808), and adds the new functionality needed to
implement a hierarchy of clocks in a single device (without the CCF).
The .get_parent hook will be useful on other platforms as well; I plan
to use it to implement PLL rate setting on sunxi.


Samuel Holland (6):
  clk: Handle error pointers in clk_valid()
  clk: Fix error handling in clk_get_rate()
  clk: Fix error handling in clk_get_parent()
  clk: Fix rate caching in clk_get_parent_rate()
  clk: Remove an unneeded check from clk_get_parent_rate()
  clk: Add a .get_parent operation

 drivers/clk/clk-uclass.c | 35 ++++++++++++++++-------------------
 include/clk-uclass.h     |  2 ++
 include/clk.h            |  2 +-
 3 files changed, 19 insertions(+), 20 deletions(-)

-- 
2.39.2

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

[Samuel Holland]

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@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);
-- 
2.39.2

[PATCH 2/6] clk: Fix error handling in clk_get_rate()

[Samuel Holland]

log_ret() cannot work with unsigned values, and the assignment to 'ret'
incorrectly truncates the rate from long to int.

Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/clk/clk-uclass.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a261..78299dbceb2 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
 ulong clk_get_rate(struct clk *clk)
 {
 	const struct clk_ops *ops;
-	int ret;
 
 	debug("%s(clk=%p)\n", __func__, clk);
 	if (!clk_valid(clk))
@@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
 	if (!ops->get_rate)
 		return -ENOSYS;
 
-	ret = ops->get_rate(clk);
-	if (ret)
-		return log_ret(ret);
-
-	return 0;
+	return ops->get_rate(clk);
 }
 
 struct clk *clk_get_parent(struct clk *clk)
-- 
2.39.2

[PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Samuel Holland]

Do not return both NULL and error pointers. The function is only
documented as returning error pointers.

Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/clk/clk-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 78299dbceb2..5bce976b060 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
 
 	debug("%s(clk=%p)\n", __func__, clk);
 	if (!clk_valid(clk))
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	pdev = dev_get_parent(clk->dev);
 	if (!pdev)
-- 
2.39.2

[PATCH 4/6] clk: Fix rate caching in clk_get_parent_rate()

[Samuel Holland]

clk_get_rate() can return an error value. Recompute the rate if the
cached value is an error value.

Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/clk/clk-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 5bce976b060..9d052e5a814 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk)
 		return -ENOSYS;
 
 	/* Read the 'rate' if not already set or if proper flag set*/
-	if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
+	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
+	    pclk->flags & CLK_GET_RATE_NOCACHE)
 		pclk->rate = clk_get_rate(pclk);
 
 	return pclk->rate;
-- 
2.39.2

[PATCH 5/6] clk: Remove an unneeded check from clk_get_parent_rate()

[Samuel Holland]

There is no need to check the parent clock's ops. The following call to
clk_get_rate() does that already.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/clk/clk-uclass.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 9d052e5a814..53cfd819779 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
 
 ulong clk_get_parent_rate(struct clk *clk)
 {
-	const struct clk_ops *ops;
 	struct clk *pclk;
 
 	debug("%s(clk=%p)\n", __func__, clk);
@@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk)
 	if (IS_ERR(pclk))
 		return -ENODEV;
 
-	ops = clk_dev_ops(pclk->dev);
-	if (!ops->get_rate)
-		return -ENOSYS;
-
 	/* Read the 'rate' if not already set or if proper flag set*/
 	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
 	    pclk->flags & CLK_GET_RATE_NOCACHE)
-- 
2.39.2

[PATCH 6/6] clk: Add a .get_parent operation

[Samuel Holland]

This allows clk_get_parent() to work with non-CCF clock drivers.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/clk/clk-uclass.c | 18 ++++++++++++------
 include/clk-uclass.h     |  2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 53cfd819779..d0f8906cd60 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
 
 struct clk *clk_get_parent(struct clk *clk)
 {
+	const struct clk_ops *ops;
 	struct udevice *pdev;
 	struct clk *pclk;
 
@@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk)
 	if (!clk_valid(clk))
 		return ERR_PTR(-ENODEV);
 
-	pdev = dev_get_parent(clk->dev);
-	if (!pdev)
-		return ERR_PTR(-ENODEV);
-	pclk = dev_get_clk_ptr(pdev);
-	if (!pclk)
-		return ERR_PTR(-ENODEV);
+	ops = clk_dev_ops(clk->dev);
+	if (ops->get_parent) {
+		pclk = ops->get_parent(clk);
+	} else {
+		pdev = dev_get_parent(clk->dev);
+		if (!pdev)
+			return ERR_PTR(-ENODEV);
+		pclk = dev_get_clk_ptr(pdev);
+		if (!pclk)
+			return ERR_PTR(-ENODEV);
+	}
 
 	return pclk;
 }
diff --git a/include/clk-uclass.h b/include/clk-uclass.h
index 65ebff9ed27..4d616720865 100644
--- a/include/clk-uclass.h
+++ b/include/clk-uclass.h
@@ -22,6 +22,7 @@ struct ofnode_phandle_args;
  * @round_rate: Adjust a rate to the exact rate a clock can provide.
  * @get_rate: Get current clock rate.
  * @set_rate: Set current clock rate.
+ * @get_parent: Get current clock parent
  * @set_parent: Set current clock parent
  * @enable: Enable a clock.
  * @disable: Disable a clock.
@@ -36,6 +37,7 @@ struct clk_ops {
 	ulong (*round_rate)(struct clk *clk, ulong rate);
 	ulong (*get_rate)(struct clk *clk);
 	ulong (*set_rate)(struct clk *clk, ulong rate);
+	struct clk *(*get_parent)(struct clk *clk);
 	int (*set_parent)(struct clk *clk, struct clk *parent);
 	int (*enable)(struct clk *clk);
 	int (*disable)(struct clk *clk);
-- 
2.39.2

Re: [PATCH 2/6] clk: Fix error handling in clk_get_rate()

[Michal Suchánek]

Hello,

On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
> log_ret() cannot work with unsigned values, and the assignment to 'ret'
> incorrectly truncates the rate from long to int.
> 
> Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/clk-uclass.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index dc3e9d6a261..78299dbceb2 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
>  ulong clk_get_rate(struct clk *clk)
>  {
>  	const struct clk_ops *ops;
> -	int ret;
>  
>  	debug("%s(clk=%p)\n", __func__, clk);
>  	if (!clk_valid(clk))
> @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
>  	if (!ops->get_rate)
>  		return -ENOSYS;
>  
> -	ret = ops->get_rate(clk);
> -	if (ret)
> -		return log_ret(ret);
> -
> -	return 0;
> +	return ops->get_rate(clk);
>  }

This is generally poor design of the clock stuff in u-boot.

It returns -ERROR for many error conditions, but also 0 for other error
conditions.

Some error handling checks for 0, some for errval, some casts to int and
checks for <= 0.

I think that using -ERROR for clocks does not make much sense in u-boot.

Even in the kernel the errval checks are pretty much limited to places
where integers are used to store page frame numbers or pointers, that is
errptr stored in an integer. For adresses it is pretty easy to make sure
that the last page is not mapped making the error pointers invalid
(although bugs in that part happened too).

For clocks no such guarantee exists. The only apparently invalid clock
is 0, and the correct fix is to fix up the clock code to return 0 on
error, always. It's a lot of code to fix, though.

If you do not want to fix everything then the correct thing to do is
make ret ulong, and check for errval *and* 0.

There is not much point in returning detailed error codes in u-boot,
anyway. It's not like there is some userspace that could interpret them.
Most errors are logged when they happen if ever, and callers only check
if error happened or not.

Thanks

Michal

Re: [PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Michal Suchánek]

On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
> Do not return both NULL and error pointers. The function is only
> documented as returning error pointers.
> 
> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/clk-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 78299dbceb2..5bce976b060 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
>  
>  	debug("%s(clk=%p)\n", __func__, clk);
>  	if (!clk_valid(clk))
> -		return NULL;
> +		return ERR_PTR(-ENODEV);
>  
>  	pdev = dev_get_parent(clk->dev);
>  	if (!pdev)

Do we really need this?

Who cares about the distinction?

Just returning NULL on any error makes the code so much simpler and
easier to understand.

Thanks

Michal

Re: [PATCH 4/6] clk: Fix rate caching in clk_get_parent_rate()

[Michal Suchánek]

On Sun, Feb 19, 2023 at 11:59:37PM -0600, Samuel Holland wrote:
> clk_get_rate() can return an error value. Recompute the rate if the
> cached value is an error value.
> 
> Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/clk-uclass.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 5bce976b060..9d052e5a814 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk)
>  		return -ENOSYS;
>  
>  	/* Read the 'rate' if not already set or if proper flag set*/
> -	if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
> +	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
> +	    pclk->flags & CLK_GET_RATE_NOCACHE)
>  		pclk->rate = clk_get_rate(pclk);
>  
>  	return pclk->rate;


With the current code that randomly returns one or the other this is the
correct thing to do.

Reviewed-by: Michal Suchánek <msuchanek@suse.de>

Thanks

Michal

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

[Michal Suchánek]

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

Thanks

Michal

Re: [PATCH 5/6] clk: Remove an unneeded check from clk_get_parent_rate()

[Michal Suchánek]

On Sun, Feb 19, 2023 at 11:59:38PM -0600, Samuel Holland wrote:
> There is no need to check the parent clock's ops. The following call to
> clk_get_rate() does that already.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/clk-uclass.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 9d052e5a814..53cfd819779 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
>  
>  ulong clk_get_parent_rate(struct clk *clk)
>  {
> -	const struct clk_ops *ops;
>  	struct clk *pclk;
>  
>  	debug("%s(clk=%p)\n", __func__, clk);
> @@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk)
>  	if (IS_ERR(pclk))
>  		return -ENODEV;
>  
> -	ops = clk_dev_ops(pclk->dev);
> -	if (!ops->get_rate)
> -		return -ENOSYS;
> -
>  	/* Read the 'rate' if not already set or if proper flag set*/
>  	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
>  	    pclk->flags & CLK_GET_RATE_NOCACHE)

Reviewed-by: Michal Suchánek <msuchanek@suse.de>

Thanks

Michal

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

[Sean Anderson]

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@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
fine to pass NULL or uninitialized clocks to other clock functions, but it's
not OK to ignore errors.

--Sean

Re: [PATCH 2/6] clk: Fix error handling in clk_get_rate()

[Sean Anderson]

On 2/20/23 05:37, Michal Suchánek wrote:
> Hello,
> 
> On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
>> log_ret() cannot work with unsigned values, and the assignment to 'ret'
>> incorrectly truncates the rate from long to int.
>>
>> Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>   drivers/clk/clk-uclass.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index dc3e9d6a261..78299dbceb2 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
>>   ulong clk_get_rate(struct clk *clk)
>>   {
>>   	const struct clk_ops *ops;
>> -	int ret;
>>   
>>   	debug("%s(clk=%p)\n", __func__, clk);
>>   	if (!clk_valid(clk))
>> @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
>>   	if (!ops->get_rate)
>>   		return -ENOSYS;
>>   
>> -	ret = ops->get_rate(clk);
>> -	if (ret)
>> -		return log_ret(ret);
>> -
>> -	return 0;
>> +	return ops->get_rate(clk);
>>   }
> 
> This is generally poor design of the clock stuff in u-boot.
> 
> It returns -ERROR for many error conditions, but also 0 for other error
> conditions.
> 
> Some error handling checks for 0, some for errval, some casts to int and
> checks for <= 0.
> 
> I think that using -ERROR for clocks does not make much sense in u-boot.
> 
> Even in the kernel the errval checks are pretty much limited to places
> where integers are used to store page frame numbers or pointers, that is
> errptr stored in an integer. For adresses it is pretty easy to make sure
> that the last page is not mapped making the error pointers invalid
> (although bugs in that part happened too).
> 
> For clocks no such guarantee exists. The only apparently invalid clock
> is 0, and the correct fix is to fix up the clock code to return 0 on
> error, always. It's a lot of code to fix, though.
> 
> If you do not want to fix everything then the correct thing to do is
> make ret ulong, and check for errval *and* 0.
> 
> There is not much point in returning detailed error codes in u-boot,
> anyway. It's not like there is some userspace that could interpret them.
> Most errors are logged when they happen if ever, and callers only check
> if error happened or not.
> 
> Thanks
> 
> Michal

clk_get_parent is the only place where we return a clock pointer directly.
Everywhere else, we have an integer return we can use. This function is
different of course because CCF is broken and assumes there is only one
canonical struct clk for a logical clock. So it can't initialize a caller-passed
struct clk, but instead has to return the one true struct clk.

I agree with Michal here. The signature should really be

int clk_get_parent(struct clk *child, struct clk *parent)

but for now there is no point confusing the rest of the clock subsystem to make
it work with error pointers.

--Sean

Re: [PATCH 4/6] clk: Fix rate caching in clk_get_parent_rate()

[Sean Anderson]

On 2/20/23 00:59, Samuel Holland wrote:
> clk_get_rate() can return an error value. Recompute the rate if the
> cached value is an error value.
> 
> Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations")
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>   drivers/clk/clk-uclass.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 5bce976b060..9d052e5a814 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk)
>   		return -ENOSYS;
>   
>   	/* Read the 'rate' if not already set or if proper flag set*/
> -	if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
> +	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
> +	    pclk->flags & CLK_GET_RATE_NOCACHE)
>   		pclk->rate = clk_get_rate(pclk);
>   
>   	return pclk->rate;

The correct fix here is

	/* Read the 'rate' if not already set or if proper flag set*/
	if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) {
		rate = clk_get_rate(pclk);
		if (!IS_ERR_VALUE(rate))
			pclk->rate = rate;
	}

	return rate;

No point caching errors.

--Sean

Re: [PATCH 5/6] clk: Remove an unneeded check from clk_get_parent_rate()

[Sean Anderson]

On 2/20/23 00:59, Samuel Holland wrote:
> There is no need to check the parent clock's ops. The following call to
> clk_get_rate() does that already.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>   drivers/clk/clk-uclass.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 9d052e5a814..53cfd819779 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -504,7 +504,6 @@ struct clk *clk_get_parent(struct clk *clk)
>   
>   ulong clk_get_parent_rate(struct clk *clk)
>   {
> -	const struct clk_ops *ops;
>   	struct clk *pclk;
>   
>   	debug("%s(clk=%p)\n", __func__, clk);
> @@ -515,10 +514,6 @@ ulong clk_get_parent_rate(struct clk *clk)
>   	if (IS_ERR(pclk))
>   		return -ENODEV;
>   
> -	ops = clk_dev_ops(pclk->dev);
> -	if (!ops->get_rate)
> -		return -ENOSYS;
> -
>   	/* Read the 'rate' if not already set or if proper flag set*/
>   	if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
>   	    pclk->flags & CLK_GET_RATE_NOCACHE)

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [PATCH 6/6] clk: Add a .get_parent operation

[Sean Anderson]

On 2/20/23 00:59, Samuel Holland wrote:
> This allows clk_get_parent() to work with non-CCF clock drivers.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>   drivers/clk/clk-uclass.c | 18 ++++++++++++------
>   include/clk-uclass.h     |  2 ++
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 53cfd819779..d0f8906cd60 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
>   
>   struct clk *clk_get_parent(struct clk *clk)
>   {
> +	const struct clk_ops *ops;
>   	struct udevice *pdev;
>   	struct clk *pclk;
>   
> @@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk)
>   	if (!clk_valid(clk))
>   		return ERR_PTR(-ENODEV);
>   
> -	pdev = dev_get_parent(clk->dev);
> -	if (!pdev)
> -		return ERR_PTR(-ENODEV);
> -	pclk = dev_get_clk_ptr(pdev);
> -	if (!pclk)
> -		return ERR_PTR(-ENODEV);
> +	ops = clk_dev_ops(clk->dev);
> +	if (ops->get_parent) {
> +		pclk = ops->get_parent(clk);
> +	} else {
> +		pdev = dev_get_parent(clk->dev);
> +		if (!pdev)
> +			return ERR_PTR(-ENODEV);
> +		pclk = dev_get_clk_ptr(pdev);
> +		if (!pclk)
> +			return ERR_PTR(-ENODEV);
> +	}
>   
>   	return pclk;
>   }
> diff --git a/include/clk-uclass.h b/include/clk-uclass.h
> index 65ebff9ed27..4d616720865 100644
> --- a/include/clk-uclass.h
> +++ b/include/clk-uclass.h
> @@ -22,6 +22,7 @@ struct ofnode_phandle_args;
>    * @round_rate: Adjust a rate to the exact rate a clock can provide.
>    * @get_rate: Get current clock rate.
>    * @set_rate: Set current clock rate.
> + * @get_parent: Get current clock parent
>    * @set_parent: Set current clock parent
>    * @enable: Enable a clock.
>    * @disable: Disable a clock.
> @@ -36,6 +37,7 @@ struct clk_ops {
>   	ulong (*round_rate)(struct clk *clk, ulong rate);
>   	ulong (*get_rate)(struct clk *clk);
>   	ulong (*set_rate)(struct clk *clk, ulong rate);
> +	struct clk *(*get_parent)(struct clk *clk);
>   	int (*set_parent)(struct clk *clk, struct clk *parent);
>   	int (*enable)(struct clk *clk);
>   	int (*disable)(struct clk *clk);

Missing documentation.

--Sean

Re: [PATCH 6/6] clk: Add a .get_parent operation

[Simon Glass]

Hi Samuel,

On Sun, 19 Feb 2023 at 23:00, Samuel Holland <samuel@sholland.org> wrote:
>
> This allows clk_get_parent() to work with non-CCF clock drivers.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>
>  drivers/clk/clk-uclass.c | 18 ++++++++++++------
>  include/clk-uclass.h     |  2 ++
>  2 files changed, 14 insertions(+), 6 deletions(-)
>

Please can you add a test for this and do full comments for the
function you added?

> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 53cfd819779..d0f8906cd60 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -485,6 +485,7 @@ ulong clk_get_rate(struct clk *clk)
>
>  struct clk *clk_get_parent(struct clk *clk)
>  {
> +       const struct clk_ops *ops;
>         struct udevice *pdev;
>         struct clk *pclk;
>
> @@ -492,12 +493,17 @@ struct clk *clk_get_parent(struct clk *clk)
>         if (!clk_valid(clk))
>                 return ERR_PTR(-ENODEV);
>
> -       pdev = dev_get_parent(clk->dev);
> -       if (!pdev)
> -               return ERR_PTR(-ENODEV);
> -       pclk = dev_get_clk_ptr(pdev);
> -       if (!pclk)
> -               return ERR_PTR(-ENODEV);
> +       ops = clk_dev_ops(clk->dev);
> +       if (ops->get_parent) {
> +               pclk = ops->get_parent(clk);
> +       } else {
> +               pdev = dev_get_parent(clk->dev);
> +               if (!pdev)
> +                       return ERR_PTR(-ENODEV);
> +               pclk = dev_get_clk_ptr(pdev);
> +               if (!pclk)
> +                       return ERR_PTR(-ENODEV);
> +       }
>
>         return pclk;
>  }
> diff --git a/include/clk-uclass.h b/include/clk-uclass.h
> index 65ebff9ed27..4d616720865 100644
> --- a/include/clk-uclass.h
> +++ b/include/clk-uclass.h
> @@ -22,6 +22,7 @@ struct ofnode_phandle_args;
>   * @round_rate: Adjust a rate to the exact rate a clock can provide.
>   * @get_rate: Get current clock rate.
>   * @set_rate: Set current clock rate.
> + * @get_parent: Get current clock parent
>   * @set_parent: Set current clock parent
>   * @enable: Enable a clock.
>   * @disable: Disable a clock.
> @@ -36,6 +37,7 @@ struct clk_ops {
>         ulong (*round_rate)(struct clk *clk, ulong rate);
>         ulong (*get_rate)(struct clk *clk);
>         ulong (*set_rate)(struct clk *clk, ulong rate);
> +       struct clk *(*get_parent)(struct clk *clk);
>         int (*set_parent)(struct clk *clk, struct clk *parent);
>         int (*enable)(struct clk *clk);
>         int (*disable)(struct clk *clk);
> --
> 2.39.2
>

Regards,
Simon

Re: [PATCH 2/6] clk: Fix error handling in clk_get_rate()

[Michal Suchánek]

Hello,

On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
> On 2/20/23 05:37, Michal Suchánek wrote:
> > Hello,
> > 
> > On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
> > > log_ret() cannot work with unsigned values, and the assignment to 'ret'
> > > incorrectly truncates the rate from long to int.
> > > 
> > > Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
> > > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > > ---
> > > 
> > >   drivers/clk/clk-uclass.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > > index dc3e9d6a261..78299dbceb2 100644
> > > --- a/drivers/clk/clk-uclass.c
> > > +++ b/drivers/clk/clk-uclass.c
> > > @@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
> > >   ulong clk_get_rate(struct clk *clk)
> > >   {
> > >   	const struct clk_ops *ops;
> > > -	int ret;
> > >   	debug("%s(clk=%p)\n", __func__, clk);
> > >   	if (!clk_valid(clk))
> > > @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
> > >   	if (!ops->get_rate)
> > >   		return -ENOSYS;
> > > -	ret = ops->get_rate(clk);
> > > -	if (ret)
> > > -		return log_ret(ret);
> > > -
> > > -	return 0;
> > > +	return ops->get_rate(clk);
> > >   }
> > 
> > This is generally poor design of the clock stuff in u-boot.
> > 
> > It returns -ERROR for many error conditions, but also 0 for other error
> > conditions.
> > 
> > Some error handling checks for 0, some for errval, some casts to int and
> > checks for <= 0.
> > 
> > I think that using -ERROR for clocks does not make much sense in u-boot.
> > 
> > Even in the kernel the errval checks are pretty much limited to places
> > where integers are used to store page frame numbers or pointers, that is
> > errptr stored in an integer. For adresses it is pretty easy to make sure
> > that the last page is not mapped making the error pointers invalid
> > (although bugs in that part happened too).
> > 
> > For clocks no such guarantee exists. The only apparently invalid clock
> > is 0, and the correct fix is to fix up the clock code to return 0 on
> > error, always. It's a lot of code to fix, though.
> > 
> > If you do not want to fix everything then the correct thing to do is
> > make ret ulong, and check for errval *and* 0.
> > 
> > There is not much point in returning detailed error codes in u-boot,
> > anyway. It's not like there is some userspace that could interpret them.
> > Most errors are logged when they happen if ever, and callers only check
> > if error happened or not.
> > 
> > Thanks
> > 
> > Michal
> 
> clk_get_parent is the only place where we return a clock pointer directly.
> Everywhere else, we have an integer return we can use. This function is
> different of course because CCF is broken and assumes there is only one
> canonical struct clk for a logical clock. So it can't initialize a caller-passed
> struct clk, but instead has to return the one true struct clk.
> 
> I agree with Michal here. The signature should really be
> 
> int clk_get_parent(struct clk *child, struct clk *parent)
> 
> but for now there is no point confusing the rest of the clock subsystem to make
> it work with error pointers.

This is clk_get_rate. It returns a clock rate. A clock rate of 2.5GHz is
not out of question, and cannot be expressed with a signed integer.

Hence clock rate should not be treated as signed, and -ERROR should not
be returned as clock rate.

Thanks

Michal

Re: [PATCH 2/6] clk: Fix error handling in clk_get_rate()

[Sean Anderson]

On 2/20/23 12:27, Michal Suchánek wrote:
> Hello,
> 
> On Mon, Feb 20, 2023 at 11:08:41AM -0500, Sean Anderson wrote:
>> On 2/20/23 05:37, Michal Suchánek wrote:
>>> Hello,
>>>
>>> On Sun, Feb 19, 2023 at 11:59:35PM -0600, Samuel Holland wrote:
>>>> log_ret() cannot work with unsigned values, and the assignment to 'ret'
>>>> incorrectly truncates the rate from long to int.
>>>>
>>>> Fixes: 5c5992cb90cf ("clk: Add debugging for return values")
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>
>>>>    drivers/clk/clk-uclass.c | 7 +------
>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>>>> index dc3e9d6a261..78299dbceb2 100644
>>>> --- a/drivers/clk/clk-uclass.c
>>>> +++ b/drivers/clk/clk-uclass.c
>>>> @@ -471,7 +471,6 @@ void clk_free(struct clk *clk)
>>>>    ulong clk_get_rate(struct clk *clk)
>>>>    {
>>>>    	const struct clk_ops *ops;
>>>> -	int ret;
>>>>    	debug("%s(clk=%p)\n", __func__, clk);
>>>>    	if (!clk_valid(clk))
>>>> @@ -481,11 +480,7 @@ ulong clk_get_rate(struct clk *clk)
>>>>    	if (!ops->get_rate)
>>>>    		return -ENOSYS;
>>>> -	ret = ops->get_rate(clk);
>>>> -	if (ret)
>>>> -		return log_ret(ret);
>>>> -
>>>> -	return 0;
>>>> +	return ops->get_rate(clk);
>>>>    }
>>>
>>> This is generally poor design of the clock stuff in u-boot.
>>>
>>> It returns -ERROR for many error conditions, but also 0 for other error
>>> conditions.
>>>
>>> Some error handling checks for 0, some for errval, some casts to int and
>>> checks for <= 0.
>>>
>>> I think that using -ERROR for clocks does not make much sense in u-boot.
>>>
>>> Even in the kernel the errval checks are pretty much limited to places
>>> where integers are used to store page frame numbers or pointers, that is
>>> errptr stored in an integer. For adresses it is pretty easy to make sure
>>> that the last page is not mapped making the error pointers invalid
>>> (although bugs in that part happened too).
>>>
>>> For clocks no such guarantee exists. The only apparently invalid clock
>>> is 0, and the correct fix is to fix up the clock code to return 0 on
>>> error, always. It's a lot of code to fix, though.
>>>
>>> If you do not want to fix everything then the correct thing to do is
>>> make ret ulong, and check for errval *and* 0.
>>>
>>> There is not much point in returning detailed error codes in u-boot,
>>> anyway. It's not like there is some userspace that could interpret them.
>>> Most errors are logged when they happen if ever, and callers only check
>>> if error happened or not.
>>>
>>> Thanks
>>>
>>> Michal
>>
>> clk_get_parent is the only place where we return a clock pointer directly.
>> Everywhere else, we have an integer return we can use. This function is
>> different of course because CCF is broken and assumes there is only one
>> canonical struct clk for a logical clock. So it can't initialize a caller-passed
>> struct clk, but instead has to return the one true struct clk.
>>
>> I agree with Michal here. The signature should really be
>>
>> int clk_get_parent(struct clk *child, struct clk *parent)
>>
>> but for now there is no point confusing the rest of the clock subsystem to make
>> it work with error pointers.
> 
> This is clk_get_rate.

Ah, whoops. In that case,

Reviewed-by: Sean Anderson <seanga2@gmail.com>

> It returns a clock rate. A clock rate of 2.5GHz is
> not out of question, and cannot be expressed with a signed integer.

And of course neither is a rate of 5 GHz :)

> Hence clock rate should not be treated as signed, and -ERROR should not
> be returned as clock rate.

Yes...

I would accept a conversion to "0 means unknown rate (for whatever reason)" API.

--Sean

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

[Michal Suchánek]

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

> 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()

[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@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

Re: [PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Samuel Holland]

On 2/20/23 04:39, Michal Suchánek wrote:
> On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
>> Do not return both NULL and error pointers. The function is only
>> documented as returning error pointers.
>>
>> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/clk/clk-uclass.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index 78299dbceb2..5bce976b060 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
>>  
>>  	debug("%s(clk=%p)\n", __func__, clk);
>>  	if (!clk_valid(clk))
>> -		return NULL;
>> +		return ERR_PTR(-ENODEV);
>>  
>>  	pdev = dev_get_parent(clk->dev);
>>  	if (!pdev)
> 
> Do we really need this?
> 
> Who cares about the distinction?
> 
> Just returning NULL on any error makes the code so much simpler and
> easier to understand.

I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done
later in the function, but returning both makes the error handling in
callers more complicated for no benefit. It sounds like you would prefer
I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I
can do that for v2.

Regards,
Samuel

Re: [PATCH 4/6] clk: Fix rate caching in clk_get_parent_rate()

[Samuel Holland]

On 2/20/23 10:11, Sean Anderson wrote:
> On 2/20/23 00:59, Samuel Holland wrote:
>> clk_get_rate() can return an error value. Recompute the rate if the
>> cached value is an error value.
>>
>> Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk
>> operations")
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>   drivers/clk/clk-uclass.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index 5bce976b060..9d052e5a814 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -520,7 +520,8 @@ ulong clk_get_parent_rate(struct clk *clk)
>>           return -ENOSYS;
>>         /* Read the 'rate' if not already set or if proper flag set*/
>> -    if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE)
>> +    if (!pclk->rate || IS_ERR_VALUE(pclk->rate) ||
>> +        pclk->flags & CLK_GET_RATE_NOCACHE)
>>           pclk->rate = clk_get_rate(pclk);
>>         return pclk->rate;
> 
> The correct fix here is
> 
>     /* Read the 'rate' if not already set or if proper flag set*/
>     if (!pclk->rate || pclk->flags & CLK_GET_RATE_NOCACHE) {
>         rate = clk_get_rate(pclk);
>         if (!IS_ERR_VALUE(rate))
>             pclk->rate = rate;
>     }
> 
>     return rate;
> 
> No point caching errors.

Good point. I will do this for v2.

Regards,
Samuel

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

[Michal Suchánek]

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 <samuel@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 <msuchanek@suse.de>

Thanks

Michal

Re: [PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Michal Suchánek]

On Sat, Mar 04, 2023 at 01:58:17PM -0600, Samuel Holland wrote:
> On 2/20/23 04:39, Michal Suchánek wrote:
> > On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
> >> Do not return both NULL and error pointers. The function is only
> >> documented as returning error pointers.
> >>
> >> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >>  drivers/clk/clk-uclass.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> >> index 78299dbceb2..5bce976b060 100644
> >> --- a/drivers/clk/clk-uclass.c
> >> +++ b/drivers/clk/clk-uclass.c
> >> @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
> >>  
> >>  	debug("%s(clk=%p)\n", __func__, clk);
> >>  	if (!clk_valid(clk))
> >> -		return NULL;
> >> +		return ERR_PTR(-ENODEV);
> >>  
> >>  	pdev = dev_get_parent(clk->dev);
> >>  	if (!pdev)
> > 
> > Do we really need this?
> > 
> > Who cares about the distinction?
> > 
> > Just returning NULL on any error makes the code so much simpler and
> > easier to understand.
> 
> I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done
> later in the function, but returning both makes the error handling in
> callers more complicated for no benefit. It sounds like you would prefer
> I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I
> can do that for v2.

I think NULL is easier to check, and so long as there is no meaningful
distinction between ERR_PTR and NULL we should converge towards NULL as
the single error return value.

Thanks

Michal

Re: [PATCH 3/6] clk: Fix error handling in clk_get_parent()

[Sean Anderson]

On 3/4/23 15:46, Michal Suchánek wrote:
> On Sat, Mar 04, 2023 at 01:58:17PM -0600, Samuel Holland wrote:
>> On 2/20/23 04:39, Michal Suchánek wrote:
>>> On Sun, Feb 19, 2023 at 11:59:36PM -0600, Samuel Holland wrote:
>>>> Do not return both NULL and error pointers. The function is only
>>>> documented as returning error pointers.
>>>>
>>>> Fixes: 8a1661f20e6c ("drivers: clk: Handle gracefully NULL pointers")
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>
>>>>   drivers/clk/clk-uclass.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>>>> index 78299dbceb2..5bce976b060 100644
>>>> --- a/drivers/clk/clk-uclass.c
>>>> +++ b/drivers/clk/clk-uclass.c
>>>> @@ -490,7 +490,7 @@ struct clk *clk_get_parent(struct clk *clk)
>>>>   
>>>>   	debug("%s(clk=%p)\n", __func__, clk);
>>>>   	if (!clk_valid(clk))
>>>> -		return NULL;
>>>> +		return ERR_PTR(-ENODEV);
>>>>   
>>>>   	pdev = dev_get_parent(clk->dev);
>>>>   	if (!pdev)
>>>
>>> Do we really need this?
>>>
>>> Who cares about the distinction?
>>>
>>> Just returning NULL on any error makes the code so much simpler and
>>> easier to understand.
>>
>> I'm fine with returning only NULL, or only ERR_PTR(-ENODEV) as done
>> later in the function, but returning both makes the error handling in
>> callers more complicated for no benefit. It sounds like you would prefer
>> I change the later ERR_PTR(-ENODEV) to NULL instead of this change? I
>> can do that for v2.
> 
> I think NULL is easier to check, and so long as there is no meaningful
> distinction between ERR_PTR and NULL we should converge towards NULL as
> the single error return value.

The only issue I can see is if you have e.g. an I2C clock and you need to
examine the register to determine the parent and the slave doesn't respond.
Is the distinction between "I have no parent" and "I have a parent
but I don't know which" meaningful? Linux uses one return because parents
must be determined at probe time. I'm fine with making this return NULL on
error; this should be uncommon enough that we can recommend dev_err()
reporting in the driver.

--Sean