[PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Borislav Petkov]

These patches remove redundant DMA_..BIT_MASK definitions across two
drivers. The computation of the majority of the bitmasks is done by the
compiler. The initial split of the patch touching each a different file got
removed due to possible git bisect breakage.

Andrew, can you please apply this patch for it touches drivers maintained by
different people and i there might be responsibility issues, imho.

Signed-off-by: Borislav Petkov <bbpetkov@yahoo.de>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Muli Ben-Yehuda <muli@il.ibm.com>

 drivers/net/netxen/netxen_nic_main.c |    3 ---
 drivers/scsi/gdth.c                  |    5 -----
 include/linux/dma-mapping.h          |   23 +++++++++++++----------
 3 files changed, 13 insertions(+), 18 deletions(-)

--
Index: b/include/linux/dma-mapping.h
===================================================================
--- a/include/linux/dma-mapping.h	2007-09-18 21:12:30.000000000 +0200
+++ b/include/linux/dma-mapping.h	2007-09-18 21:13:17.000000000 +0200
@@ -13,16 +13,19 @@
 	DMA_NONE = 3,
 };
 
-#define DMA_64BIT_MASK	0xffffffffffffffffULL
-#define DMA_48BIT_MASK	0x0000ffffffffffffULL
-#define DMA_40BIT_MASK	0x000000ffffffffffULL
-#define DMA_39BIT_MASK	0x0000007fffffffffULL
-#define DMA_32BIT_MASK	0x00000000ffffffffULL
-#define DMA_31BIT_MASK	0x000000007fffffffULL
-#define DMA_30BIT_MASK	0x000000003fffffffULL
-#define DMA_29BIT_MASK	0x000000001fffffffULL
-#define DMA_28BIT_MASK	0x000000000fffffffULL
-#define DMA_24BIT_MASK	0x0000000000ffffffULL
+#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
+
+#define DMA_64BIT_MASK	(~0ULL)
+#define DMA_48BIT_MASK	DMA_BIT_MASK(48)
+#define DMA_40BIT_MASK	DMA_BIT_MASK(40)
+#define DMA_39BIT_MASK	DMA_BIT_MASK(39)
+#define DMA_35BIT_MASK	DMA_BIT_MASK(35)
+#define DMA_32BIT_MASK	DMA_BIT_MASK(32)
+#define DMA_31BIT_MASK	DMA_BIT_MASK(31)
+#define DMA_30BIT_MASK	DMA_BIT_MASK(30)
+#define DMA_29BIT_MASK	DMA_BIT_MASK(29)
+#define DMA_28BIT_MASK	DMA_BIT_MASK(28)
+#define DMA_24BIT_MASK	DMA_BIT_MASK(24)
 
 static inline int valid_dma_direction(int dma_direction)
 {
Index: b/drivers/net/netxen/netxen_nic_main.c
===================================================================
--- a/drivers/net/netxen/netxen_nic_main.c	2007-09-18 21:13:06.000000000 +0200
+++ b/drivers/net/netxen/netxen_nic_main.c	2007-09-18 21:13:46.000000000 +0200
@@ -54,9 +54,6 @@
 #define NETXEN_ADAPTER_UP_MAGIC 777
 #define NETXEN_NIC_PEG_TUNE 0
 
-#define DMA_32BIT_MASK	0x00000000ffffffffULL
-#define DMA_35BIT_MASK	0x00000007ffffffffULL
-
 /* Local functions to NetXen NIC driver */
 static int __devinit netxen_nic_probe(struct pci_dev *pdev,
 				      const struct pci_device_id *ent);
Index: b/drivers/scsi/gdth.c
===================================================================
--- a/drivers/scsi/gdth.c	2007-09-18 21:12:43.000000000 +0200
+++ b/drivers/scsi/gdth.c	2007-09-18 21:14:06.000000000 +0200
@@ -392,12 +392,7 @@
 #include <linux/proc_fs.h>
 #include <linux/time.h>
 #include <linux/timer.h>
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,6)
 #include <linux/dma-mapping.h>
