[PATCH] add new DMA_ADDR_T_SIZE define

[PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

Hi Linus,

This patch adds a new preprocessor define called DMA_ADDR_T_SIZE for all 
architectures, for the benefit of those drivers who care about its size 
(and yes, starfire is one of them).

Alternatives are:

1. a really ugly #ifdef in every single driver, which is error-prone and 
likely to break (see drivers/net/starfire.c around line 274 and have a 
barf bag ready).

2. always cast it to u64, which adds unnecessary overhead to 32-bit 
platforms.

3. use run-time checks all over the place, of the 
"sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary overhead to 
all platforms.

4. use the results from pci_set_dma_mask(), which still amounts to 
unnecessary run-time overhead on platforms which have a 32-bit dma_addr_t 
to begin with.

So I think a define in each architecture's types.h file is the cleanest 
way to approach this, and that's what my patch does.

Comments and/or suggestions are appreciated.

Thanks,
Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.
-------------------------------------------------------
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-alpha/types.h linux-2.5.62/include/asm-alpha/types.h
--- linux-2.5.62.vanilla/include/asm-alpha/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-alpha/types.h	Wed Feb 19 10:19:57 2003
@@ -53,6 +53,7 @@
 typedef signed long s64;
 typedef unsigned long u64;
 
+#define DMA_ADDR_T_SIZE 64
 typedef u64 dma_addr_t;
 typedef u64 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-arm/types.h linux-2.5.62/include/asm-arm/types.h
--- linux-2.5.62.vanilla/include/asm-arm/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-arm/types.h	Wed Feb 19 10:19:40 2003
@@ -48,7 +48,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u32 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-cris/types.h linux-2.5.62/include/asm-cris/types.h
--- linux-2.5.62.vanilla/include/asm-cris/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-cris/types.h	Wed Feb 19 10:19:24 2003
@@ -48,7 +48,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide, just like our other addresses.  */
- 
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-i386/types.h linux-2.5.62/include/asm-i386/types.h
--- linux-2.5.62.vanilla/include/asm-i386/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-i386/types.h	Wed Feb 19 10:19:10 2003
@@ -52,8 +52,10 @@
 /* DMA addresses come in generic and 64-bit flavours.  */
 
 #ifdef CONFIG_HIGHMEM
+#define DMA_ADDR_T_SIZE 64
 typedef u64 dma_addr_t;
 #else
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 #endif
 typedef u64 dma64_addr_t;
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-ia64/types.h linux-2.5.62/include/asm-ia64/types.h
--- linux-2.5.62.vanilla/include/asm-ia64/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-ia64/types.h	Wed Feb 19 10:18:31 2003
@@ -62,7 +62,7 @@
 #define BITS_PER_LONG 64
 
 /* DMA addresses are 64-bits wide, in general.  */
-
+#define DMA_ADDR_T_SIZE 64
 typedef u64 dma_addr_t;
 
 # endif /* __KERNEL__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-m68k/types.h linux-2.5.62/include/asm-m68k/types.h
--- linux-2.5.62.vanilla/include/asm-m68k/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-m68k/types.h	Wed Feb 19 10:18:20 2003
@@ -56,7 +56,7 @@
 typedef unsigned long long u64;
 
 /* DMA addresses are always 32-bits wide */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u32 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-m68knommu/types.h linux-2.5.62/include/asm-m68knommu/types.h
--- linux-2.5.62.vanilla/include/asm-m68knommu/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-m68knommu/types.h	Wed Feb 19 10:18:02 2003
@@ -56,7 +56,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-mips/types.h linux-2.5.62/include/asm-mips/types.h
--- linux-2.5.62.vanilla/include/asm-mips/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-mips/types.h	Wed Feb 19 10:17:50 2003
@@ -76,6 +76,7 @@
 
 #endif
 
+#define DMA_ADDR_T_SIZE 32			/* XXX is this right? */
 typedef unsigned long dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-mips64/types.h linux-2.5.62/include/asm-mips64/types.h
--- linux-2.5.62.vanilla/include/asm-mips64/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-mips64/types.h	Wed Feb 19 10:17:05 2003
@@ -75,6 +75,7 @@
 
 #endif
 
+#define DMA_ADDR_T_SIZE 64			/* XXX is this right? */
 typedef unsigned long dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-parisc/types.h linux-2.5.62/include/asm-parisc/types.h
--- linux-2.5.62.vanilla/include/asm-parisc/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-parisc/types.h	Wed Feb 19 10:16:22 2003
@@ -52,7 +52,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u64 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-ppc/types.h linux-2.5.62/include/asm-ppc/types.h
--- linux-2.5.62.vanilla/include/asm-ppc/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-ppc/types.h	Wed Feb 19 10:16:10 2003
@@ -52,6 +52,7 @@
 typedef __vector128 vector128;
 
 /* DMA addresses are 32-bits wide */
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u64 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-ppc64/types.h linux-2.5.62/include/asm-ppc64/types.h
--- linux-2.5.62.vanilla/include/asm-ppc64/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-ppc64/types.h	Wed Feb 19 10:15:43 2003
@@ -63,6 +63,7 @@
 
 typedef __vector128 vector128;
 
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u64 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-s390/types.h linux-2.5.62/include/asm-s390/types.h
--- linux-2.5.62.vanilla/include/asm-s390/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-s390/types.h	Wed Feb 19 10:15:32 2003
@@ -60,6 +60,7 @@
 typedef signed long long s64;
 typedef unsigned long long u64;
 
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 typedef union {
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-s390x/types.h linux-2.5.62/include/asm-s390x/types.h
--- linux-2.5.62.vanilla/include/asm-s390x/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-s390x/types.h	Wed Feb 19 10:15:18 2003
@@ -61,6 +61,7 @@
 typedef signed long s64;
 typedef unsigned  long u64;
 
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-sh/types.h linux-2.5.62/include/asm-sh/types.h
--- linux-2.5.62.vanilla/include/asm-sh/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-sh/types.h	Wed Feb 19 10:15:05 2003
@@ -48,7 +48,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 #endif /* __ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-sparc/types.h linux-2.5.62/include/asm-sparc/types.h
--- linux-2.5.62.vanilla/include/asm-sparc/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-sparc/types.h	Wed Feb 19 10:14:47 2003
@@ -51,6 +51,7 @@
 typedef __signed__ long long s64;
 typedef unsigned long long u64;
 
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u32 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-sparc64/types.h linux-2.5.62/include/asm-sparc64/types.h
--- linux-2.5.62.vanilla/include/asm-sparc64/types.h	Thu Feb 13 15:22:22 2003
+++ linux-2.5.62/include/asm-sparc64/types.h	Wed Feb 19 10:14:31 2003
@@ -52,7 +52,7 @@
 typedef unsigned long u64;
 
 /* Dma addresses come in generic and 64-bit flavours.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 typedef u64 dma64_addr_t;
 
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-v850/types.h linux-2.5.62/include/asm-v850/types.h
--- linux-2.5.62.vanilla/include/asm-v850/types.h	Thu Feb 13 15:22:23 2003
+++ linux-2.5.62/include/asm-v850/types.h	Wed Feb 19 10:13:34 2003
@@ -56,7 +56,7 @@
 typedef unsigned long long u64;
 
 /* Dma addresses are 32-bits wide.  */
-
+#define DMA_ADDR_T_SIZE 32
 typedef u32 dma_addr_t;
 
 #endif /* !__ASSEMBLY__ */
diff -urX diff_kernel_excludes linux-2.5.62.vanilla/include/asm-x86_64/types.h linux-2.5.62/include/asm-x86_64/types.h
--- linux-2.5.62.vanilla/include/asm-x86_64/types.h	Thu Feb 13 15:22:23 2003
+++ linux-2.5.62/include/asm-x86_64/types.h	Wed Feb 19 10:13:21 2003
@@ -45,6 +45,7 @@
 typedef signed long long s64;
 typedef unsigned long long u64;
 
+#define DMA_ADDR_T_SIZE 64
 typedef u64 dma64_addr_t;
 typedef u64 dma_addr_t;
 

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Randy.Dunlap]

On Wed, 19 Feb 2003 11:26:27 -0500 (EST)
Ion Badulescu <ionut@badula.org> wrote:

| This patch adds a new preprocessor define called DMA_ADDR_T_SIZE for all 
| architectures, for the benefit of those drivers who care about its size 
| (and yes, starfire is one of them).
| 
| Alternatives are:
| 
| 1. a really ugly #ifdef in every single driver, which is error-prone and 
| likely to break (see drivers/net/starfire.c around line 274 and have a 
| barf bag ready).
| 
| 2. always cast it to u64, which adds unnecessary overhead to 32-bit 
| platforms.
| 
| 3. use run-time checks all over the place, of the 
| "sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary overhead to 
| all platforms.
| 
| 4. use the results from pci_set_dma_mask(), which still amounts to 
| unnecessary run-time overhead on platforms which have a 32-bit dma_addr_t 
| to begin with.
| 
| So I think a define in each architecture's types.h file is the cleanest 
| way to approach this, and that's what my patch does.
| 
| Comments and/or suggestions are appreciated.
| -- 

Does this help with being able to printk() a <dma_addr_t>?  How?
Always use a cast to (u64) or something else?

--
~Randy

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

On Wed, 19 Feb 2003, Randy.Dunlap wrote:

> Does this help with being able to printk() a <dma_addr_t>?  How?
> Always use a cast to (u64) or something else?

It doesn't help with printk(), but frankly if you're printing a dma_addr_t 
then you're probably debugging, not performance-tuning, so the cast to u64 
is acceptable.

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

On Wed, 2003-02-19 at 09:28, Ion Badulescu wrote:
> then you're probably debugging, not performance-tuning, so the cast to u64 
> is acceptable.

Not true, you must cast to 'long long' as there is no appropriate
printf format string for u64 that works warning-free on all
systems.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

On 19 Feb 2003, David S. Miller wrote:

> On Wed, 2003-02-19 at 09:28, Ion Badulescu wrote:
> > then you're probably debugging, not performance-tuning, so the cast to u64 
> > is acceptable.
> 
> Not true, you must cast to 'long long' as there is no appropriate
> printf format string for u64 that works warning-free on all
> systems.

Yes, long long and %ll[xd] is what I meant, thanks for the correction.

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

On Wed, 2003-02-19 at 09:20, Randy.Dunlap wrote:
> Does this help with being able to printk() a <dma_addr_t>?  How?
> Always use a cast to (u64) or something else?

One should always cast to long long and use %llx.  There is no
printf format appropriate for a 'u64'.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Jeff Garzik]

