Re: [PATCH] clk: fix clk_get_rate() always return ulong

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

On ven., déc. 15, 2023 at 13:34, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Julien,
>
> Thank you for the patch.
>
> On mar., nov. 21, 2023 at 15:42, Julien Masson <jmasson@baylibre.com> wrote:
>
>> When we call clk_get_rate(), we expect to get clock rate value as
>> ulong.
>> In that case we should not use log_ret() macro since it use internally
>> an int.
>
> This is quite subtle, it only happens when log_reg is enabled via
> CONFIG_LOG_ERROR_RETURN.
>
>> Otherwise we may return an invalid/truncated clock rate value.
>>
>> Signed-off-by: Julien Masson <jmasson@baylibre.com>
>
> I'm wondering if there are any other places where this happens,
> but this change looks good to me.
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Thinking a bit more about this, shouldn't we add some check for
downcasting in log.h in order to make sure this does not happen in the future?

Something like this should work:

diff --git a/include/log.h b/include/log.h
index 6e84f080ef3d..20088399617c 100644
--- a/include/log.h
+++ b/include/log.h
@@ -332,12 +332,14 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,
  * use 4 bytes in rodata
  */
 #define log_ret(_ret) ({ \
+       _Static_assert(sizeof(_ret) <= sizeof(int), "Cannot log without downcasting"); \
        int __ret = (_ret); \
        if (__ret < 0) \
                log(LOG_CATEGORY, LOGL_ERR, "returning err=%d\n", __ret); \
        __ret; \
        })

Downcasting just when logs are enabled can be the cause for quite some
subtle bugs so breaking the build in these cases might be a good
warning.

Any thoughts?

>
>> ---
>>  drivers/clk/clk-uclass.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index 3b5e3f9c86..41dcd14be5 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -488,11 +488,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.41.0