[PATCH] Fix 64bit bugs in dscc44.c

[PATCH] Fix 64bit bugs in dscc44.c

[Andi Kleen]

ioremap_nocache returns unsigned long, not u32. This makes a difference
on 64bit hosts.

-Andi

diff -u linux-2.5-cleanup/drivers/net/wan/dscc4.c-o linux-2.5-cleanup/drivers/net/wan/dscc4.c
--- linux-2.5-cleanup/drivers/net/wan/dscc4.c-o	2003-12-01 14:04:34.000000000 +0100
+++ linux-2.5-cleanup/drivers/net/wan/dscc4.c	2003-12-02 16:55:14.298508864 +0100
@@ -980,7 +980,7 @@
  *
  * This code doesn't need to be efficient. Keep It Simple
  */
-static void dscc4_pci_reset(struct pci_dev *pdev, u32 ioaddr)
+static void dscc4_pci_reset(struct pci_dev *pdev, unsigned long ioaddr)
 {
 	int i;
 
@@ -1461,7 +1461,8 @@
 	struct dscc4_dev_priv *root = token;
 	struct dscc4_pci_priv *priv;
 	struct net_device *dev;
-	u32 ioaddr, state;
+	unsigned long ioaddr;
+	u32 state;
 	unsigned long flags;
 	int i, handled = 1;
 
@@ -1613,7 +1614,7 @@
 				goto try;
 		}
 		if (state & Xpr) {
-			u32 scc_addr, ring;
+			unsigned long scc_addr, ring;
 			int i;
 
 			/*
@@ -1954,7 +1955,7 @@
 {
 	struct dscc4_pci_priv *ppriv;
 	struct dscc4_dev_priv *root;
-	u32 ioaddr;
+	unsigned long ioaddr;
 	int i;
 
 	ppriv = pci_get_drvdata(pdev);

Re: [PATCH] Fix 64bit bugs in dscc4.c

[Francois Romieu]

Andi Kleen <ak@muc.de> :
[...]
> ioremap_nocache returns unsigned long, not u32. This makes a difference
> on 64bit hosts.

No 64 bit host to test the patch but it looks fine.

--
Ueimor - happily maintaining dscc4 driver since 2.4.3

Re: [PATCH] Fix 64bit bugs in dscc44.c

[David S. Miller]

On Tue, 7 Oct 2003 19:59:53 +0200
Andi Kleen <ak@muc.de> wrote:

> ioremap_nocache returns unsigned long, not u32. This makes a difference
> on 64bit hosts.

Incorrect, like ioremap() it returns "void *".

Even your x86_64 implementation does this.

Re: [PATCH] Fix 64bit bugs in dscc44.c

[Andi Kleen]

On Wed, Oct 08, 2003 at 08:42:05AM -0700, David S. Miller wrote:
> On Tue, 7 Oct 2003 19:59:53 +0200
> Andi Kleen <ak@muc.de> wrote:
> 
> > ioremap_nocache returns unsigned long, not u32. This makes a difference
> > on 64bit hosts.
> 
> Incorrect, like ioremap() it returns "void *".

I knew someone would nit pick this ;-)

I agree it would have been better to write:
"ioremap and ioremap_nocache return pointers and that should 
be stored in a pointer variable. However when you really want 
to store them in a integer variable for unknown reasons always
use unsigned long, not u32 or int"

However that was just too long, so I didn't write it.

-Andi

Re: [PATCH] Fix 64bit bugs in dscc44.c

[David S. Miller]

On Wed, 8 Oct 2003 17:55:48 +0200
Andi Kleen <ak@suse.de> wrote:

> I agree it would have been better to write:
> "ioremap and ioremap_nocache return pointers and that should 
> be stored in a pointer variable. However when you really want 
> to store them in a integer variable for unknown reasons always
> use unsigned long, not u32 or int"
> 
> However that was just too long, so I didn't write it.

Don't replace one error with a new warning, just add the cast
to the ioremap() call or something like that.

Re: [PATCH] Fix 64bit bugs in dscc44.c

[Andi Kleen]

On Wed, Oct 08, 2003 at 09:01:32AM -0700, David S. Miller wrote:
> On Wed, 8 Oct 2003 17:55:48 +0200
> Andi Kleen <ak@suse.de> wrote:
> 
> > I agree it would have been better to write:
> > "ioremap and ioremap_nocache return pointers and that should 
> > be stored in a pointer variable. However when you really want 
> > to store them in a integer variable for unknown reasons always
> > use unsigned long, not u32 or int"
> > 
> > However that was just too long, so I didn't write it.
> 
> Don't replace one error with a new warning, just add the cast
> to the ioremap() call or something like that.

I don't particularly care about the warning, just that it will
obviously crash on a 64bit box when loaded. My change was the simplest 
possible fix for that. I have no plans to rewrite this driver
or something.

-Andi

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[David S. Miller]

> BTW my change fixed the warning.

I know, but it added a new one.

> It's somewhat ugly to store pointers in unsigned long I agree,

I have _NO_ problem with this, all I'm asking you to do is:

-	foo = ioremap_nocache(...);
+ 	foo = (unsigned long) ioremap_nocache(...);

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[Andi Kleen]

> I don't particularly care about the warning, just that it will
> obviously crash on a 64bit box when loaded. My change was the simplest 
> possible fix for that. I have no plans to rewrite this driver
> or something.

BTW my change fixed the warning.

It's somewhat ugly to store pointers in unsigned long I agree,
but it's hard to blame the driver author when this cruft is even
enshrined in fundamental interfaces (__get_free_pages/__free_pages) 

-Andi

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[David S. Miller]

On Wed, 8 Oct 2003 18:33:45 +0200
Andi Kleen <ak@suse.de> wrote:

> Just do that then when you apply the patch. Would have been faster
> than to write all these emails ;-)

I deleted the original patch, so you have to resend it to
me anyways, so please make the fix I've suggested as you
do this.

When I ask you to do these small fixes, I don't do it to make your
life miserable, I do it because it allows me to review and apply
patches at a higher rate and scale better.

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[Andi Kleen]

On Wed, Oct 08, 2003 at 09:10:50AM -0700, David S. Miller wrote:
> > BTW my change fixed the warning.
> 
> I know, but it added a new one.
> 
> > It's somewhat ugly to store pointers in unsigned long I agree,
> 
> I have _NO_ problem with this, all I'm asking you to do is:
> 
> -	foo = ioremap_nocache(...);
> + 	foo = (unsigned long) ioremap_nocache(...);

Just do that then when you apply the patch. Would have been faster
than to write all these emails ;-)

-Andi

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[David S. Miller]

On Wed, 8 Oct 2003 19:00:05 +0200
Andi Kleen <ak@suse.de> wrote:

> Here's the patch again.

Applied, thanks Andi.

Re: [PATCH] Fix 64bit bugs in dscc44.c II

[Andi Kleen]

On Wed, Oct 08, 2003 at 09:31:53AM -0700, David S. Miller wrote:
> On Wed, 8 Oct 2003 18:33:45 +0200
> Andi Kleen <ak@suse.de> wrote:
> 
> > Just do that then when you apply the patch. Would have been faster
> > than to write all these emails ;-)
> 
> I deleted the original patch, so you have to resend it to
> me anyways, so please make the fix I've suggested as you
> do this.
> 
> When I ask you to do these small fixes, I don't do it to make your
> life miserable, I do it because it allows me to review and apply
> patches at a higher rate and scale better.

At least for such one liner changes it doesn't look like an effective
method (I bet you could have done the change quicker than typing
the first mail).  

I checked the code and the code was already using a unsigned long
cast, so no changes are needed. Just the variable it stored the
pointer in was wrong.

Here's the patch again.

---------------------------

Fix 64bit issues in dscc4


diff -u linux-2.5-cleanup/drivers/net/wan/dscc4.c-o linux-2.5-cleanup/drivers/net/wan/dscc4.c
--- linux-2.5-cleanup/drivers/net/wan/dscc4.c-o	2003-12-01 14:04:34.000000000 +0100
+++ linux-2.5-cleanup/drivers/net/wan/dscc4.c	2003-12-02 16:55:14.298508864 +0100
@@ -980,7 +980,7 @@
  *
  * This code doesn't need to be efficient. Keep It Simple
  */
-static void dscc4_pci_reset(struct pci_dev *pdev, u32 ioaddr)
+static void dscc4_pci_reset(struct pci_dev *pdev, unsigned long ioaddr)
 {
 	int i;
 
@@ -1461,7 +1461,8 @@
 	struct dscc4_dev_priv *root = token;
 	struct dscc4_pci_priv *priv;
 	struct net_device *dev;
-	u32 ioaddr, state;
+	unsigned long ioaddr;
+	u32 state;
 	unsigned long flags;
 	int i, handled = 1;
 
@@ -1613,7 +1614,7 @@
 				goto try;
 		}
 		if (state & Xpr) {
-			u32 scc_addr, ring;
+			unsigned long scc_addr, ring;
 			int i;
 
 			/*
@@ -1954,7 +1955,7 @@
 {
 	struct dscc4_pci_priv *ppriv;
 	struct dscc4_dev_priv *root;
-	u32 ioaddr;
+	unsigned long ioaddr;
 	int i;
 
 	ppriv = pci_get_drvdata(pdev);