On Wed, Feb 19, 2003 at 02:06:12PM -0800, David S. Miller wrote:
> On Wed, 2003-02-19 at 09:20, Randy.Dunlap wrote:
> > Does this help with being able to printk() a <dma_addr_t>?  How?
> > Always use a cast to (u64) or something else?
> 
> One should always cast to long long and use %llx.  There is no
> printf format appropriate for a 'u64'.

/me wishes gcc would let the user application define printf formats
for arbitrary [non-struct] user data types...

	Jeff

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ingo Oeser]

Hi there,

On Wed, Feb 19, 2003 at 04:48:17PM -0500, Jeff Garzik wrote:
> /me wishes gcc would let the user application define printf formats
> for arbitrary [non-struct] user data types...

Better might be to change the whole protocol over to sth. like
that what PASCAL does but still print into a string. GCC is
type checking and enumerating the arguments for printf and scanf
anyway so it could also supply that information to the functions
itself, if they have a special attribute (__new_style_printf__).

If GCC needs to print/scan a type, it composes a function name
similiar to C++ signatures and calls this. If it does not exists,
it warns at compile time. The decomposition of constant strings
is done at compile time. There is also a GCC function, which does
it at runtime. This is no problem, since index into type name
table and number of arguments is on the stack along with the
varargs. So we only need to specify the numeric format like
hex/numeric, leading zero fill or not, space fill and so on. No
more fiddling with word sizes and signedness.

