| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Oct 2008 15:43:02 -0700 (PDT)
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Dave Airlie <airlied@...ux.ie>
cc: Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, dri-devel@...ts.sf.net
Subject: Re: [git pull] drm patches for 2.6.27-rc1
On Fri, 17 Oct 2008, Dave Airlie wrote:
>
> Please pull the 'drm-next' branch from
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-next
Grr.
This whole merge series has been full of people sending me UNTESTED CRAP.
So what's the excuse _this_ time for adding all these stupid warnings to
my build log? Did nobody test this?
drivers/gpu/drm/drm_proc.c: In function ‘drm_gem_one_name_info’:
drivers/gpu/drm/drm_proc.c:525: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’
drivers/gpu/drm/drm_proc.c:533: warning: format ‘%9d’ expects type ‘int’, but argument 4 has type ‘size_t’
drivers/gpu/drm/i915/i915_gem.c: In function ‘i915_gem_gtt_pwrite’:
drivers/gpu/drm/i915/i915_gem.c:184: warning: unused variable ‘vaddr_atomic’
and I wonder how many other warnings got added that I never even noticed
because I don't build them?
And yes, it's not just warnings. One of thse is horribly bad code:
nid->len += sprintf(&nid->buf[nid->len],
"%6d%9d%8d%9d\n",
obj->name, obj->size,
atomic_read(&obj->handlecount.refcount),
atomic_read(&obj->refcount.refcount));
where it's just wrong to use the field width as a separator. Because if
the counts are big enough, the separator suddenly goes away!
So that format string should be
"%6d %8zd %7d %8d\n"
instead. Which gives the same output when you don't overflow, and doesn't
have the overflow bug when you do.
As to that "vaddr_atomic" thing, the warning would have been avoided if
you had just cleanly split up the optimistic fast case.
IOW, write cleaner code, and the warning just goes away on its own.
Something like the appended. UNTESTED!
Hmm?
I really wish people were more careful, and took more pride in trying to
write readable code, with small modular functions instead. And move those
variables down to the block they are needed in.
Anyway, I pulled the thing, but _please_ test this cleanup and send it
back to me if it passes your testing. Ok?
Linus
---
drivers/gpu/drm/drm_proc.c | 4 +-
drivers/gpu/drm/i915/i915_gem.c | 59 +++++++++++++++++++++++---------------
2 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_proc.c b/drivers/gpu/drm/drm_proc.c
index d490db4..ae73b7f 100644
--- a/drivers/gpu/drm/drm_proc.c
+++ b/drivers/gpu/drm/drm_proc.c
@@ -522,12 +522,12 @@ static int drm_gem_one_name_info(int id, void *ptr, void *data)
struct drm_gem_object *obj = ptr;
struct drm_gem_name_info_data *nid = data;
- DRM_INFO("name %d size %d\n", obj->name, obj->size);
+ DRM_INFO("name %d size %zd\n", obj->name, obj->size);
if (nid->eof)
return 0;
nid->len += sprintf(&nid->buf[nid->len],
- "%6d%9d%8d%9d\n",
+ "%6d %8zd %7d %8d\n",
obj->name, obj->size,
atomic_read(&obj->handlecount.refcount),
atomic_read(&obj->refcount.refcount));
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9ac73dd..b8c8b2e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -171,6 +171,36 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
return 0;
}
+/*
+ * Try to write quickly with an atomic kmap. Return true on success.
+ *
+ * If this fails (which includes a partial write), we'll redo the whole
+ * thing with the slow version.
+ *
+ * This is a workaround for the low performance of iounmap (approximate
+ * 10% cpu cost on normal 3D workloads). kmap_atomic on HIGHMEM kernels
+ * happens to let us map card memory without taking IPIs. When the vmap
+ * rework lands we should be able to dump this hack.
+ */
+static inline int fast_user_write(unsigned long pfn, char __user *user_data, int l)
+{
+#ifdef CONFIG_HIGHMEM
+ unsigned long unwritten;
+ char *vaddr_atomic;
+
+ vaddr_atomic = kmap_atomic_pfn(pfn, KM_USER0);
+#if WATCH_PWRITE
+ DRM_INFO("pwrite i %d o %d l %d pfn %ld vaddr %p\n",
+ i, o, l, pfn, vaddr_atomic);
+#endif
+ unwritten = __copy_from_user_inatomic_nocache(vaddr_atomic + o, user_data, l);
+ kunmap_atomic(vaddr_atomic, KM_USER0);
+ return !unwritten;
+#else
+ return 1;
+#endif
+}
+
static int
i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
struct drm_i915_gem_pwrite *args,
@@ -180,12 +210,7 @@ i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
ssize_t remain;
loff_t offset;
char __user *user_data;
- char __iomem *vaddr;
- char *vaddr_atomic;
- int i, o, l;
int ret = 0;
- unsigned long pfn;
- unsigned long unwritten;
user_data = (char __user *) (uintptr_t) args->data_ptr;
remain = args->size;
@@ -209,6 +234,9 @@ i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
obj_priv->dirty = 1;
while (remain > 0) {
+ unsigned long pfn;
+ int i, o, l;
+
/* Operation in this page
*
* i = page number
@@ -223,25 +251,10 @@ i915_gem_gtt_pwrite(struct drm_device *dev, struct drm_gem_object *obj,
pfn = (dev->agp->base >> PAGE_SHIFT) + i;
-#ifdef CONFIG_HIGHMEM
- /* This is a workaround for the low performance of iounmap
- * (approximate 10% cpu cost on normal 3D workloads).
- * kmap_atomic on HIGHMEM kernels happens to let us map card
- * memory without taking IPIs. When the vmap rework lands
- * we should be able to dump this hack.
- */
- vaddr_atomic = kmap_atomic_pfn(pfn, KM_USER0);
-#if WATCH_PWRITE
- DRM_INFO("pwrite i %d o %d l %d pfn %ld vaddr %p\n",
- i, o, l, pfn, vaddr_atomic);
-#endif
- unwritten = __copy_from_user_inatomic_nocache(vaddr_atomic + o,
- user_data, l);
- kunmap_atomic(vaddr_atomic, KM_USER0);
+ if (!fast_user_write(pfn, user_data, l)) {
+ unsigned long unwritten;
+ char __iomem *vaddr;
- if (unwritten)
-#endif /* CONFIG_HIGHMEM */
- {
vaddr = ioremap_wc(pfn << PAGE_SHIFT, PAGE_SIZE);
#if WATCH_PWRITE
DRM_INFO("pwrite slow i %d o %d l %d "
--
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