spidev compiler warning gone

PATCH In review David Brownell 2 participants 6 messages May 25, 2007 – May 30, 2007 5 messages not in the archive SPI

Share & tools

David Brownell

author

2007-05-25 17:35 UTC #0 lore raw

PATCH 1/1 spidev compiler warning gone

Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.

Signed-off-by David Brownell <[email protected]>

---
 drivers/spi/spidev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/spi/spidev.c +4 -2

@@ -181,7 +181,8 @@ static int spidev_message(struct spidev_
 		}
 		if (u_tmp->tx_buf) {
 			k_tmp->tx_buf = buf;
-			if (copy_from_user(buf, (const u8 __user *)u_tmp->tx_buf,
+			if (copy_from_user(buf, (const u8 __user *)
+						(ptrdiff_t) u_tmp->tx_buf,
 					u_tmp->len))
 				goto done;
 		}

@@ -213,7 +214,8 @@ static int spidev_message(struct spidev_
 	buf = spidev->buffer;
 	for (n = n_xfers, u_tmp = u_xfers; n; n--, u_tmp++) {
 		if (u_tmp->rx_buf) {
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
 					u_tmp->len)) {
 				status = -EFAULT;
 				goto done;

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

A message that is not in the archive (sent off-list or not yet received): 200705251035.10587.david-b-yBeKhBN/[email protected]

Andrew Morton

2007-05-29 22:02 UTC #2 lore raw

Quoted 22 lines · On Fri, 25 May 2007 10:35:07 -0700 David Brownell…

Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.
Signed-off-by David Brownell <[email protected]>
---
 drivers/spi/spidev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/spi/spidev.c +2 -1
@@ -181,7 +181,8 @@ static int spidev_message(struct spidev_
 		}
 		if (u_tmp->tx_buf) {
 			k_tmp->tx_buf = buf;
-			if (copy_from_user(buf, (const u8 __user *)u_tmp->tx_buf,
+			if (copy_from_user(buf, (const u8 __user *)
+						(ptrdiff_t) u_tmp->tx_buf,
 					u_tmp->len))
 				goto done;
 		}

drivers/spi/spidev.c static int spidev_message(struct spidev_ · line 221

Quoted 9 lines

drivers/spi/spidev.c +2 -1

@@ -213,7 +214,8 @@ static int spidev_message(struct spidev_
 	buf = spidev->buffer;
 	for (n = n_xfers, u_tmp = u_xfers; n; n--, u_tmp++) {
 		if (u_tmp->rx_buf) {
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
 					u_tmp->len)) {
 				status = -EFAULT;
 				goto done;

This seems overly tricky. The way we normally squish that warning is via a (long) cast.

A message that is not in the archive (sent off-list or not yet received): 20070529150214.75204439.akpm-de/[email protected]

David Brownell

author

2007-05-29 22:39 UTC #4 lore raw

Quoted 7 lines · Andrew Morton

Quoted 4 lines · On Fri, 25 May 2007 10:35:07 -0700 David Brownell…
Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.

Quoted 8 lines

drivers/spi/spidev.c static int spidev_message(struct spidev_ · line 218
Quoted 5 lines
	...
drivers/spi/spidev.c +2 -1
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
...
This seems overly tricky. The way we normally squish that warning is via a (long) cast.

Well, "ptrdiff_t" seems equivalent to "long" in common cases.

Presumably C99 guarantees pointer-to-long conversions work in both directions? It seemed there must necessarily be such a guarantee for "ptrdiff_t", even if it weren't true for "long". I didn't have a copy of a C spec to consult. ;)

- Dave

A message that is not in the archive (sent off-list or not yet received): 200705291539.04253.david-b-yBeKhBN/[email protected]

Andrew Morton

2007-05-29 22:59 UTC #6 lore raw

Quoted 17 lines · On Tue, 29 May 2007 15:39:04 -0700 David Brownell…

Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Fri, 25 May 2007 10:35:07 -0700 David Brownell…
Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.
Quoted 8 lines
drivers/spi/spidev.c static int spidev_message(struct spidev_ · line 218
Quoted 5 lines
	...
drivers/spi/spidev.c +2 -1
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
...
This seems overly tricky. The way we normally squish that warning is via a (long) cast.

Quoted 7 lines

Well, "ptrdiff_t" seems equivalent to "long" in common cases.
Presumably C99 guarantees pointer-to-long conversions work in both directions? It seemed there must necessarily be such a guarantee for "ptrdiff_t", even if it weren't true for "long". I didn't have a copy of a C spec to consult. ;)

Thing is, the kernel does a cast of an integer-type to a pointer-type in a *lot* of places. And the standard way in which we squish the warning is via a (long) cast.

So it's a standard pattern and people understand it. Whereas the ptrdiff_t thing will make readers go "wtf?". Even if it deos happen to work.

A message that is not in the archive (sent off-list or not yet received): 20070529155911.105c65b7.akpm-de/[email protected]

David Brownell

author

2007-05-30 02:05 UTC #8 lore raw

Quoted 22 lines

Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.
Quoted 8 lines
Review
Quoted 5 lines
	...
(quoted diff) +2 -1
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
...
This seems overly tricky. The way we normally squish that warning is via a (long) cast.
Quoted 7 lines
Well, "ptrdiff_t" seems equivalent to "long" in common cases.
Presumably C99 guarantees pointer-to-long conversions work in both directions? It seemed there must necessarily be such a guarantee for "ptrdiff_t", even if it weren't true for "long". I didn't have a copy of a C spec to consult. ;)

Quoted 3 lines

Thing is, the kernel does a cast of an integer-type to a pointer-type in a *lot* of places. And the standard way in which we squish the warning is via a (long) cast.

Good to know, but I didn't notice any examples anywhere in the source code of doing that. I used the first solution I found, when I went hunting for a fix. It wasn't "long". And it appears that "long" has fewer portability guarantees...

Something in Documentation/* should talk about 64-bit cleanliness, and maybe mention such issues.

Quoted 2 lines

So it's a standard pattern and people understand it. Whereas the ptrdiff_t thing will make readers go "wtf?". Even if it deos happen to work.

At this point, "long" would surprise *me*! I notice you didn't answer my question about C specs and "long". "ptrdiff_t" sure seems to be a standard C data type designed specifically to solve that class of 32/64/... portability problem. "intptr_t" would seem to be the most standard type, but I don't see it in kernel includes.

I notice that GCC docs say ptrdiff_t "will probably be one of the standard integer types (short, int, long) but might be a nonstandard type that exists only for this purpose". So using "long" isn't as portable, and isn't even guaranteed to work everywhere.

- Dave

A message that is not in the archive (sent off-list or not yet received): 200705291905.34670.david-b-yBeKhBN/[email protected]

Andrew Morton

2007-05-30 02:19 UTC #10 lore raw

Quoted 22 lines · On Tue, 29 May 2007 19:05:34 -0700 David Brownell…

Quoted 22 lines
Quoted 21 lines
Quoted 5 lines
Quoted 5 lines
Get rid of annoying GCC warning on 32-bit platforms. The trick is to add an extra cast using "ptrdiff_t" to convert the u64 to the correct size integer, and only then casting it into a "void *" pointer.
Quoted 8 lines
Review
Quoted 5 lines
	...
(quoted diff) +2 -1
-			if (__copy_to_user((u8 __user *)u_tmp->rx_buf, buf,
+			if (__copy_to_user((u8 __user *)
+					(ptrdiff_t) u_tmp->rx_buf, buf,
...
This seems overly tricky. The way we normally squish that warning is via a (long) cast.
Well, "ptrdiff_t" seems equivalent to "long" in common cases.
Presumably C99 guarantees pointer-to-long conversions work in both directions? It seemed there must necessarily be such a guarantee for "ptrdiff_t", even if it weren't true for "long". I didn't have a copy of a C spec to consult. ;)

Quoted 8 lines

Quoted 3 lines
Thing is, the kernel does a cast of an integer-type to a pointer-type in a *lot* of places. And the standard way in which we squish the warning is via a (long) cast.
Good to know, but I didn't notice any examples anywhere in the source code of doing that. I used the first solution I found, when I went hunting for a fix. It wasn't "long". And it appears that "long" has fewer portability guarantees...

Kernel assumes that sizeof(long)==sizeof(foo *) in a tremendous number of places. That's why we safely use (long) for this.

box:/usr/src/linux-2.6.22-rc3> fgrep -r '*)(long)' . | wc -l 95
box:/usr/src/linux-2.6.22-rc3> fgrep -r '(int)(long)' . | wc -l 45
box:/usr/src/linux-2.6.22-rc3> fgrep -r '(ptrdiff_t)' . | wc -l 7

Quoted 2 lines

Something in Documentation/* should talk about 64-bit cleanliness, and maybe mention such issues.

spose so.

Quoted 6 lines

Quoted 3 lines
So it's a standard pattern and people understand it. Whereas the ptrdiff_t thing will make readers go "wtf?". Even if it deos happen to work.
At this point, "long" would surprise *me*! I notice you didn't answer my question about C specs and "long".

oh. I wasn't very interested, sorry ;) I'm just pointing out that (long) will cause least-surprise to most-people.