Skip to contentSkip to conversation

Fix 64bit bugs in dscc44.c

PATCH Applied Andi Kleen 4 participants 12 messages Oct 7, 2003 – Oct 8, 2003

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Fix 64bit bugs in dscc44.c

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

-Andi

drivers/net/wan/dscc4.c renamed from drivers/net/wan/dscc4.c-o +5 -4
diff -u linux-2.5-cleanup/drivers/net/wan/dscc4.c-o linux-2.5-cleanup/drivers/net/wan/dscc4.c
@@ -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);
#1 lore raw

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

Andi Kleen <[email protected]> :
[...]

Quoted 2 lines

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

#2 lore raw
In reply to Andi Kleen
Quoted 2 lines · On Tue, 7 Oct 2003 19:59:53 +0200 Andi Kleen <[email protected]> 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.

#3 lore raw
Quoted 7 lines · David S. Miller
Quoted 2 lines · On Tue, 7 Oct 2003 19:59:53 +0200 Andi Kleen <[email protected]> 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

#4 lore raw
Quoted 7 lines · On Wed, 8 Oct 2003 17:55:48 +0200 Andi Kleen <[email protected]> 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.

#5 lore raw
Quoted 9 lines · David S. Miller
Quoted 6 lines · On Wed, 8 Oct 2003 17:55:48 +0200 Andi Kleen <[email protected]> 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"

Quoted 4 lines
Quoted 1 line

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

#6 lore raw

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

Quoted 4 lines

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

#7 lore raw

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

Quoted 1 line

BTW my change fixed the warning.

I know, but it added a new one.

Quoted 1 line

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(...);

#8 lore raw

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

Review

Quoted 10 lines · David S. Miller
Quoted 1 line

BTW my change fixed the warning.

I know, but it added a new one.

Quoted 1 line

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:

(quoted diff) +1 -1
-	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

#9 lore raw

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

Quoted 2 lines · On Wed, 8 Oct 2003 18:33:45 +0200 Andi Kleen <[email protected]> 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.

#10 lore raw

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

Quoted 6 lines · David S. Miller
Quoted 2 lines · On Wed, 8 Oct 2003 18:33:45 +0200 Andi Kleen <[email protected]> wrote:

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

Quoted 7 lines

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

drivers/net/wan/dscc4.c renamed from drivers/net/wan/dscc4.c-o +5 -4
diff -u linux-2.5-cleanup/drivers/net/wan/dscc4.c-o linux-2.5-cleanup/drivers/net/wan/dscc4.c
@@ -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);
Applied
#11 lore raw

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

Quoted 1 line · On Wed, 8 Oct 2003 19:00:05 +0200 Andi Kleen <[email protected]> wrote:

Here's the patch again.

Applied, thanks Andi.