There are a few more macros in drmP.h that are unused; DRM_GET_PRIV_SAREA,
DRM_ARRAY_SIZE, and DRM_WAITCOUNT can go away completely.
Unfortunately, DRM_COPY is still used in one place, but we can at least
move it to where it's used. It's an awful looking macro..
Signed-off-by: Andres Salomon <[email protected]>
---
drivers/gpu/drm/drm_drv.c | 12 ++++++++++++
include/drm/drmP.h | 25 -------------------------
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a75ca63..43297ca 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -366,6 +366,18 @@ module_init(drm_core_init);
module_exit(drm_core_exit);
/**
+ * Copy and IOCTL return string to user space
+ */
+#define DRM_COPY(name, value) \
+ len = strlen(value); \
+ if (len > name##_len) len = name##_len; \
+ name##_len = strlen(value); \
+ if (len && name) { \
+ if (copy_to_user(name, value, len)) \
+ return -EFAULT; \
+ }
+
+/**
* Get version information
*
* \param inode device inode.
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index af30da3..b2112f4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -251,23 +251,10 @@ extern void drm_ut_debug_printk(unsigned int request_level,
/** \name Internal types and structures */
/*@{*/
-#define DRM_ARRAY_SIZE(x) ARRAY_SIZE(x)
-
#define DRM_LEFTCOUNT(x) (((x)->rp + (x)->count - (x)->wp) % ((x)->count + 1))
#define DRM_BUFCOUNT(x) ((x)->count - DRM_LEFTCOUNT(x))
-#define DRM_WAITCOUNT(dev,idx) DRM_BUFCOUNT(&dev->queuelist[idx]->waitlist)
#define DRM_IF_VERSION(maj, min) (maj << 16 | min)
-/**
- * Get the private SAREA mapping.
- *
- * \param _dev DRM device.
- * \param _ctx context number.
- * \param _map output mapping.
- */
-#define DRM_GET_PRIV_SAREA(_dev, _ctx, _map) do { \
- (_map) = (_dev)->context_sareas[_ctx]; \
-} while(0)
/**
* Test that the hardware lock is held by the caller, returning otherwise.
@@ -287,18 +274,6 @@ do { \
} while (0)
/**
- * Copy and IOCTL return string to user space
- */
-#define DRM_COPY( name, value ) \
- len = strlen( value ); \
- if ( len > name##_len ) len = name##_len; \
- name##_len = strlen( value ); \
- if ( len && name ) { \
- if ( copy_to_user( name, value, len ) ) \
- return -EFAULT; \
- }
-
-/**
* Ioctl function type.
*
* \param inode device inode.
--
1.5.6.5
On Mon, 19 Oct 2009 01:37:08 -0400 Andres Salomon <[email protected]> wrote:
> There are a few more macros in drmP.h that are unused; DRM_GET_PRIV_SAREA,
> DRM_ARRAY_SIZE, and DRM_WAITCOUNT can go away completely.
>
> Unfortunately, DRM_COPY is still used in one place, but we can at least
> move it to where it's used. It's an awful looking macro..
It would have been nice to fix the (valid) checkpatch warnings while
you were there.
On Mon, 2 Nov 2009 23:41:24 -0800
Andrew Morton <[email protected]> wrote:
> On Mon, 19 Oct 2009 01:37:08 -0400 Andres Salomon
> <[email protected]> wrote:
>
> > There are a few more macros in drmP.h that are unused;
> > DRM_GET_PRIV_SAREA, DRM_ARRAY_SIZE, and DRM_WAITCOUNT can go away
> > completely.
> >
> > Unfortunately, DRM_COPY is still used in one place, but we can at
> > least move it to where it's used. It's an awful looking macro..
>
> It would have been nice to fix the (valid) checkpatch warnings while
> you were there.
>
How about I do one better and replace the macro w/ a function?
>From 535091717408e4ec4974e8f309212f61aabd1880 Mon Sep 17 00:00:00 2001
From: Andres Salomon <[email protected]>
Date: Tue, 3 Nov 2009 15:03:52 -0500
Subject: [PATCH] drm: replace DRM_COPY macro w/ a function
Don't inline it; the compiler can figure it out. Comments added that are
based upon my interpretation of the code. Hopefully they're correct. :)
Signed-off-by: Andres Salomon <[email protected]>
---
drivers/gpu/drm/drm_drv.c | 34 ++++++++++++++++++++++------------
1 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 43297ca..ec0e3ae 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -368,14 +368,25 @@ module_exit(drm_core_exit);
/**
* Copy and IOCTL return string to user space
*/
-#define DRM_COPY(name, value) \
- len = strlen(value); \
- if (len > name##_len) len = name##_len; \
- name##_len = strlen(value); \
- if (len && name) { \
- if (copy_to_user(name, value, len)) \
- return -EFAULT; \
- }
+static int drm_copy_field(char *buf, size_t *buf_len, const char *value)
+{
+ int len;
+
+ /* don't overflow userbuf */
+ len = strlen(value);
+ if (len > *buf_len)
+ len = *buf_len;
+
+ /* let userspace know exact length of driver value (which could be
+ * larger than the userspace-supplied buffer) */
+ *buf_len = strlen(value);
+
+ /* finally, try filling in the userbuf */
+ if (len && buf)
+ if (copy_to_user(buf, value, len))
+ return -EFAULT;
+ return 0;
+}
/**
* Get version information
@@ -392,14 +403,13 @@ static int drm_version(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
struct drm_version *version = data;
- int len;
version->version_major = dev->driver->major;
version->version_minor = dev->driver->minor;
version->version_patchlevel = dev->driver->patchlevel;
- DRM_COPY(version->name, dev->driver->name);
- DRM_COPY(version->date, dev->driver->date);
- DRM_COPY(version->desc, dev->driver->desc);
+ drm_copy_field(version->name, &version->name_len, dev->driver->name);
+ drm_copy_field(version->date, &version->date_len, dev->driver->date);
+ drm_copy_field(version->desc, &version->desc_len, dev->driver->desc);
return 0;
}
--
1.5.6.5
On Tue, 3 Nov 2009 15:09:54 -0500
Andres Salomon <[email protected]> wrote:
> On Mon, 2 Nov 2009 23:41:24 -0800
> Andrew Morton <[email protected]> wrote:
>
> > On Mon, 19 Oct 2009 01:37:08 -0400 Andres Salomon
> > <[email protected]> wrote:
> >
> > > There are a few more macros in drmP.h that are unused;
> > > DRM_GET_PRIV_SAREA, DRM_ARRAY_SIZE, and DRM_WAITCOUNT can go away
> > > completely.
> > >
> > > Unfortunately, DRM_COPY is still used in one place, but we can at
> > > least move it to where it's used. It's an awful looking macro..
> >
> > It would have been nice to fix the (valid) checkpatch warnings while
> > you were there.
> >
>
> How about I do one better and replace the macro w/ a function?
>
>
>
> From 535091717408e4ec4974e8f309212f61aabd1880 Mon Sep 17 00:00:00 2001
> From: Andres Salomon <[email protected]>
> Date: Tue, 3 Nov 2009 15:03:52 -0500
> Subject: [PATCH] drm: replace DRM_COPY macro w/ a function
>
> Don't inline it; the compiler can figure it out. Comments added that
> are based upon my interpretation of the code. Hopefully they're
> correct. :)
>
And here's one that actually checks the return values from
drm_copy_field. This is separate from the pure cleanup patch(es)
as it changes behavior that userspace
sees, so it can be bisected and reverted apart from the others if
needed.
>From 041636a0e2b6d7ec5c4fdb8f495682d6928b73bf Mon Sep 17 00:00:00 2001
From: Andres Salomon <[email protected]>
Date: Tue, 3 Nov 2009 15:15:51 -0500
Subject: [PATCH] drm: check return values in drm_version
In drm_version, actually check the results from function calls so that
we're not potentially passing garbage back to userspace.
Signed-off-by: Andres Salomon <[email protected]>
---
drivers/gpu/drm/drm_drv.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index ec0e3ae..5bd3f94 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -403,15 +403,21 @@ static int drm_version(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
struct drm_version *version = data;
+ int err;
version->version_major = dev->driver->major;
version->version_minor = dev->driver->minor;
version->version_patchlevel = dev->driver->patchlevel;
- drm_copy_field(version->name, &version->name_len, dev->driver->name);
- drm_copy_field(version->date, &version->date_len, dev->driver->date);
- drm_copy_field(version->desc, &version->desc_len, dev->driver->desc);
-
- return 0;
+ err = drm_copy_field(version->name, &version->name_len,
+ dev->driver->name);
+ if (!err)
+ err = drm_copy_field(version->date, &version->date_len,
+ dev->driver->date);
+ if (!err)
+ err = drm_copy_field(version->desc, &version->desc_len,
+ dev->driver->desc);
+
+ return err;
}
/**
--
1.5.6.5