Re: From Eric Anholt:

Re: From Eric Anholt:

[Greg KH]

On Sat, Apr 10, 2004 at 04:32:02AM +0000, Linux Kernel Mailing List wrote:
> diff -Nru a/drivers/char/drm/drm.h b/drivers/char/drm/drm.h
> --- a/drivers/char/drm/drm.h	Tue May 11 15:11:32 2004
> +++ b/drivers/char/drm/drm.h	Tue May 11 15:11:32 2004
> @@ -580,6 +580,16 @@
>  	unsigned long handle;	/**< Used for mapping / unmapping */
>  } drm_scatter_gather_t;
>  
> +/**
> + * DRM_IOCTL_SET_VERSION ioctl argument type.
> + */
> +typedef struct drm_set_version {
> +	int drm_di_major;
> +	int drm_di_minor;
> +	int drm_dd_major;
> +	int drm_dd_minor;
> +} drm_set_version_t;
> +

Ick, you can't use "int" as an ioctl structure member, sorry.  Please
use the proper "__u16" or "__u32" value instead.

And what about kernels running in 64bit mode with 32bit userspace?  Care
to provide the proper thunking layer for them too?

thanks,

greg k-h

Re: From Eric Anholt:

[Dave Airlie]

> >
> > +/**
> > + * DRM_IOCTL_SET_VERSION ioctl argument type.
> > + */
> > +typedef struct drm_set_version {
> > +	int drm_di_major;
> > +	int drm_di_minor;
> > +	int drm_dd_major;
> > +	int drm_dd_minor;
> > +} drm_set_version_t;
> > +
>
> Ick, you can't use "int" as an ioctl structure member, sorry.  Please
> use the proper "__u16" or "__u32" value instead.

okay I'll submit a fix to Linus rsn ...
>
> And what about kernels running in 64bit mode with 32bit userspace?  Care
> to provide the proper thunking layer for them too?

would love to don't have anyone on team that does 64-bit at the moment or
even has access to 64-bit hardware .. f someone provides the code I'll accept
it, anyway the DRM isn't completely 64-bit safe AFAIK, it has other
"issues",

Thanks,
Dave.

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person

Re: From Eric Anholt:

[Dave Airlie]

>
> Ick, you can't use "int" as an ioctl structure member, sorry.  Please
> use the proper "__u16" or "__u32" value instead.

I just looked at drm.h and nearly all the ioctls use int, this file is
included in user-space applications also at the moment, I'm worried
changing all ints to __u32 will break some of these, anyone on DRI list
care to comment?

Dave.

 >
> And what about kernels running in 64bit mode with 32bit userspace?  Care
> to provide the proper thunking layer for them too?
>
> thanks,
>
> greg k-h
>

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person

Re: From Eric Anholt:

[Valdis.Kletnieks]

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

On Wed, 12 May 2004 00:20:51 BST, Dave Airlie said:

> I just looked at drm.h and nearly all the ioctls use int, this file is
> included in user-space applications also at the moment, I'm worried
> changing all ints to __u32 will break some of these, anyone on DRI list
> care to comment?

Is this a case where somebody is *really* including kernel headers in userspace
and we need to smack them, or are they using a copy that's been sanitized
(and possibly fixed)?

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

Re: From Eric Anholt:

[Greg KH]

On Tue, May 11, 2004 at 07:34:39PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 12 May 2004 00:20:51 BST, Dave Airlie said:
> 
> > I just looked at drm.h and nearly all the ioctls use int, this file is
> > included in user-space applications also at the moment, I'm worried
> > changing all ints to __u32 will break some of these, anyone on DRI list
> > care to comment?
> 
> Is this a case where somebody is *really* including kernel headers in userspace
> and we need to smack them, or are they using a copy that's been sanitized
> (and possibly fixed)?

Don't know, but how are you dealing with the issue that an "int" is
different for different kernel sizes (64 vs 32) and userspace too.
That's why you can't use it in an ioctl and expect things to work
properly.

thanks,

greg k-h

re: ioctls in drm.h

[Dave Airlie]

> > care to comment?
>
> Is this a case where somebody is *really* including kernel headers in userspace
> and we need to smack them, or are they using a copy that's been sanitized
> (and possibly fixed)?