-#else
-#define DMA_32BIT_MASK	0x00000000ffffffffULL
-#define DMA_64BIT_MASK	0xffffffffffffffffULL
-#endif
 
 #ifdef GDTH_RTC
 #include <linux/mc146818rtc.h>

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Satyam Sharma]

On Tue, 18 Sep 2007, Borislav Petkov wrote:
> 
> These patches remove redundant DMA_..BIT_MASK definitions across two
> drivers. The computation of the majority of the bitmasks is done by the
> compiler. The initial split of the patch touching each a different file got
> removed due to possible git bisect breakage.
> 
> Andrew, can you please apply this patch for it touches drivers maintained by
> different people and i there might be responsibility issues, imho.
> 
> Signed-off-by: Borislav Petkov <bbpetkov@yahoo.de>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Muli Ben-Yehuda <muli@il.ibm.com>

Thanks, Borislav.

Reviewed-by: Satyam Sharma <satyam@infradead.org>

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Andrew Morton]

On Tue, 18 Sep 2007 21:46:47 +0200
Borislav Petkov <bbpetkov@yahoo.de> wrote:

> These patches remove redundant DMA_..BIT_MASK definitions across two
> drivers. The computation of the majority of the bitmasks is done by the
> compiler. The initial split of the patch touching each a different file got
> removed due to possible git bisect breakage.
> 
> Andrew, can you please apply this patch for it touches drivers maintained by
> different people and i there might be responsibility issues, imho.
> 
> Signed-off-by: Borislav Petkov <bbpetkov@yahoo.de>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Muli Ben-Yehuda <muli@il.ibm.com>
> 
>  drivers/net/netxen/netxen_nic_main.c |    3 ---
>  drivers/scsi/gdth.c                  |    5 -----
>  include/linux/dma-mapping.h          |   23 +++++++++++++----------
>  3 files changed, 13 insertions(+), 18 deletions(-)
> 
> ...
>
> -#define DMA_64BIT_MASK	0xffffffffffffffffULL
> -#define DMA_48BIT_MASK	0x0000ffffffffffffULL
> -#define DMA_40BIT_MASK	0x000000ffffffffffULL
> -#define DMA_39BIT_MASK	0x0000007fffffffffULL
> -#define DMA_32BIT_MASK	0x00000000ffffffffULL
> -#define DMA_31BIT_MASK	0x000000007fffffffULL
> -#define DMA_30BIT_MASK	0x000000003fffffffULL
> -#define DMA_29BIT_MASK	0x000000001fffffffULL
> -#define DMA_28BIT_MASK	0x000000000fffffffULL
> -#define DMA_24BIT_MASK	0x0000000000ffffffULL
> +#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
> +
> +#define DMA_64BIT_MASK	(~0ULL)
> +#define DMA_48BIT_MASK	DMA_BIT_MASK(48)
> +#define DMA_40BIT_MASK	DMA_BIT_MASK(40)
> +#define DMA_39BIT_MASK	DMA_BIT_MASK(39)
> +#define DMA_35BIT_MASK	DMA_BIT_MASK(35)
> +#define DMA_32BIT_MASK	DMA_BIT_MASK(32)
> +#define DMA_31BIT_MASK	DMA_BIT_MASK(31)
> +#define DMA_30BIT_MASK	DMA_BIT_MASK(30)
> +#define DMA_29BIT_MASK	DMA_BIT_MASK(29)
> +#define DMA_28BIT_MASK	DMA_BIT_MASK(28)
> +#define DMA_24BIT_MASK	DMA_BIT_MASK(24)
>  

Now that you've done this, those DMA_xxBIT_MASK macros are pointless and
stupid and we should aim to get rid of them.


From: Andrew Morton <akpm@linux-foundation.org>

