[mm patch] drm, minor fixes

[mm patch] drm, minor fixes

[Frederik Deweerdt]

On Sun, Aug 13, 2006 at 01:24:54AM -0700, Andrew Morton wrote:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm1/
> 
Hi Andrew,

The following patch adds minor fixes to the drm code:
- fix return values that are wrong (return E* instead of return -E*)
- replaces an argument to a sizeof in drm_setversion to match the actual
variable name
- handle drm_set_busid() return value in drm_setversion

Regards,
Frederik

Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>
diff --git a/drivers/char/drm/i915_dma.c b/drivers/char/drm/i915_dma.c
index d932f80..f9615b5 100644
--- a/drivers/char/drm/i915_dma.c
+++ b/drivers/char/drm/i915_dma.c
@@ -391,7 +391,7 @@ static int i915_emit_box(drm_device_t * 
 	RING_LOCALS;
 
 	if (DRM_COPY_FROM_USER_UNCHECKED(&box, &boxes[i], sizeof(box))) {
-		return EFAULT;
+		return DRM_ERR(EFAULT);
 	}
 
 	if (box.y2 <= box.y1 || box.x2 <= box.x1 || box.y2 <= 0 || box.x2 <= 0) {
diff --git a/drivers/char/drm/drm_ioctl.c b/drivers/char/drm/drm_ioctl.c
index e158998..a93832f 100644
--- a/drivers/char/drm/drm_ioctl.c
+++ b/drivers/char/drm/drm_ioctl.c
@@ -141,12 +141,12 @@ static int drm_set_busid(drm_device_t * 
 	int len;
 
 	if (dev->unique != NULL)
-		return EBUSY;
+		return -EBUSY;
 
 	dev->unique_len = 40;
 	dev->unique = drm_alloc(dev->unique_len + 1, DRM_MEM_DRIVER);
 	if (dev->unique == NULL)
-		return ENOMEM;
+		return -ENOMEM;
 
 	len = snprintf(dev->unique, dev->unique_len, "pci:%04x:%02x:%02x.%d",
 		       drm_get_pci_domain(dev), dev->pdev->bus->number,
@@ -160,7 +160,7 @@ static int drm_set_busid(drm_device_t * 
 	    drm_alloc(strlen(dev->driver->pci_driver.name) + dev->unique_len +
 		      2, DRM_MEM_DRIVER);
 	if (dev->devname == NULL)
-		return ENOMEM;
+		return -ENOMEM;
 
 	sprintf(dev->devname, "%s@%s", dev->driver->pci_driver.name,
 		dev->unique);
@@ -342,20 +342,22 @@ int drm_setversion(DRM_IOCTL_ARGS)
 	retv.drm_dd_major = dev->driver->major;
 	retv.drm_dd_minor = dev->driver->minor;
 
-	if (copy_to_user(argp, &retv, sizeof(sv)))
+	if (copy_to_user(argp, &retv, sizeof(retv)))
 		return -EFAULT;
 
 	if (sv.drm_di_major != -1) {
 		if (sv.drm_di_major != DRM_IF_MAJOR ||
 		    sv.drm_di_minor < 0 || sv.drm_di_minor > DRM_IF_MINOR)
-			return EINVAL;
+			return -EINVAL;
 		if_version = DRM_IF_VERSION(sv.drm_di_major, sv.drm_di_minor);
 		dev->if_version = max(if_version, dev->if_version);
 		if (sv.drm_di_minor >= 1) {
 			/*
 			 * Version 1.1 includes tying of DRM to specific device
 			 */
-			drm_set_busid(dev);
+			int ret = drm_set_busid(dev);
+			if (ret)
+				return ret;
 		}
 	}
 
@@ -363,7 +365,7 @@ int drm_setversion(DRM_IOCTL_ARGS)
 		if (sv.drm_dd_major != dev->driver->major ||
 		    sv.drm_dd_minor < 0
 		    || sv.drm_dd_minor > dev->driver->minor)
-			return EINVAL;
+			return -EINVAL;
 
 		if (dev->driver->set_version)
 			dev->driver->set_version(dev, &sv);

Re: [mm patch] drm, minor fixes

[Arjan van de Ven]

On Sat, 2006-08-19 at 23:16 +0000, Frederik Deweerdt wrote:
> On Sun, Aug 13, 2006 at 01:24:54AM -0700, Andrew Morton wrote:
> > 
> > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm1/
> > 
> Hi Andrew,
> 
> The following patch adds minor fixes to the drm code:
> - fix return values that are wrong (return E* instead of return -E*)

are you sure the callers of these don't wrap it inside a DRM_ERR()
macro ?



- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [mm patch] drm, minor fixes

[Frederik Deweerdt]

On Sun, Aug 20, 2006 at 11:37:06AM +0200, Arjan van de Ven wrote:
> On Sat, 2006-08-19 at 23:16 +0000, Frederik Deweerdt wrote:
> > On Sun, Aug 13, 2006 at 01:24:54AM -0700, Andrew Morton wrote:
> > > 
> > > ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc4/2.6.18-rc4-mm1/
> > > 
> > Hi Andrew,
> > 
> > The following patch adds minor fixes to the drm code:
> > - fix return values that are wrong (return E* instead of return -E*)
> 
> are you sure the callers of these don't wrap it inside a DRM_ERR()
> macro ?
I changed the values when:
- I've checked what seemed right, getting back to the system call.
  drm_ioctl(), through a call to func().
  That's the case for:
  - the EFAULT value in i915_emit_box
  - two EINVAL values in drm_setversion
- the return value wasn't used. That was the case for
  drm_set_busid return values, I felt having returned values negative
  from the start was more consistent.

Is there a particular change that looked suspicious to you?
Thanks,
Frederik

Re: [mm patch] drm, minor fixes

[Dave Airlie]

>>
>> are you sure the callers of these don't wrap it inside a DRM_ERR()
>> macro ?
> I changed the values when:
> - I've checked what seemed right, getting back to the system call.
>  drm_ioctl(), through a call to func().
>  That's the case for:
>  - the EFAULT value in i915_emit_box
>  - two EINVAL values in drm_setversion
> - the return value wasn't used. That was the case for
>  drm_set_busid return values, I felt having returned values negative
>  from the start was more consistent.
>
> Is there a particular change that looked suspicious to you?

These are all actual bugs , however I doubt any of the codepaths are 
causing a major problem, a lot of those code paths are for older X 
systems or not very likely hit, I'll pull the fixes into the DRM tree 
now... the i915 one is a worry I must give out the TG/Intel folks :-)

Thanks,
Dave.

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

Re: [mm patch] drm, minor fixes

[Frederik Deweerdt]

(Sorry, I forgot to CCs.)
On Mon, Aug 21, 2006 at 12:22:42PM +0100, Dave Airlie wrote:
> >>
> >>are you sure the callers of these don't wrap it inside a DRM_ERR()
> >>macro ?
> >I changed the values when:
> >- I've checked what seemed right, getting back to the system call.
> > drm_ioctl(), through a call to func().
> > That's the case for:
> > - the EFAULT value in i915_emit_box
> > - two EINVAL values in drm_setversion
> >- the return value wasn't used. That was the case for
> > drm_set_busid return values, I felt having returned values negative
> > from the start was more consistent.
> >
> >Is there a particular change that looked suspicious to you?
> 
> [...], however I doubt any of the codepaths are causing a major problem [...]
I agree, I just corrected those because I happen to stumble upon them.
I've tried a kernel with those corrections alone, and it didn't changed
anything on the oopses.

Regards,
Frederik