* [PATCH-next] arm: fix clang build warning in include/asm/memory.h
@ 2024-03-14 7:54 Yipeng Zou
2024-03-15 0:43 ` Nathan Chancellor
0 siblings, 1 reply; 10+ messages in thread
From: Yipeng Zou @ 2024-03-14 7:54 UTC (permalink / raw)
To: linux, nathan, ndesaulniers, morbo, justinstitt, linus.walleij,
ssantosh, linux-arm-kernel
Cc: zouyipeng
There is a build error has been founded with build in clang-15.0.4:
./arch/arm/include/asm/memory.h:358:12: error: result of comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false [-Werror, -Wtautological-type-limit-compare]
if (addr > (u32)~0)
~~~~ ^ ~~~~~~~
It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap() functionality")
Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
---
arch/arm/include/asm/memory.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index ef2aa79ece5a..64ced9eb42ff 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
}
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
#define IDMAP_INVALID_ADDR ((u32)~0)
+#endif
static inline unsigned long phys_to_idmap(phys_addr_t addr)
{
if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
addr += arch_phys_to_idmap_offset;
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
if (addr > (u32)~0)
addr = IDMAP_INVALID_ADDR;
+#endif
}
return addr;
}
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-14 7:54 [PATCH-next] arm: fix clang build warning in include/asm/memory.h Yipeng Zou
@ 2024-03-15 0:43 ` Nathan Chancellor
2024-03-15 7:40 ` Linus Walleij
2024-03-19 3:16 ` Yipeng Zou
0 siblings, 2 replies; 10+ messages in thread
From: Nathan Chancellor @ 2024-03-15 0:43 UTC (permalink / raw)
To: Yipeng Zou
Cc: linux, ndesaulniers, morbo, justinstitt, linus.walleij, ssantosh,
linux-arm-kernel
On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
> There is a build error has been founded with build in clang-15.0.4:
>
> ./arch/arm/include/asm/memory.h:358:12: error: result of comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false [-Werror, -Wtautological-type-limit-compare]
> if (addr > (u32)~0)
> ~~~~ ^ ~~~~~~~
>
> It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
>
> Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
>
> Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap() functionality")
> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
> ---
> arch/arm/include/asm/memory.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index ef2aa79ece5a..64ced9eb42ff 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
> return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
> }
>
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> #define IDMAP_INVALID_ADDR ((u32)~0)
> +#endif
>
> static inline unsigned long phys_to_idmap(phys_addr_t addr)
> {
> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> addr += arch_phys_to_idmap_offset;
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> if (addr > (u32)~0)
> addr = IDMAP_INVALID_ADDR;
> +#endif
> }
> return addr;
> }
> --
> 2.34.1
>
Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
I can't see why this would not work for avoiding that warning.
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index ef2aa79ece5a..fbff07bc3b28 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -353,7 +353,7 @@ static inline unsigned long phys_to_idmap(phys_addr_t addr)
{
if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
addr += arch_phys_to_idmap_offset;
- if (addr > (u32)~0)
+ if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
addr = IDMAP_INVALID_ADDR;
}
return addr;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 0:43 ` Nathan Chancellor
@ 2024-03-15 7:40 ` Linus Walleij
2024-03-15 10:08 ` Russell King (Oracle)
2024-03-19 3:13 ` Yipeng Zou
2024-03-19 3:16 ` Yipeng Zou
1 sibling, 2 replies; 10+ messages in thread
From: Linus Walleij @ 2024-03-15 7:40 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Yipeng Zou, linux, ndesaulniers, morbo, justinstitt, ssantosh,
linux-arm-kernel
On Fri, Mar 15, 2024 at 1:43 AM Nathan Chancellor <nathan@kernel.org> wrote:
> On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
> > There is a build error has been founded with build in clang-15.0.4:
> >
> > ./arch/arm/include/asm/memory.h:358:12: error: result of comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false [-Werror, -Wtautological-type-limit-compare]
> > if (addr > (u32)~0)
> > ~~~~ ^ ~~~~~~~
> >
> > It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
> >
> > Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
> >
> > Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap() functionality")
> > Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
> > ---
> > arch/arm/include/asm/memory.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index ef2aa79ece5a..64ced9eb42ff 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
> > return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
> > }
> >
> > +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> > #define IDMAP_INVALID_ADDR ((u32)~0)
> > +#endif
> >
> > static inline unsigned long phys_to_idmap(phys_addr_t addr)
> > {
> > if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> > addr += arch_phys_to_idmap_offset;
> > +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> > if (addr > (u32)~0)
> > addr = IDMAP_INVALID_ADDR;
> > +#endif
> > }
> > return addr;
> > }
> > --
> > 2.34.1
> >
>
> Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
> I can't see why this would not work for avoiding that warning.
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index ef2aa79ece5a..fbff07bc3b28 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -353,7 +353,7 @@ static inline unsigned long phys_to_idmap(phys_addr_t addr)
> {
> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> addr += arch_phys_to_idmap_offset;
> - if (addr > (u32)~0)
> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
> addr = IDMAP_INVALID_ADDR;
+1 and I would probably use this as well:
<linux/limits.h>
#define IDMAP_INVALID_ADDR PHYS_ADDR_MAX
if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > PHYS_ADDR_MAX)
addr = IDMAP_INVALID_ADDR;
Because then it is clear what is going on: we are capping to the max physical
address.
PHYS_ADDR_MAX is defined to (~(phys_addr_t)0) which on
ARM is (~(u32)0).
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 7:40 ` Linus Walleij
@ 2024-03-15 10:08 ` Russell King (Oracle)
2024-03-15 13:16 ` Linus Walleij
2024-03-19 3:13 ` Yipeng Zou
1 sibling, 1 reply; 10+ messages in thread
From: Russell King (Oracle) @ 2024-03-15 10:08 UTC (permalink / raw)
To: Linus Walleij
Cc: Nathan Chancellor, Yipeng Zou, ndesaulniers, morbo, justinstitt,
ssantosh, linux-arm-kernel
On Fri, Mar 15, 2024 at 08:40:28AM +0100, Linus Walleij wrote:
> +1 and I would probably use this as well:
>
> <linux/limits.h>
>
> #define IDMAP_INVALID_ADDR PHYS_ADDR_MAX
>
> if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > PHYS_ADDR_MAX)
> addr = IDMAP_INVALID_ADDR;
>
> Because then it is clear what is going on: we are capping to the max physical
> address.
>
> PHYS_ADDR_MAX is defined to (~(phys_addr_t)0) which on
> ARM is (~(u32)0).
... which is _not_ the same as (u32)~0, so using PHYS_ADDR_MAX is not
appropriate.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 10:08 ` Russell King (Oracle)
@ 2024-03-15 13:16 ` Linus Walleij
2024-03-15 14:46 ` Russell King (Oracle)
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2024-03-15 13:16 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Nathan Chancellor, Yipeng Zou, ndesaulniers, morbo, justinstitt,
ssantosh, linux-arm-kernel
On Fri, Mar 15, 2024 at 11:08 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Fri, Mar 15, 2024 at 08:40:28AM +0100, Linus Walleij wrote:
> > +1 and I would probably use this as well:
> >
> > <linux/limits.h>
> >
> > #define IDMAP_INVALID_ADDR PHYS_ADDR_MAX
> >
> > if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > PHYS_ADDR_MAX)
> > addr = IDMAP_INVALID_ADDR;
> >
> > Because then it is clear what is going on: we are capping to the max physical
> > address.
> >
> > PHYS_ADDR_MAX is defined to (~(phys_addr_t)0) which on
> > ARM is (~(u32)0).
>
> ... which is _not_ the same as (u32)~0, so using PHYS_ADDR_MAX is not
> appropriate.
Attention to detail! Thanks Russell.
However U32_MAX is
#define U32_MAX ((u32)~0U)
So that should work and also convey the attention I think?
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 13:16 ` Linus Walleij
@ 2024-03-15 14:46 ` Russell King (Oracle)
0 siblings, 0 replies; 10+ messages in thread
From: Russell King (Oracle) @ 2024-03-15 14:46 UTC (permalink / raw)
To: Linus Walleij
Cc: Nathan Chancellor, Yipeng Zou, ndesaulniers, morbo, justinstitt,
ssantosh, linux-arm-kernel
On Fri, Mar 15, 2024 at 02:16:23PM +0100, Linus Walleij wrote:
> On Fri, Mar 15, 2024 at 11:08 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> > On Fri, Mar 15, 2024 at 08:40:28AM +0100, Linus Walleij wrote:
> > > +1 and I would probably use this as well:
> > >
> > > <linux/limits.h>
> > >
> > > #define IDMAP_INVALID_ADDR PHYS_ADDR_MAX
> > >
> > > if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > PHYS_ADDR_MAX)
> > > addr = IDMAP_INVALID_ADDR;
> > >
> > > Because then it is clear what is going on: we are capping to the max physical
> > > address.
> > >
> > > PHYS_ADDR_MAX is defined to (~(phys_addr_t)0) which on
> > > ARM is (~(u32)0).
> >
> > ... which is _not_ the same as (u32)~0, so using PHYS_ADDR_MAX is not
> > appropriate.
>
> Attention to detail! Thanks Russell.
>
> However U32_MAX is
> #define U32_MAX ((u32)~0U)
>
> So that should work and also convey the attention I think?
If you want to use that to define IDMAP_INVALID_ADDR then yes. If you're
thinking about getting rid of IDMAP_INVALID_ADDR, then I think that
would reduce code readability, because the idea here is that addresses
above the 32-bit virtual space are invalid for IDMAP, and just using
U32_MAX is a rather opaque way to convey that.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 7:40 ` Linus Walleij
2024-03-15 10:08 ` Russell King (Oracle)
@ 2024-03-19 3:13 ` Yipeng Zou
1 sibling, 0 replies; 10+ messages in thread
From: Yipeng Zou @ 2024-03-19 3:13 UTC (permalink / raw)
To: Linus Walleij, Nathan Chancellor
Cc: linux, ndesaulniers, morbo, justinstitt, ssantosh,
linux-arm-kernel
在 2024/3/15 15:40, Linus Walleij 写道:
> On Fri, Mar 15, 2024 at 1:43 AM Nathan Chancellor <nathan@kernel.org> wrote:
>> On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
>>> There is a build error has been founded with build in clang-15.0.4:
>>>
>>> ./arch/arm/include/asm/memory.h:358:12: error: result of comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false [-Werror, -Wtautological-type-limit-compare]
>>> if (addr > (u32)~0)
>>> ~~~~ ^ ~~~~~~~
>>>
>>> It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
>>>
>>> Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
>>>
>>> Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap() functionality")
>>> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
>>> ---
>>> arch/arm/include/asm/memory.h | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>>> index ef2aa79ece5a..64ced9eb42ff 100644
>>> --- a/arch/arm/include/asm/memory.h
>>> +++ b/arch/arm/include/asm/memory.h
>>> @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
>>> return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
>>> }
>>>
>>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> #define IDMAP_INVALID_ADDR ((u32)~0)
>>> +#endif
>>>
>>> static inline unsigned long phys_to_idmap(phys_addr_t addr)
>>> {
>>> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
>>> addr += arch_phys_to_idmap_offset;
>>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> if (addr > (u32)~0)
>>> addr = IDMAP_INVALID_ADDR;
>>> +#endif
>>> }
>>> return addr;
>>> }
>>> --
>>> 2.34.1
>>>
>> Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
>> I can't see why this would not work for avoiding that warning.
>>
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index ef2aa79ece5a..fbff07bc3b28 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -353,7 +353,7 @@ static inline unsigned long phys_to_idmap(phys_addr_t addr)
>> {
>> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
>> addr += arch_phys_to_idmap_offset;
>> - if (addr > (u32)~0)
>> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
>> addr = IDMAP_INVALID_ADDR;
> +1 and I would probably use this as well:
>
> <linux/limits.h>
>
> #define IDMAP_INVALID_ADDR PHYS_ADDR_MAX
>
> if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > PHYS_ADDR_MAX)
> addr = IDMAP_INVALID_ADDR;
>
> Because then it is clear what is going on: we are capping to the max physical
> address.
Yes, indeed, It's clearer to indicate what is going on.
Will fix it in that way.
> PHYS_ADDR_MAX is defined to (~(phys_addr_t)0) which on
> ARM is (~(u32)0).
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Regards,
Yipeng Zou
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-15 0:43 ` Nathan Chancellor
2024-03-15 7:40 ` Linus Walleij
@ 2024-03-19 3:16 ` Yipeng Zou
2024-03-19 3:38 ` Yipeng Zou
1 sibling, 1 reply; 10+ messages in thread
From: Yipeng Zou @ 2024-03-19 3:16 UTC (permalink / raw)
To: Nathan Chancellor
Cc: linux, ndesaulniers, morbo, justinstitt, linus.walleij, ssantosh,
linux-arm-kernel
在 2024/3/15 8:43, Nathan Chancellor 写道:
> On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
>> There is a build error has been founded with build in clang-15.0.4:
>>
>> ./arch/arm/include/asm/memory.h:358:12: error: result of comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false [-Werror, -Wtautological-type-limit-compare]
>> if (addr > (u32)~0)
>> ~~~~ ^ ~~~~~~~
>>
>> It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
>>
>> Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
>>
>> Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap() functionality")
>> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
>> ---
>> arch/arm/include/asm/memory.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index ef2aa79ece5a..64ced9eb42ff 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
>> return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
>> }
>>
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> #define IDMAP_INVALID_ADDR ((u32)~0)
>> +#endif
>>
>> static inline unsigned long phys_to_idmap(phys_addr_t addr)
>> {
>> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
>> addr += arch_phys_to_idmap_offset;
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> if (addr > (u32)~0)
>> addr = IDMAP_INVALID_ADDR;
>> +#endif
>> }
>> return addr;
>> }
>> --
>> 2.34.1
>>
> Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
> I can't see why this would not work for avoiding that warning.
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index ef2aa79ece5a..fbff07bc3b28 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -353,7 +353,7 @@ static inline unsigned long phys_to_idmap(phys_addr_t addr)
> {
> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> addr += arch_phys_to_idmap_offset;
> - if (addr > (u32)~0)
> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
> addr = IDMAP_INVALID_ADDR;
> }
> return addr;
It absolutely works for it and yes, It's a better way to avoiding that.
Will fix it in that way.
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Regards,
Yipeng Zou
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-19 3:16 ` Yipeng Zou
@ 2024-03-19 3:38 ` Yipeng Zou
2024-03-19 14:46 ` Nathan Chancellor
0 siblings, 1 reply; 10+ messages in thread
From: Yipeng Zou @ 2024-03-19 3:38 UTC (permalink / raw)
To: Nathan Chancellor
Cc: linux, ndesaulniers, morbo, justinstitt, linus.walleij, ssantosh,
linux-arm-kernel
在 2024/3/19 11:16, Yipeng Zou 写道:
>
> 在 2024/3/15 8:43, Nathan Chancellor 写道:
>> On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
>>> There is a build error has been founded with build in clang-15.0.4:
>>>
>>> ./arch/arm/include/asm/memory.h:358:12: error: result of comparison
>>> "phys addr_t’ (aka 'unsigned int’) > 4294967295 is always false
>>> [-Werror, -Wtautological-type-limit-compare]
>>> if (addr > (u32)~0)
>>> ~~~~ ^ ~~~~~~~
>>>
>>> It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
>>>
>>> Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
>>>
>>> Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap()
>>> functionality")
>>> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
>>> ---
>>> arch/arm/include/asm/memory.h | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm/include/asm/memory.h
>>> b/arch/arm/include/asm/memory.h
>>> index ef2aa79ece5a..64ced9eb42ff 100644
>>> --- a/arch/arm/include/asm/memory.h
>>> +++ b/arch/arm/include/asm/memory.h
>>> @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
>>> return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
>>> }
>>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> #define IDMAP_INVALID_ADDR ((u32)~0)
>>> +#endif
>>> static inline unsigned long phys_to_idmap(phys_addr_t addr)
>>> {
>>> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
>>> addr += arch_phys_to_idmap_offset;
>>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>>> if (addr > (u32)~0)
>>> addr = IDMAP_INVALID_ADDR;
>>> +#endif
>>> }
>>> return addr;
>>> }
>>> --
>>> 2.34.1
>>>
>> Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
>> I can't see why this would not work for avoiding that warning.
>>
>> diff --git a/arch/arm/include/asm/memory.h
>> b/arch/arm/include/asm/memory.h
>> index ef2aa79ece5a..fbff07bc3b28 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -353,7 +353,7 @@ static inline unsigned long
>> phys_to_idmap(phys_addr_t addr)
>> {
>> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
>> addr += arch_phys_to_idmap_offset;
>> - if (addr > (u32)~0)
>> + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
>> addr = IDMAP_INVALID_ADDR;
>> }
>> return addr;
>
> It absolutely works for it and yes, It's a better way to avoiding that.
>
> Will fix it in that way.
>
But I notice that, this way can not silence it when compile, it only can
avoid it in running time.
So, maybe we still need to silence it in ifdef's.
Other than that, IDMAP_INVALID_ADDR was used in other place, need to
keep it defined.
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 158ad13e9f6a..7143147cd7cc 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -351,8 +351,10 @@ static inline unsigned long
phys_to_idmap(phys_addr_t addr)
{
if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
addr += arch_phys_to_idmap_offset;
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
if (addr > (u32)~0)
addr = IDMAP_INVALID_ADDR;
+#endif
}
return addr;
}
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Regards,
Yipeng Zou
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH-next] arm: fix clang build warning in include/asm/memory.h
2024-03-19 3:38 ` Yipeng Zou
@ 2024-03-19 14:46 ` Nathan Chancellor
0 siblings, 0 replies; 10+ messages in thread
From: Nathan Chancellor @ 2024-03-19 14:46 UTC (permalink / raw)
To: Yipeng Zou
Cc: linux, ndesaulniers, morbo, justinstitt, linus.walleij, ssantosh,
linux-arm-kernel
On Tue, Mar 19, 2024 at 11:38:02AM +0800, Yipeng Zou wrote:
>
> 在 2024/3/19 11:16, Yipeng Zou 写道:
> >
> > 在 2024/3/15 8:43, Nathan Chancellor 写道:
> > > On Thu, Mar 14, 2024 at 03:54:09PM +0800, Yipeng Zou wrote:
> > > > There is a build error has been founded with build in clang-15.0.4:
> > > >
> > > > ./arch/arm/include/asm/memory.h:358:12: error: result of
> > > > comparison "phys addr_t’ (aka 'unsigned int’) > 4294967295 is
> > > > always false [-Werror, -Wtautological-type-limit-compare]
> > > > if (addr > (u32)~0)
> > > > ~~~~ ^ ~~~~~~~
> > > >
> > > > It will be always goes fail without CONFIG_PHYS_ADDR_T_64BIT.
> > > >
> > > > Directly silence it by Use CONFIG_PHYS_ADDR_T_64BIT.
> > > >
> > > > Fixes: 981b6714dbd2 ("ARM: provide improved virt_to_idmap()
> > > > functionality")
> > > > Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
> > > > ---
> > > > arch/arm/include/asm/memory.h | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm/include/asm/memory.h
> > > > b/arch/arm/include/asm/memory.h
> > > > index ef2aa79ece5a..64ced9eb42ff 100644
> > > > --- a/arch/arm/include/asm/memory.h
> > > > +++ b/arch/arm/include/asm/memory.h
> > > > @@ -347,14 +347,18 @@ static inline bool arm_has_idmap_alias(void)
> > > > return IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset != 0;
> > > > }
> > > > +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> > > > #define IDMAP_INVALID_ADDR ((u32)~0)
> > > > +#endif
> > > > static inline unsigned long phys_to_idmap(phys_addr_t addr)
> > > > {
> > > > if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> > > > addr += arch_phys_to_idmap_offset;
> > > > +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> > > > if (addr > (u32)~0)
> > > > addr = IDMAP_INVALID_ADDR;
> > > > +#endif
> > > > }
> > > > return addr;
> > > > }
> > > > --
> > > > 2.34.1
> > > >
> > > Would it be better to avoid the ifdef's here and just use IS_ENABLED()?
> > > I can't see why this would not work for avoiding that warning.
> > >
> > > diff --git a/arch/arm/include/asm/memory.h
> > > b/arch/arm/include/asm/memory.h
> > > index ef2aa79ece5a..fbff07bc3b28 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -353,7 +353,7 @@ static inline unsigned long
> > > phys_to_idmap(phys_addr_t addr)
> > > {
> > > if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> > > addr += arch_phys_to_idmap_offset;
> > > - if (addr > (u32)~0)
> > > + if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) && addr > (u32)~0)
> > > addr = IDMAP_INVALID_ADDR;
> > > }
> > > return addr;
> >
> > It absolutely works for it and yes, It's a better way to avoiding that.
> >
> > Will fix it in that way.
> >
> But I notice that, this way can not silence it when compile, it only can
> avoid it in running time.
Yeah, I actually tested my suggestion and saw the same thing. Sometimes
diagnostics will be hidden by '0 &&' but it seems like that is not the
case here. It is not the end of the world, I think that diff looks fine
for the issue at hand.
> So, maybe we still need to silence it in ifdef's.
>
> Other than that, IDMAP_INVALID_ADDR was used in other place, need to keep it
> defined.
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 158ad13e9f6a..7143147cd7cc 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -351,8 +351,10 @@ static inline unsigned long phys_to_idmap(phys_addr_t
> addr)
> {
> if (IS_ENABLED(CONFIG_MMU) && arch_phys_to_idmap_offset) {
> addr += arch_phys_to_idmap_offset;
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> if (addr > (u32)~0)
> addr = IDMAP_INVALID_ADDR;
> +#endif
> }
> return addr;
> }
>
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> --
> Regards,
> Yipeng Zou
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-19 14:47 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-14 7:54 [PATCH-next] arm: fix clang build warning in include/asm/memory.h Yipeng Zou
2024-03-15 0:43 ` Nathan Chancellor
2024-03-15 7:40 ` Linus Walleij
2024-03-15 10:08 ` Russell King (Oracle)
2024-03-15 13:16 ` Linus Walleij
2024-03-15 14:46 ` Russell King (Oracle)
2024-03-19 3:13 ` Yipeng Zou
2024-03-19 3:16 ` Yipeng Zou
2024-03-19 3:38 ` Yipeng Zou
2024-03-19 14:46 ` Nathan Chancellor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).