hmm drm.h is used by all drm users and it hasn't really been sanitised..
we probably do need to look into what goes where.. are the rules for 32/64
user/kernel ioctls written down anywhere?

Dave.
>

-- 
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person

Re: From Eric Anholt:

[Daniel Jacobowitz]

On Tue, May 11, 2004 at 04:43:29PM -0700, Greg KH wrote:
> On Tue, May 11, 2004 at 07:34:39PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Wed, 12 May 2004 00:20:51 BST, Dave Airlie said:
> > 
> > > I just looked at drm.h and nearly all the ioctls use int, this file is
> > > included in user-space applications also at the moment, I'm worried
> > > changing all ints to __u32 will break some of these, anyone on DRI list
> > > care to comment?
> > 
> > Is this a case where somebody is *really* including kernel headers in userspace
> > and we need to smack them, or are they using a copy that's been sanitized
> > (and possibly fixed)?
> 
> Don't know, but how are you dealing with the issue that an "int" is
> different for different kernel sizes (64 vs 32) and userspace too.
> That's why you can't use it in an ioctl and expect things to work
> properly.

I'm not disagreeing that it ought to use __u32, but are there any Linux
supported targets that don't have a 32-bit int?  It's long that tends
to change size.

-- 
Daniel Jacobowitz

Re: From Eric Anholt:

[H. Peter Anvin]

Followup to:  <20040511234329.GA27242@kroah.com>
By author:    Greg KH <greg@kroah.com>
In newsgroup: linux.dev.kernel
> 
> Don't know, but how are you dealing with the issue that an "int" is
> different for different kernel sizes (64 vs 32) and userspace too.
> That's why you can't use it in an ioctl and expect things to work
> properly.
> 

On Linux, "int" should always be 32 bits.  "long", and "void *",
however, may be 32 or 64 bits.

	-hpa

Re: From Eric Anholt:

[Greg KH]

On Tue, May 11, 2004 at 08:07:09PM -0400, Daniel Jacobowitz wrote:
> On Tue, May 11, 2004 at 04:43:29PM -0700, Greg KH wrote:
> > On Tue, May 11, 2004 at 07:34:39PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > > On Wed, 12 May 2004 00:20:51 BST, Dave Airlie said:
> > > 
> > > > I just looked at drm.h and nearly all the ioctls use int, this file is
> > > > included in user-space applications also at the moment, I'm worried
> > > > changing all ints to __u32 will break some of these, anyone on DRI list
> > > > care to comment?
> > > 
> > > Is this a case where somebody is *really* including kernel headers in userspace
> > > and we need to smack them, or are they using a copy that's been sanitized
> > > (and possibly fixed)?
> > 
> > Don't know, but how are you dealing with the issue that an "int" is
> > different for different kernel sizes (64 vs 32) and userspace too.
> > That's why you can't use it in an ioctl and expect things to work
> > properly.
> 
> I'm not disagreeing that it ought to use __u32, but are there any Linux
> supported targets that don't have a 32-bit int?  It's long that tends
> to change size.

I don't think so, but I am not sure.  That's why you should use __u32 to
keep people from guessing :)

thanks,

greg k-h

Re: From Eric Anholt:

[Jon Smirl]

Would int16_t and int32_t work? Those int's were in there before I started
working on it. __u16 and __u32 are Linux kernel defines that aren't always there
in user space.

I would much rather have one h file than two. When we had two the variable and
structure names, comments, etc had drifted over the years until they didn't even
look like the same file anymore. drm.h only defines the ioctls, no internal
kernel structures are exposed.

--- Dave Airlie <airlied@linux.ie> wrote:
> >
> > Ick, you can't use "int" as an ioctl structure member, sorry.  Please
> > use the proper "__u16" or "__u32" value instead.
> 
> I just looked at drm.h and nearly all the ioctls use int, this file is
> included in user-space applications also at the moment, I'm worried
> changing all ints to __u32 will break some of these, anyone on DRI list
> care to comment?
> 
> Dave.
> 
>  >
> > And what about kernels running in 64bit mode with 32bit userspace?  Care
> > to provide the proper thunking layer for them too?
> >
> > thanks,
> >
> > greg k-h
> >
> 
> -- 
> David Airlie, Software Engineer
> http://www.skynet.ie/~airlied / airlied at skynet.ie
> pam_smb / Linux DECstation / Linux VAX / ILUG person
> 
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by Sleepycat Software
> Learn developer strategies Cisco, Motorola, Ericsson & Lucent use to 
> deliver higher performing products faster, at low TCO.
> http://www.sleepycat.com/telcomwpreg.php?From=osdnemail3
> --
> _______________________________________________
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel


