| Message ID | 871qcj5frz.fsf@baylibre.com |
|---|---|
| State | Superseded |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | clk: fix clk_get_rate() always return ulong | expand |
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> > --- > 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
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
Sorry for the noise but I miss a (obvious) warning:
drivers/clk/clk-uclass.c:474:8: warning: unused variable ‘ret’ [-Wunused-variable]
474 | ulong ret;
| ^~~
I will send a V2.
On Fri 15 Dec 2023 at 14:32, 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.
> Otherwise we may return an invalid/truncated clock rate value.
>
> Signed-off-by: Julien Masson <jmasson@baylibre.com>
> ---
> 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
>
Hi Mattijs, Thanks for the review. On Fri 15 Dec 2023 at 14:38, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote: > 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? > I agree with the suggestion but in that case all the macros in log.h may need to be updated with the same kind of checks. And that is not the initial intention of this patch, we should do that in a separate patch I guess. >> >>> --- >>> 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
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)
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. Otherwise we may return an invalid/truncated clock rate value. Signed-off-by: Julien Masson <jmasson@baylibre.com> --- drivers/clk/clk-uclass.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)