Now that we have DMA_BIT_MASK(), these macros are pointless.

Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/dma-mapping.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff -puN include/linux/dma-mapping.h~a include/linux/dma-mapping.h
--- a/include/linux/dma-mapping.h~a
+++ a/include/linux/dma-mapping.h
@@ -15,6 +15,12 @@ enum dma_data_direction {
 
 #define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
 
+/*
+ * NOTE: do not use the below macros in new code and do not add new definitions
+ * here.
+ *
+ * Instead, just open-code DMA_BIT_MASK(n) within your driver
+ */
 #define DMA_64BIT_MASK	(~0ULL)
 #define DMA_48BIT_MASK	DMA_BIT_MASK(48)
 #define DMA_47BIT_MASK	DMA_BIT_MASK(47)
_

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
>
> Now that we have DMA_BIT_MASK(), these macros are pointless.
>   

Except, unfortunately, DMA_64BIT_MASK.  I guess we could special case
it, assuming this works in all the contexts the macro is used in (ie,
compile-time constant?):

#define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))



    J

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Andrew Morton]

On Fri, 05 Oct 2007 13:43:54 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> >
> > Now that we have DMA_BIT_MASK(), these macros are pointless.
> >   
> 
> Except, unfortunately, DMA_64BIT_MASK.  I guess we could special case
> it, assuming this works in all the contexts the macro is used in (ie,
> compile-time constant?):
> 
> #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> 

doh.  Thanks.

--- a/include/linux/dma-mapping.h~stop-using-dma_xxbit_mask-fix
+++ a/include/linux/dma-mapping.h
@@ -13,7 +13,7 @@ enum dma_data_direction {
 	DMA_NONE = 3,
 };
 
-#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
+#define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
 
 /*
  * NOTE: do not use the below macros in new code and do not add new definitions
@@ -21,7 +21,7 @@ enum dma_data_direction {
  *
  * Instead, just open-code DMA_BIT_MASK(n) within your driver
  */
-#define DMA_64BIT_MASK	(~0ULL)
+#define DMA_64BIT_MASK	DMA_BIT_MASK(64)
 #define DMA_48BIT_MASK	DMA_BIT_MASK(48)
 #define DMA_47BIT_MASK	DMA_BIT_MASK(47)
 #define DMA_40BIT_MASK	DMA_BIT_MASK(40)
_


it's irksome that there doesn't seem to be a neater way of doing
this, until they give us unsigned long long longs.

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Robert P. J. Day]

On Fri, 5 Oct 2007, Andrew Morton wrote:

> -#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
> +#define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))

or you could take advantage of the macros in kernel.h and write that
as:

+#define DMA_BIT_MASK(n)      (((n) == 64) ? ULLONG_MAX : ((1ULL<<(n))-1))

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Jeremy Fitzhardinge]

Robert P. J. Day wrote:
> or you could take advantage of the macros in kernel.h and write that
> as:
>
> +#define DMA_BIT_MASK(n)      (((n) == 64) ? ULLONG_MAX : ((1ULL<<(n))-1))
>   

But that's a more indirect way of expressing "I want all 1's".

    J

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Andreas Schwab]

"Robert P. J. Day" <rpjday@mindspring.com> writes:

> On Fri, 5 Oct 2007, Andrew Morton wrote:
>
>> -#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
>> +#define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
>
> or you could take advantage of the macros in kernel.h and write that
> as:
>
> +#define DMA_BIT_MASK(n)      (((n) == 64) ? ULLONG_MAX : ((1ULL<<(n))-1))

#define DMA_BIT_MASK(n)         ((u64)-1 >> (64 - (n)))

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Jeremy Fitzhardinge]

Andreas Schwab wrote:
> #define DMA_BIT_MASK(n)         ((u64)-1 >> (64 - (n)))
>   

Yeah, that's cleaner.

    J

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Andrew Morton]