Even structures may be printed/assigned, by defining a start_level,
next_item and end_level function to print the equivalent of '{'
',' and '}' in the structure.

I would really vote for such an extension to GCC, since doing
this without the help of GCC is error prone (because it's not
really typesafe) and leads to many problems. 

That way even the config file parsers get much simpler, because
you can define the lexical rules with scanf already today and now
you can even define a simple grammar likewise ;-)

But this is not the right list for this, but I just wanted to
braindump that ;-)

Regards

Ingo Oeser
-- 
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

On Wed, 2003-02-19 at 08:26, Ion Badulescu wrote:
> This patch adds a new preprocessor define called DMA_ADDR_T_SIZE for all 
> architectures, for the benefit of those drivers who care about its size 
> (and yes, starfire is one of them).

I don't think you are making things any better by adding
a new ifdef to all the drivers.

> 2. always cast it to u64, which adds unnecessary overhead to 32-bit 
> platforms.

Not to HIGHMEM ones.  And frankly, trying to super-optimize a driver
because it has two different descriptor format is a mess you as a driver
author choose to get involved in.

Nearly all cards today are 64-bit DMA address descriptors only.
So if anything, this new ifdef will get less and less used over
time.

> 3. use run-time checks all over the place, of the 
> "sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary overhead to 
> all platforms.

The compiler optimizes this completely away, it becomes
a compile time test and the unused code block and the test
never make it into the assembler.

So this argument is bogus, as is the define.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[James Bottomley]

> 3. use run-time checks all over the place, of the 
> "sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary
> overhead to 
> all platforms.

