[PATCH] introduce lower_32_bits() macro

[PATCH] introduce lower_32_bits() macro

[Joerg Roedel]

The file kernel.h contains the upper_32_bits macro. This patch adds the other
part, the lower_32_bits macro. Its first use will be in the driver for AMD
IOMMU.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 include/linux/kernel.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f9cd7a5..6fd2977 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -73,6 +73,12 @@ extern const char linux_proc_banner[];
  */
 #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
 
+/**
+ * lower_32_bits - return bits 0-31 of a number
+ * @n: the number we're accessing
+ */
+#define lower_32_bits(n) ((n) & 0xffffffffULL)
+
 #define	KERN_EMERG	"<0>"	/* system is unusable			*/
 #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
 #define	KERN_CRIT	"<2>"	/* critical conditions			*/
-- 
1.5.3.7

Re: [PATCH] introduce lower_32_bits() macro

[H. Peter Anvin]

Joerg Roedel wrote:
> The file kernel.h contains the upper_32_bits macro. This patch adds the other
> part, the lower_32_bits macro. Its first use will be in the driver for AMD
> IOMMU.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  include/linux/kernel.h |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index f9cd7a5..6fd2977 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -73,6 +73,12 @@ extern const char linux_proc_banner[];
>   */
>  #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
>  
> +/**
> + * lower_32_bits - return bits 0-31 of a number
> + * @n: the number we're accessing
> + */
> +#define lower_32_bits(n) ((n) & 0xffffffffULL)
> +

NAK.  These are assymmetric with regards to type, which is the *last* 
thing we want.

The symmetric definition would be ((u32)(n)), but that's already 
idiomatic use, so why not use it as-is?

	-hpa

Re: [PATCH] introduce lower_32_bits() macro

[Andrew Morton]

On Fri, 25 Jul 2008 12:06:27 -0400
"H. Peter Anvin" <hpa@zytor.com> wrote:

> Joerg Roedel wrote:
> > The file kernel.h contains the upper_32_bits macro. This patch adds the other
> > part, the lower_32_bits macro. Its first use will be in the driver for AMD
> > IOMMU.
> > 
> > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> > ---
> >  include/linux/kernel.h |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index f9cd7a5..6fd2977 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -73,6 +73,12 @@ extern const char linux_proc_banner[];
> >   */
> >  #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
> >  
> > +/**
> > + * lower_32_bits - return bits 0-31 of a number
> > + * @n: the number we're accessing
> > + */
> > +#define lower_32_bits(n) ((n) & 0xffffffffULL)
> > +
> 
> NAK.  These are assymmetric with regards to type, which is the *last* 
> thing we want.

Yes, it will convert a 32-bit expression into a 64-bit one.

> The symmetric definition would be ((u32)(n)), but that's already 
> idiomatic use, so why not use it as-is?

There's some readability benefit.  Sometimes it is hard to understand
why some random open-coded cast was used.  But I seem to recall that
there was another reason why we decided we needed this.  I forget, and
so apparently did the changelog author ;)

Re: [PATCH] introduce lower_32_bits() macro

[Roland Dreier]

 > There's some readability benefit.  Sometimes it is hard to understand
 > why some random open-coded cast was used.  But I seem to recall that
 > there was another reason why we decided we needed this.  I forget, and
 > so apparently did the changelog author ;)

The guy who added the macro explained it in a comment ;)

 * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
 * the "right shift count >= width of type" warning when that quantity is
 * 32-bits.

 - R.

Re: [PATCH] introduce lower_32_bits() macro

[H. Peter Anvin]

Andrew Morton wrote:
>>>   */
>>>  #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
>>>  
>>> +/**
>>> + * lower_32_bits - return bits 0-31 of a number
>>> + * @n: the number we're accessing
>>> + */
>>> +#define lower_32_bits(n) ((n) & 0xffffffffULL)
>>> +
>> NAK.  These are assymmetric with regards to type, which is the *last* 
>> thing we want.
> 
> Yes, it will convert a 32-bit expression into a 64-bit one.
> 

Sort of.  upper_32_bits() takes a 32- or 64-bit expression, and delivers 
the upper 32 bits *as a 32-bit number*.  An equivalent expression would be:

	((u32)((u64)(n) >> 32))

... which might actually produce better code, for all I know.

Logically speaking, if we have a lower_32_bits() macro, it should also 
produce a 32-bit type as result.

	-hpa

Re: [PATCH] introduce lower_32_bits() macro

[H. Peter Anvin]

Roland Dreier wrote:
>  > There's some readability benefit.  Sometimes it is hard to understand
>  > why some random open-coded cast was used.  But I seem to recall that
>  > there was another reason why we decided we needed this.  I forget, and
>  > so apparently did the changelog author ;)
> 
> The guy who added the macro explained it in a comment ;)
> 
>  * A basic shift-right of a 64- or 32-bit quantity.  Use this to suppress
>  * the "right shift count >= width of type" warning when that quantity is
>  * 32-bits.

That was for upper_32_bits() -- and can also be done with an internal 
(u64) cast; it would be interesting to find out if that would generate 
better code.

	-hpa

Re: [PATCH] introduce lower_32_bits() macro

[Joerg Roedel]

On Fri, Jul 25, 2008 at 12:06:27PM -0400, H. Peter Anvin wrote:
> Joerg Roedel wrote:
> >The file kernel.h contains the upper_32_bits macro. This patch adds the other
> >part, the lower_32_bits macro. Its first use will be in the driver for AMD
> >IOMMU.
> >Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> >---
> > include/linux/kernel.h |    6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> >index f9cd7a5..6fd2977 100644
> >--- a/include/linux/kernel.h
> >+++ b/include/linux/kernel.h
> >@@ -73,6 +73,12 @@ extern const char linux_proc_banner[];
> >  */
> > #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
> > +/**
> >+ * lower_32_bits - return bits 0-31 of a number
> >+ * @n: the number we're accessing
> >+ */
> >+#define lower_32_bits(n) ((n) & 0xffffffffULL)
> >+
> 
> NAK.  These are assymmetric with regards to type, which is the *last*
> thing we want.
> 
> The symmetric definition would be ((u32)(n)), but that's already
> idiomatic use, so why not use it as-is?

Ok, true. Sorry for the broken patch. I will send an updated one.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

[PATCH] introduce lower_32_bits() macro

[Joerg Roedel]

The file kernel.h contains the upper_32_bits macro. This patch adds the other
part, the lower_32_bits macro. Its first use will be in the driver for AMD
IOMMU.

Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 include/linux/kernel.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fdbbf72..aaa998f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -75,6 +75,12 @@ extern const char linux_proc_banner[];
  */
 #define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
 
+/**
+ * lower_32_bits - return bits 0-31 of a number
+ * @n: the number we're accessing
+ */
+#define lower_32_bits(n) ((u32)(n))
+
 #define	KERN_EMERG	"<0>"	/* system is unusable			*/
 #define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
 #define	KERN_CRIT	"<2>"	/* critical conditions			*/
-- 
1.5.3.7