Re: radeon warning on 64-bit platforms

Re: radeon warning on 64-bit platforms

[David Mosberger]

Linus and Michel,

Here is a proposed patch for fixing the compile-warning that shows up
when compiling radeon_state.c on a 64-bit platform (Itanium, in my
case).  According to Michel, RADEON_PARAM_SAREA_HANDLE is used only on
embedded platforms and since it is not possible to fix the problem
without breaking backwards-compatibility for those platforms, the
interim fix is to simply desupport this particular ioctl() on 64-bit
platforms (i.e., make it fail with EINVAL).

If there are no objections, please apply.

	--david

=== drivers/char/drm/radeon_state.c 1.23 vs edited ==--- 1.23/drivers/char/drm/radeon_state.c	Tue Feb  3 21:29:26 2004
+++ edited/drivers/char/drm/radeon_state.c	Tue Feb 17 15:23:01 2004
@@ -2186,9 +2186,24 @@
 		value = dev_priv->ring_rptr_offset;
 		break;
 	case RADEON_PARAM_SAREA_HANDLE:
-		/* The lock is the first dword in the sarea. */
-		value = (int)dev->lock.hw_lock; 
-		break;	
+		/*
+		 * This ioctl() doesn't work on 64-bit platforms because hw_lock is a
+		 * pointer which can't fit into an int-sized variable.  According to
+		 * Michael Dänzer, the ioctl() is only used on embedded platforms, so not
+		 * supporting it shouldn't be a problem.  If the same functionality is
+		 * needed on 64-bit platforms, a new ioctl() would have to be added, so
+		 * backwards-compatibility for the embedded platforms can be maintained.
+		 *	--davidm 4-Feb-2004.
+		 */
+	  	if (sizeof (dev->lock.hw_lock) <= sizeof (int))
+		    /*
+		     * The lock is the first dword in the sarea.  Cast to "long" so it
+		     * compiles without warning on 64-bit platforms.
+		     */
+		    value = (long)dev->lock.hw_lock;
+		else
+		    return DRM_ERR(EINVAL);
+		break;
 	case RADEON_PARAM_GART_TEX_HANDLE:
 		value = dev_priv->gart_textures_offset;
 		break;

Re: radeon warning on 64-bit platforms

[Michel Dänzer]

David,


looks good to me, except for a detail:

> +		 * Michael Dänzer, the ioctl() is only used on embedded platforms, so not
                     ^^^^^^^
That's not quite my first name. :)


I'd appreciate a heads up when this (or whatever becomes of it) goes in
so I can apply it to DRI CVS as well. On the other hand, I'll probably
see it at the latest when Ben merges it into his tree. :)


-- 
Earthling Michel Dänzer      |     Debian (powerpc), X and DRI developer
Libre software enthusiast    |   http://svcs.affero.net/rm.php?rÚenzer

Re: radeon warning on 64-bit platforms

[Anton Blanchard]

> Here is a proposed patch for fixing the compile-warning that shows up
> when compiling radeon_state.c on a 64-bit platform (Itanium, in my
> case).  According to Michel, RADEON_PARAM_SAREA_HANDLE is used only on
> embedded platforms and since it is not possible to fix the problem
> without breaking backwards-compatibility for those platforms, the
> interim fix is to simply desupport this particular ioctl() on 64-bit
> platforms (i.e., make it fail with EINVAL).
> 
> If there are no objections, please apply.

A small thing, could the comment be constrained to 80 columns? :)

Anton

Re: radeon warning on 64-bit platforms

[David Mosberger]

>>>>> On Wed, 18 Feb 2004 10:48:49 +1100, Anton Blanchard <anton@samba.org> said:

  Anton> A small thing, could the comment be constrained to 80
  Anton> columns? :)

I don't really see the point of that, given that pretty much all
existing Linux source code is formatted for 100 columns.  I don't feel
strongly about it, however, so I changed it.

>>>>> On Tue, 17 Feb 2004 15:53:05 -0800 (PST), Linus Torvalds <torvalds@osdl.org> said:

  Linus> How about this alternate edited one that gets indentation
  Linus> right and is more explicit about what's going on? Does this
  Linus> work for you?

Fine by me.

>>>>> On Wed, 18 Feb 2004 00:44:16 +0100, Michel Dänzer <michel@daenzer.net> said:

  Michel> looks good to me, except for a detail:

  >> + * Michael Dänzer, the ioctl() is only used on embedded
  >> platforms, so not

  Michel> ^^^^^^^ That's not quite my first name. :)