=====
Jon Smirl
jonsmirl@yahoo.com


	
		
__________________________________
Do you Yahoo!?
Win a $20,000 Career Makeover at Yahoo! HotJobs  
http://hotjobs.sweepstakes.yahoo.com/careermakeover 

Re: From Eric Anholt:

[Greg KH]

On Tue, May 11, 2004 at 06:07:27PM -0700, Jon Smirl wrote:
> Would int16_t and int32_t work?

No, sorry.  See the lkml archives for why.

> Those int's were in there before I started working on it. __u16 and
> __u32 are Linux kernel defines that aren't always there in user space.

Don't share header files between userspace and the kernel.  End of
problem :)

thanks,

greg k-h

Re: From Eric Anholt:

[Linus Torvalds]

On Wed, 12 May 2004, Dave Airlie wrote:
> 
> I just looked at drm.h and nearly all the ioctls use int, this file is
> included in user-space applications also at the moment, I'm worried
> changing all ints to __u32 will break some of these, anyone on DRI list
> care to comment?

Right now, all architectures have "int" being 32-bit, so nothing should 
break. Apart from sign issues, of course.

If there are pointers and "long", then those should just not exist. Never
expose kernel pointers to user mode (and you really never should pass user
pointers back), and "long" should really just be "__u32" instead (since 
that is what it is on a 32-bit platform - and if it works there, then it 
should work on a 64-bit platform too).

		Linus

Re: From Eric Anholt:

[Eric Anholt]

On Tue, 2004-05-11 at 16:34, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 12 May 2004 00:20:51 BST, Dave Airlie said:
> 
> > I just looked at drm.h and nearly all the ioctls use int, this file is
> > included in user-space applications also at the moment, I'm worried
> > changing all ints to __u32 will break some of these, anyone on DRI list
> > care to comment?
> 
> Is this a case where somebody is *really* including kernel headers in userspace
> and we need to smack them, or are they using a copy that's been sanitized
> (and possibly fixed)?

These headers being discussed are what define the interface between
userland and kernel, and nothing else.  They are included by both
userland (libdrm, statically linked in the 3d drivers and in the X
server) and kernel.

-- 
Eric Anholt                                eta@lclark.edu          
http://people.freebsd.org/~anholt/         anholt@FreeBSD.org

Re: From Eric Anholt:

[Egbert Eich]

Linus Torvalds writes:
 > 
 > 
 > On Wed, 12 May 2004, Dave Airlie wrote:
 > > 
 > > I just looked at drm.h and nearly all the ioctls use int, this file is
 > > included in user-space applications also at the moment, I'm worried
 > > changing all ints to __u32 will break some of these, anyone on DRI list
 > > care to comment?
 > 
 > Right now, all architectures have "int" being 32-bit, so nothing should 
 > break. Apart from sign issues, of course.
 > 
 > If there are pointers and "long", then those should just not exist. Never
 > expose kernel pointers to user mode (and you really never should pass user
 > pointers back), and "long" should really just be "__u32" instead (since 
 > that is what it is on a 32-bit platform - and if it works there, then it 
 > should work on a 64-bit platform too).
 > 

Unfortunately this is done in some places in DRM. Pointers are used as a 
simple 'handle' to point to areas that are to be mapped by mmap()
from user mode.
I've done some ioctl32() interfaces for DRM for a few drivers (mga, 
radeon, r128) to make 32bit software work on 64bit systems such as AMD64.
In most cases it was easy to work around the longs as they either did
not have to exist at all or could be replaced by something that's
platform independent.
I have plans to submit these ioctl32() interfaces to Mesa when I get
around to do it.
Anyway they are unnecessary kludges which should have been avoided
from the beginning.

Egbert.