Re: [git pull] drm patches for 2.6.27-rc1

[Eric Anholt]

On Fri, 2008-10-17 at 15:43 -0700, Linus Torvalds wrote:
> 
> 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!

Yeah, none of us are on x86-64, so we missed those warnings in testing.
The change looks pretty good except for s/return 1/return 0/.  We wanted
to pull the i_wish_it_was_ioremap_atomic() hack out so that we could use
it for relocation updates as well (which aren't copy_from_user), and
I've started writing that patch.  Expect something pushed at you soon.

-- 
Eric Anholt
eric@xxxxxxxxxx                         eric.anholt@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part