Sorry about that, fixed below (or so I hope).

	--david

=== drivers/char/drm/radeon_state.c 1.23 vs edited ==--- 1.23/drivers/char/drm/radeon_state.c	Tue Feb  3 21:29:26 2004
+++ edited/drivers/char/drm/radeon_state.c	Tue Feb 17 16:49:15 2004
@@ -2185,10 +2185,21 @@
 	case RADEON_PARAM_STATUS_HANDLE:
 		value = dev_priv->ring_rptr_offset;
 		break;
+#if BITS_PER_LONG = 32
+	/*
+	 * This ioctl() doesn't work on 64-bit platforms because hw_lock is a
+	 * pointer which can't fit into an int-sized variable.  According to
+	 * Michel Dänzer, the ioctl() is only used on embedded platforms, so
+	 * not supporting it shouldn't be a problem.  If the same functionality
+	 * is needed on 64-bit platforms, a new ioctl() would have to be added,
+	 * so backwards-compatibility for the embedded platforms can be
+	 * maintained.  --davidm 4-Feb-2004.
+	 */
 	case RADEON_PARAM_SAREA_HANDLE:
 		/* The lock is the first dword in the sarea. */
-		value = (int)dev->lock.hw_lock; 
-		break;	
+		value = (long)dev->lock.hw_lock;
+		break;
+#endif
 	case RADEON_PARAM_GART_TEX_HANDLE:
 		value = dev_priv->gart_textures_offset;
 		break;

Re: radeon warning on 64-bit platforms

[Matthew Wilcox]

On Tue, Feb 17, 2004 at 04:51:24PM -0800, David Mosberger wrote:
> >>>>> On Wed, 18 Feb 2004 10:48:49 +1100, Anton Blanchard <anton@samba.org> said:
> 
>   Anton> A small thing, could the comment be constrained to 80
>   Anton> columns? :)
> 
> I don't really see the point of that, given that pretty much all
> existing Linux source code is formatted for 100 columns.  I don't feel
> strongly about it, however, so I changed it.

Um, only your crap.  Everybody else follows Documentation/CodingStyle.
This is mentioned in a couple of places:

Now, some people will claim that having 8-character indentations makes
the code move too far to the right, and makes it hard to read on a
80-character terminal screen.  The answer to that is that if you need
more than 3 levels of indentation, you're screwed anyway, and should fix
your program. 

...

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well. 


I wish the ia64 code weren't indented to 100 columns.  It's why I hate
working with your code.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: radeon warning on 64-bit platforms

[David Mosberger]

>>>>> On Wed, 18 Feb 2004 01:54:23 +0000, Matthew Wilcox <willy@debian.org> said:

  >> I don't really see the point of that, given that pretty much all
  >> existing Linux source code is formatted for 100 columns.  I don't feel
  >> strongly about it, however, so I changed it.

  Matthew> Um, only your crap.  Everybody else follows
  Matthew> Documentation/CodingStyle.

Wow.  Revisionists at work? ;-)

I personally would be more than happy to reformat things to 80 cols,
but it's a waste of time unless almost all Linux code gets
reformatted.

	--david

Re: radeon warning on 64-bit platforms

[Matthew Wilcox]

On Tue, Feb 17, 2004 at 05:59:12PM -0800, David Mosberger wrote:
> I personally would be more than happy to reformat things to 80 cols,
> but it's a waste of time unless almost all Linux code gets
> reformatted.

Hm?  I don't know where you're getting that from.  Let's talk numbers.

Of the 60525 lines in .c files in arch/i386, 460 are longer than 80 cols.
Of the 67398 lines in .c files in arch/ia64, 1189 are longer than 80 cols.
Of the 496510 lines in .c files in drivers/net, 4044 are longer than 80 cols.

So arch/i386 has 0.76% > 80 column lines, drivers/net is 0.81% and
arch/ia64 is 1.76%.  Seems fairly convincing to me that ia64 is out of
step with the rest of Linux.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: radeon warning on 64-bit platforms

[David Mosberger]

>>>>> On Wed, 18 Feb 2004 02:28:31 +0000, Matthew Wilcox <willy@debian.org> said:

  Matthew> On Tue, Feb 17, 2004 at 05:59:12PM -0800, David Mosberger
  Matthew> wrote:
  >> I personally would be more than happy to reformat things to 80
  >> cols, but it's a waste of time unless almost all Linux code gets
  >> reformatted.

  Matthew> Hm?  I don't know where you're getting that from.  Let's
  Matthew> talk numbers.

