diff mbox series

[v5,1/7] bits: split the definition of the asm and non-asm GENMASK()

Message ID 20250306-fixed-type-genmasks-v5-1-b443e9dcba63@wanadoo.fr (mailing list archive)
State New
Headers show
Series bits: Fixed-type GENMASK()/BIT() | expand

Commit Message

Vincent Mailhol via B4 Relay March 6, 2025, 11:29 a.m. UTC 
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

In an upcoming change, GENMASK() and its friends will indirectly
depend on sizeof() which is not available in asm.

Instead of adding further complexity to __GENMASK() to make it work
for both asm and non asm, just split the definition of the two
variants.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v4 -> v5:

    - Use tab indentations instead of single space to separate the
      macro name from its body.

  v3 -> v4:

    - New patch in the series
---
 include/linux/bits.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko March 6, 2025, 1:05 p.m. UTC | #1 
On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> In an upcoming change, GENMASK() and its friends will indirectly
> depend on sizeof() which is not available in asm.
> 
> Instead of adding further complexity to __GENMASK() to make it work
> for both asm and non asm, just split the definition of the two
> variants.

...

> -/*
> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> - * disable the input check if that is the case.
> - */

I believe this comment is still valid...

> +#else /* defined(__ASSEMBLY__) */


...here.

Otherwise justify its removal in the commit message.

> +#define GENMASK(h, l)		__GENMASK(h, l)
> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> +
> +#endif /* !defined(__ASSEMBLY__) */
Lucas De Marchi March 6, 2025, 2:34 p.m. UTC | #2 
On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote:
>From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
>In an upcoming change, GENMASK() and its friends will indirectly
>depend on sizeof() which is not available in asm.
>
>Instead of adding further complexity to __GENMASK() to make it work
>for both asm and non asm, just split the definition of the two
>variants.
>
>Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>---
>Changelog:
>
>  v4 -> v5:
>
>    - Use tab indentations instead of single space to separate the
>      macro name from its body.
>
>  v3 -> v4:
>
>    - New patch in the series
>---
> include/linux/bits.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
>diff --git a/include/linux/bits.h b/include/linux/bits.h
>index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 100644
>--- a/include/linux/bits.h
>+++ b/include/linux/bits.h
>@@ -19,23 +19,17 @@
>  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>  */
> #if !defined(__ASSEMBLY__)
>+
> #include <linux/build_bug.h>
> #include <linux/compiler.h>
>+
> #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>-#else
>-/*
>- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>- * disable the input check if that is the case.
>- */

it seems we now have 1 inconsistency that we comment why
GENMASK_U128() is not available in asm, but we don't comment why
GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
top of GENMASK_INPUT_CHECK().

Anyway,

	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks for picking up this series.

Lucas De Marchi

>-#define GENMASK_INPUT_CHECK(h, l) 0
>-#endif
>
> #define GENMASK(h, l) \
> 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> #define GENMASK_ULL(h, l) \
> 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
>-#if !defined(__ASSEMBLY__)
> /*
>  * Missing asm support
>  *
>@@ -48,6 +42,12 @@
>  */
> #define GENMASK_U128(h, l) \
> 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>-#endif
>+
>+#else /* defined(__ASSEMBLY__) */
>+
>+#define GENMASK(h, l)		__GENMASK(h, l)
>+#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
>+
>+#endif /* !defined(__ASSEMBLY__) */
>
> #endif	/* __LINUX_BITS_H */
>
>-- 
>2.45.3
>
>
Vincent Mailhol March 6, 2025, 3:07 p.m. UTC | #3 
On 06/03/2025 at 22:05, Andy Shevchenko wrote:
> On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>
>> In an upcoming change, GENMASK() and its friends will indirectly
>> depend on sizeof() which is not available in asm.
>>
>> Instead of adding further complexity to __GENMASK() to make it work
>> for both asm and non asm, just split the definition of the two
>> variants.
> 
> ...
> 
>> -/*
>> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>> - * disable the input check if that is the case.
>> - */
> 
> I believe this comment is still valid...
> 
>> +#else /* defined(__ASSEMBLY__) */
> 
> 
> ...here.
> 
> Otherwise justify its removal in the commit message.

