Date:	Tue, 1 May 2007 09:10:00 +1000
From:	"Dave Airlie" <airlied@...il.com>
To:	"Greg KH" <greg@...ah.com>
Cc:	"Linux Kernel" <linux-kernel@...r.kernel.org>,
	"Thomas Hellstr?m" <thomas@...gstengraphics.com>,
	"Jesse Barnes" <jbarnes@...tuousgeek.org>,
	"Eric Anholt" <eric@...olt.net>
Subject: Re: [RFC] [PATCH] DRM TTM Memory Manager patch

>
> A few easy and simple comments based on looking at this for 5 minutes:
>   - drop the typedefs.  Yeah, they might be a drm thing, but we don't
>     need them here.

Okay I think in-kernel typedefs have to go, but we have a defined DRM
interface like it or not and I'd like to be consistent on the
userspace interface and continue to use typedefs, I don't want to have
a major inconsistency like half the interface in typedefs the other
half in non-typedefs, considering the old interface is frozen...

>   - the comment style for the functions is "odd" and not in kerneldoc
>     form, but something else.  Either use kerneldoc or nothing, don't
>     invent something new please.

Most likely in doxygen as that is what Mesa uses and the intersection
of developers is higher in that area, I'll take it as a task to try
and kerneldoc the drm at some stage..

>   - what's with the /proc interface?  Don't add new proc code for
>     non-process related things.  This should all go into sysfs
>     somewhere.  And yes, I know /proc/dri/ is there today, but don't add
>     new stuff please.

Well we should move all that stuff to sysfs, but we have all the
infrastructure for publishing this stuff under /proc/dri and adding
new files doesn't take a major amount, as much as I appreciate sysfs,
it isn't suitable for this sort of information dump, the whole one
value per file is quite useless to provide this sort of information
which is uni-directional for users to send to us for debugging without
have to install some special tool to join all the values into one
place.. and I don't think drmfs is the answer either... or maybe it
is....

>   - struct drm_bo_arg can't use an int or unsigned, as it crosses the
>     userspace/kernelspace boundry, use the proper types for all values
>     in those types of structures (__u32, etc.)

int is defined, unsigned I'm not so sure about, the drm user space
interface is usually specified in non-system specific types so the
drm.h file is consistent across systems, so we would probably have to
use uint32_t which other people have objected to, but I'd rather use
uint32_t than unspecified types..

>   - there doesn't seem to be any validity checking for the arguments
>     passed into this new ioctl.  Possibly that's just the way the rest
>     of the dri interface is, which is scary, but with the memory stuff,
>     you really should check things properly...

Okay this needs fixing, we do check most ioctls args, the main thing
passed in are handles and these are all looked up in the hash table,
it may not be so obvious, also most of the ioctls are probably going
to end up root or DRM master only, I'd like do an ioctl fuzzer at some
stage, I'd suspect a lot more then the dri would be oopsable with
permissions...

Thanks,
Dave.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists