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 <[email protected]>
Cc: Jeremy Fitzhardinge <[email protected]>
Cc: Muli Ben-Yehuda <[email protected]>
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.
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 <[email protected]>
> Cc: Jeremy Fitzhardinge <[email protected]>
> Cc: Muli Ben-Yehuda <[email protected]>
Thanks, Borislav.
Reviewed-by: Satyam Sharma <[email protected]>
On Tue, 18 Sep 2007 21:46:47 +0200
Borislav Petkov <[email protected]> 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 <[email protected]>
> Cc: Jeremy Fitzhardinge <[email protected]>
> Cc: Muli Ben-Yehuda <[email protected]>
>
> 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 <[email protected]>
Now that we have DMA_BIT_MASK(), these macros are pointless.
Signed-off-by: Andrew Morton <[email protected]>
---
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)
_
Andrew Morton wrote:
> From: Andrew Morton <[email protected]>
>
> 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
On Fri, 05 Oct 2007 13:43:54 -0700
Jeremy Fitzhardinge <[email protected]> wrote:
> Andrew Morton wrote:
> > From: Andrew Morton <[email protected]>
> >
> > 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.
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
========================================================================
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
"Robert P. J. Day" <[email protected]> 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, [email protected]
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."
Andreas Schwab wrote:
> #define DMA_BIT_MASK(n) ((u64)-1 >> (64 - (n)))
>
Yeah, that's cleaner.
J
On Fri, 05 Oct 2007 14:28:45 -0700
Jeremy Fitzhardinge <[email protected]> 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 ;)
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
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.
On Fri, Oct 05, 2007 at 02:00:50PM -0700, Andrew Morton wrote:
> On Fri, 05 Oct 2007 13:43:54 -0700
> Jeremy Fitzhardinge <[email protected]> wrote:
>
> > Andrew Morton wrote:
> > > From: Andrew Morton <[email protected]>
> > >
> > > 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.