Actually, these aren't technically run time checks.  Although the cpp
can't be used for sizeof(), the compiler (at least gcc) does seem to
have enough sense to optimise away if(..) branches it has enough
information to know won't be taken at compile time.

As long as this optimisation works, I think the if(sizeof(..)) checks
are fine for this.

James

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

On 19 Feb 2003, James Bottomley wrote:

> > 3. use run-time checks all over the place, of the 
> > "sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary
> > overhead to 
> > all platforms.
> 
> Actually, these aren't technically run time checks.  Although the cpp
> can't be used for sizeof(), the compiler (at least gcc) does seem to
> have enough sense to optimise away if(..) branches it has enough
> information to know won't be taken at compile time.
> 
> As long as this optimisation works, I think the if(sizeof(..)) checks
> are fine for this.

Well, yes and no. Indeed those checks are optimized away, but as a result 
of using them most data-access macros must be converted to inline 
functions. And, last I heard at least, gcc was optimizing inline functions 
much worse than preprocessor macros.

There are various other things that are made easier by a preprocessor 
directive -- constructing the right data structures, for instance.

However, if such a patch is ultimately not getting accepted, these checks
is probably what I'll end up using...

Thanks,
Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

On Wed, 2003-02-19 at 10:20, Ion Badulescu wrote:
> Well, yes and no. Indeed those checks are optimized away, but as a result 
> of using them most data-access macros must be converted to inline 
> functions. And, last I heard at least, gcc was optimizing inline functions 
> much worse than preprocessor macros.

Not true, you can still use macros and GCC is saner with inlines
these days.

Your arguments are nice to encourage your patch to be accepted,
they are however not correct.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

On 19 Feb 2003, David S. Miller wrote:

> I don't think you are making things any better by adding
> a new ifdef to all the drivers.

I'm adding an ifdef to types.h, not to all the drivers -- the drivers can 
use it if they choose to.

> And frankly, trying to super-optimize a driver
> because it has two different descriptor format is a mess you as a driver
> author choose to get involved in.

Indeed. But why would you want to make my life miserable if you make that 
choice? Take a look at starfire.c to see how all that 32/64 bit code is 
neatly handled by a few very simple macros, and the only really hideous 
part is the #ifdef which decides if dma_addr_t is 32- or 64-bit.

I would gladly define my own macros if cpp groked sizeof(). But it 
doesn't, that's why I want this simple macro where it belongs: next to the 
definition of dma_addr_t.

> Nearly all cards today are 64-bit DMA address descriptors only.
> So if anything, this new ifdef will get less and less used over
> time.

Not true. Even if the only descriptors available are 64-bit, there is no 
reason why I should have to care about the upper 32 bits when I know that 
my dma_addr_t will always be 32-bit, at compile time. I can simply 
memset(0) the entire descriptor and initialize only the bottom 32 bits. 

What you're saying is, in essence, "screw your card, screw your 32-bit
platform, the future is all-64-bit anyway and/or everyone has gobs of
memory and is using highmem". A little bit exagerated, of course, but...

> > 3. use run-time checks all over the place, of the 
> > "sizeof(dma_addr_t)==sizeof(u64)" kind, which adds unnecessary overhead to 
> > all platforms.
> 
> The compiler optimizes this completely away, it becomes
> a compile time test and the unused code block and the test
> never make it into the assembler.
> 
> So this argument is bogus, as is the define.

It's not completely bogus, although I already conceded that the code is
indeed optimized away by the compiler. What the compiler does not optimize
away is the larger data structures needed to hold 64 bit values.

As I said in a different email, this is what I'll end up using if the new 
define is not accepted.

On 19 Feb 2003, David S. Miller wrote:

> On Wed, 2003-02-19 at 10:20, Ion Badulescu wrote:
> > Well, yes and no. Indeed those checks are optimized away, but as a result 
> > of using them most data-access macros must be converted to inline 
> > functions. And, last I heard at least, gcc was optimizing inline functions 
> > much worse than preprocessor macros.
> 
> Not true, you can still use macros and GCC is saner with inlines
> these days.

I'm curious about this, actually. If I do something like

#if DMA_ADDR_T_SIZE == 64
#define cpu_to_dma(x) cpu_to_le64(x)
#else
#define cpu_to_dma(x) cpu_to_le32(x)
#endif

descr.addr = cpu_to_dma(dma_addr)

or

if (sizeof(dma_addr_t) == sizeof(u64))
	descr64.addr = cpu_to_le64(dma_addr);
else
	descr32.addr = cpu_to_le32(dma_addr);