OK. I will restore the comment in v6, but will move it to the #else
branch, like this:

  #else /* defined(__ASSEMBLY__) */

  /*
   * BUILD_BUG_ON_ZERO is not available in h files included from asm
   * files, so no input checks in assembly.
   */
  #define GENMASK(h, l)		__GENMASK(h, l)
  #define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)

  #endif /* !defined(__ASSEMBLY__) */

>> +#define GENMASK(h, l)		__GENMASK(h, l)
>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
>> +
>> +#endif /* !defined(__ASSEMBLY__) */
> 

Yours sincerely,
Vincent Mailhol
Vincent Mailhol March 6, 2025, 5:10 p.m. UTC | #4 
On 06/03/2025 at 23:34, Lucas De Marchi wrote:
> On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay
> wrote:

(...)

> it seems we now have 1 inconsistency that we comment why
> GENMASK_U128() is not available in asm, but we don't comment why
> GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
> top of GENMASK_INPUT_CHECK().

I will restore the comment in v6 and put it next to the asm definition,
c.f. my reply to Andy.

> Anyway,
> 
>     Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Is this only valid for the first patch or for the full series? If this
is for the full series, would you mind replying to the cover letter with
your review tag?

> thanks for picking up this series.

You are welcome!

> Lucas De Marchi

(...)

Yours sincerely,
Vincent Mailhol
Andy Shevchenko March 6, 2025, 5:51 p.m. UTC | #5 
On Fri, Mar 07, 2025 at 12:07:45AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 22:05, Andy Shevchenko wrote:
> > On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote:
> >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

...

> >> -/*
> >> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> >> - * disable the input check if that is the case.
> >> - */
> > 
> > I believe this comment is still valid...
> > 
> >> +#else /* defined(__ASSEMBLY__) */
> > 
> > 
> > ...here.
> > 
> > Otherwise justify its removal in the commit message.
> 
> OK. I will restore the comment in v6, but will move it to the #else
> branch,

Isn't it what I suggested? :-)

> like this:

>   #else /* defined(__ASSEMBLY__) */
> 
>   /*
>    * BUILD_BUG_ON_ZERO is not available in h files included from asm
>    * files, so no input checks in assembly.
>    */
>   #define GENMASK(h, l)		__GENMASK(h, l)
>   #define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> 
>   #endif /* !defined(__ASSEMBLY__) */
> 
> >> +#define GENMASK(h, l)		__GENMASK(h, l)
> >> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> >> +
> >> +#endif /* !defined(__ASSEMBLY__) */
David Laight March 6, 2025, 7:23 p.m. UTC | #6 
On Thu, 06 Mar 2025 20:29:52 +0900
Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:

> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> In an upcoming change, GENMASK() and its friends will indirectly
> depend on sizeof() which is not available in asm.
> 
> Instead of adding further complexity to __GENMASK() to make it work
> for both asm and non asm, just split the definition of the two
> variants.
...
> +#else /* defined(__ASSEMBLY__) */
> +
> +#define GENMASK(h, l)		__GENMASK(h, l)
> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)

What do those actually expand to now?
As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
so the expansions of __GENMASK() and __GENMASK_ULL() contained the
same numeric constants.
This means they should be generating the same values.
I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
My suspicion is that a 32bit assembler used 32bit signed integers and a
64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
be 64bit).
So the asm versions need to avoid the right shift and only do left shifts.

Which probably means they need to be enirely separate from the C versions.
And then the C ones can have all the ULL() removed.

	David

> +
> +#endif /* !defined(__ASSEMBLY__) */
>  
>  #endif	/* __LINUX_BITS_H */
>
diff mbox series

Patch

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,23 +19,17 @@ 
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
 #if !defined(__ASSEMBLY__)
+
 #include <linux/build_bug.h>
 #include <linux/compiler.h>
+
 #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
-#else
-/*
- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
- * disable the input check if that is the case.
- */
-#define GENMASK_INPUT_CHECK(h, l) 0
-#endif
 
 #define GENMASK(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
 #define GENMASK_ULL(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
 
-#if !defined(__ASSEMBLY__)
 /*
  * Missing asm support
  *
@@ -48,6 +42,12 @@ 
  */
 #define GENMASK_U128(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
-#endif
+
+#else /* defined(__ASSEMBLY__) */
+
+#define GENMASK(h, l)		__GENMASK(h, l)
+#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
+
+#endif /* !defined(__ASSEMBLY__) */
 
 #endif	/* __LINUX_BITS_H */