On Fri, 05 Oct 2007 14:28:45 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andreas Schwab wrote:
> > #define DMA_BIT_MASK(n)         ((u64)-1 >> (64 - (n)))
> >   
> 
> Yeah, that's cleaner.
> 

Well yes, but DMA_BIT_MASK(0) invokes undefined behaviour, generates a
compiler warning and evaluates to 0xffffffffffffffff (with my setup).

That won't be a problem in practice, but it is strictly wrong and doesn't set
a good exmaple for the children ;)

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> Well yes, but DMA_BIT_MASK(0) invokes undefined behaviour, generates a
> compiler warning and evaluates to 0xffffffffffffffff (with my setup).
>
> That won't be a problem in practice, but it is strictly wrong and doesn't set
> a good exmaple for the children ;)
>   

It's interesting that it doesn't seem to be possible to define this
without invoking some undefined behaviour.  But a device that supports 0
bits of DMA address probably isn't terribly concerned about this - it's
certainly better than making 64 bit masks warty.

    J

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Borislav Petkov]

On Fri, Oct 05, 2007 at 02:23:41PM -0700, Jeremy Fitzhardinge wrote:
> Robert P. J. Day wrote:
> > or you could take advantage of the macros in kernel.h and write that
> > as:
> >
> > +#define DMA_BIT_MASK(n)      (((n) == 64) ? ULLONG_MAX : ((1ULL<<(n))-1))
> >   
> 
> But that's a more indirect way of expressing "I want all 1's".
... and ULLONG_MAX _is_ (~0ULL).

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 1/1] unify DMA_..BIT_MASK definitions: v3.1

[Borislav Petkov]

On Fri, Oct 05, 2007 at 02:00:50PM -0700, Andrew Morton wrote:
> On Fri, 05 Oct 2007 13:43:54 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> 
> > Andrew Morton wrote:
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > >
> > > Now that we have DMA_BIT_MASK(), these macros are pointless.
> > >   
> > 
> > Except, unfortunately, DMA_64BIT_MASK.  I guess we could special case
> > it, assuming this works in all the contexts the macro is used in (ie,
> > compile-time constant?):
> > 
> > #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
> > 
> 
> doh.  Thanks.
> 
> --- a/include/linux/dma-mapping.h~stop-using-dma_xxbit_mask-fix
> +++ a/include/linux/dma-mapping.h
> @@ -13,7 +13,7 @@ enum dma_data_direction {
>  	DMA_NONE = 3,
>  };
>  
> -#define DMA_BIT_MASK(n)	((1ULL<<(n))-1)
> +#define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
>  
>  /*
>   * NOTE: do not use the below macros in new code and do not add new definitions
> @@ -21,7 +21,7 @@ enum dma_data_direction {
>   *
>   * Instead, just open-code DMA_BIT_MASK(n) within your driver
>   */
> -#define DMA_64BIT_MASK	(~0ULL)
> +#define DMA_64BIT_MASK	DMA_BIT_MASK(64)
>  #define DMA_48BIT_MASK	DMA_BIT_MASK(48)
>  #define DMA_47BIT_MASK	DMA_BIT_MASK(47)
>  #define DMA_40BIT_MASK	DMA_BIT_MASK(40)
>

Hi and sorry for the late reply. IMHO, this solution is the most concise and still
clear enough a macro to understand what it does after a quick scan (unlike
uglies like IO_COND() in lib/iomap.c¹, :)) And, as a next step, we probably
should do

perl -pi -e 's/DMA_(..)BIT_MASK/DMA_BIT_MASK($1)/g' *

after removing the #define DMA_..BIT_MASK defines in /include/linux/dma-mapping.h
and the other two headers in the original patch after the x86 merge.
Current git (9f34073b4e54ad58541e0e2b4a87f4f6c1460e21) contains about 394
instances of usage of those macros, including the #definitions.


¹ this is not a flame!
-- 
Regards/Gruß,
    Boris.