then the compiler generates equivalent code? Even if that's true, which 
code fragment strikes you as uglier and/or harder to read, particularly if 
it's repeated multiple times in close succession?


Or maybe you are suggesting an abomination like

void assign_dma_addr(descr64_t *descr64, descr32_t *descr32, dma_addt_t dma_addr)
{
	if (sizeof(dma_addr_t) == sizeof(u64))
		descr64->addr = cpu_to_le64(dma_addr);
	else
		descr32->addr = cpu_to_le32(dma_addr);
}

or, worse, the gcc-specific abomination

#define cpu_to_dma(x) \
({ dma_addr_t dma_addr; \
   if (sizeof(dma_addr_t) == sizeof(u64)) \
     dma_addr = cpu_to_le64(x); \
   else \
     dma_addr = cpu_to_le32(x); \
   dma_addr; \
});


And all these just for avoiding a very clearly-defined #ifdef's in a 
header file?...

> Your arguments are nice to encourage your patch to be accepted,
> they are however not correct.

I think you're being a bit harsh here. :) "Some arguments were not 100% 
correct" would be a better description...

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

   From: Ion Badulescu <ionut@badula.org>
   Date: Wed, 19 Feb 2003 17:41:39 -0500 (EST)

   > Nearly all cards today are 64-bit DMA address descriptors only.
   > So if anything, this new ifdef will get less and less used over
   > time.
   
   Not true. Even if the only descriptors available are 64-bit, there is no 
   reason why I should have to care about the upper 32 bits when I know that 
   my dma_addr_t will always be 32-bit, at compile time. I can simply 
   memset(0) the entire descriptor and initialize only the bottom 32 bits. 

Yes true, storing the two consequetive 32-bit values is better
for store buffer compression of the cpu.  Using memset is much
more inefficient because you push the full set of data once
then you push non-compressible stores to the same data through
the cpu.

I'm not talking out of my ass, I've measured this.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Ion Badulescu]

On Wed, 19 Feb 2003, David S. Miller wrote:

> Yes true, storing the two consequetive 32-bit values is better
> for store buffer compression of the cpu.  Using memset is much
> more inefficient because you push the full set of data once
> then you push non-compressible stores to the same data through
> the cpu.
> 
> I'm not talking out of my ass, I've measured this.

So is the current wisdom something like "always treat dma_addr_t as a u64
and be happy"?

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

   From: Ion Badulescu <ionut@badula.org>
   Date: Wed, 19 Feb 2003 17:53:02 -0500 (EST)
   
   So is the current wisdom something like "always treat dma_addr_t as a u64
   and be happy"?

Yes.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Albert Cahalan]

David S. Miller writes:
> On Wed, 2003-02-19 at 09:28, Ion Badulescu wrote:

>> then you're probably debugging, not performance-tuning,
>> so the cast to u64 is acceptable.
>
> Not true, you must cast to 'long long' as there is no
> appropriate printf format string for u64 that works
> warning-free on all systems.

Casts are ugly and they hide bugs. There is a fix
for this problem: make u64 be "unsigned long long"
for every arch. That works for both 32-bit and 64-bit
systems. Likewise, choose "unsigned" for u32 even
if an "unsigned long" would work for a given arch.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[David S. Miller]

   From: Albert Cahalan <albert@users.sourceforge.net>
   Date: 23 Feb 2003 02:02:01 -0500
   
   Casts are ugly and they hide bugs. There is a fix
   for this problem: make u64 be "unsigned long long"
   for every arch. That works for both 32-bit and 64-bit
   systems. Likewise, choose "unsigned" for u32 even
   if an "unsigned long" would work for a given arch.
   
That merely hides the lack of user defined printf types
in gcc, it doesn't make the real problem go away.
What you suggest is merely a bandaid.

Re: [PATCH] add new DMA_ADDR_T_SIZE define

[Albert Cahalan]

On Sun, 2003-02-23 at 02:00, David S. Miller wrote:
>    From: Albert Cahalan <albert@users.sourceforge.net>
>    Date: 23 Feb 2003 02:02:01 -0500
>    
>    Casts are ugly and they hide bugs. There is a fix
>    for this problem: make u64 be "unsigned long long"
>    for every arch. That works for both 32-bit and 64-bit
>    systems. Likewise, choose "unsigned" for u32 even
>    if an "unsigned long" would work for a given arch.
>    
> That merely hides the lack of user defined printf types
> in gcc, it doesn't make the real problem go away.
> What you suggest is merely a bandaid.

Sure. It's an obviously useful bandaid that works
with gcc 2.91, 2.92, 2.95, 2.96, 3.0, 3.2, 3.3, etc.