If it bothers you, you're perfectly free to submit patches.
Source-code width is not something I get overly excited about.

	--david

Re: radeon warning on 64-bit platforms

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On Wed, 18 Feb 2004 02:28:31 GMT, Matthew Wilcox said:
> On Tue, Feb 17, 2004 at 05:59:12PM -0800, David Mosberger wrote:
> > I personally would be more than happy to reformat things to 80 cols,
> > but it's a waste of time unless almost all Linux code gets
> > reformatted.
> 
> Hm?  I don't know where you're getting that from.  Let's talk numbers.
> 
> Of the 60525 lines in .c files in arch/i386, 460 are longer than 80 cols.
> Of the 67398 lines in .c files in arch/ia64, 1189 are longer than 80 cols.
> Of the 496510 lines in .c files in drivers/net, 4044 are longer than 80 cols.

You apparently made the mistake of looking at character count < 80, not
lines that extend past column 80.

For 2.6.3-mm1, I see:
[/usr/src/linux-2.6.3-mm1/arch/i386]2 find . -name '*.c' | xargs cat | wc -l
  64542
[/usr/src/linux-2.6.3-mm1/arch/i386]2 find . -name '*.c' | xargs cat | awk 'length() > 80 { print $0}' | wc -l
    477
[/usr/src/linux-2.6.3-mm1/arch/i386]2 find . -name '*.c' | xargs cat |sed 's/\t/        /g'| awk 'length() > 80 { print $0}' | wc -l
   1291

(replace \t with whatever your shell needs to enter a literal tab .  Yes, this
botches on the relatively rare line that has an embedded tab. Deal with it. ;).

In other words, the situation is a lot worse if you count columns, not
characters.

(For IA64, it's 67,376 and either 1,185 or 3,222, and for drivers/net I see
493,027 and either 4,004 or 15,606 - ia64 is worse at the 80-col thing either way)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: radeon warning on 64-bit platforms

[Andreas Schwab]

Valdis.Kletnieks@vt.edu writes:

> [/usr/src/linux-2.6.3-mm1/arch/i386]2 find . -name '*.c' | xargs cat |sed 's/\t/        /g'| awk 'length() > 80 { print $0}' | wc -l
>    1291
>
> (replace \t with whatever your shell needs to enter a literal tab .  Yes, this
> botches on the relatively rare line that has an embedded tab. Deal with it. ;).

Use expand.

$ find . -name '*.c' | xargs cat | expand | awk 'length() > 80 { print $0}' | wc -l
1125

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: radeon warning on 64-bit platforms

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Fri, 20 Feb 2004 16:51:00 +0100, Andreas Schwab said:

> Use expand.

Learn something new every day.  Usually, the task at hand is the inverse task -
fixing a file that has leading blanks to have leading tabs instead. ;)


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: radeon warning on 64-bit platforms

[Linus Torvalds]

On Tue, 17 Feb 2004, David Mosberger wrote:
> 
> Here is a proposed patch for fixing the compile-warning that shows up
> when compiling radeon_state.c on a 64-bit platform (Itanium, in my
> case).

How about this alternate edited one that gets indentation right
and is more explicit about what's going on? Does this work for you?

		Linus

---
=== drivers/char/drm/radeon_state.c 1.23 vs edited ==--- 1.23/drivers/char/drm/radeon_state.c	Tue Feb  3 21:29:26 2004
+++ edited/drivers/char/drm/radeon_state.c	Tue Feb 17 15:51:53 2004
@@ -2185,10 +2185,21 @@
 	case RADEON_PARAM_STATUS_HANDLE:
 		value = dev_priv->ring_rptr_offset;
 		break;
+#if BITS_PER_LONG = 32
+	/*
+	 * This ioctl() doesn't work on 64-bit platforms because hw_lock is a
+	 * pointer which can't fit into an int-sized variable.  According to
+	 * Michel Dänzer, the ioctl() is only used on embedded platforms, so not
+	 * supporting it shouldn't be a problem.  If the same functionality is
+	 * needed on 64-bit platforms, a new ioctl() would have to be added, so
+	 * backwards-compatibility for the embedded platforms can be maintained.
+	 *      --davidm 4-Feb-2004.
+	 */
 	case RADEON_PARAM_SAREA_HANDLE:
 		/* The lock is the first dword in the sarea. */
 		value = (int)dev->lock.hw_lock; 
 		break;	
+#endif
 	case RADEON_PARAM_GART_TEX_HANDLE:
 		value = dev_priv->gart_textures_offset;
 		break;