On 4/16/22 2:04 PM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
On 4/16/22 11:33 AM, Joe Perches wrote:
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
In insert_mappable_node(), the parameter node is cleared late in node's use with memset. insert_mappable_node() is a singleton, called only from i915_gem_gtt_prepare() which itself is only called by i915_gem_gtt_pread() and i915_gem_gtt_pwrite_fast() where the definition of node originates.
Instead of using memset, initialize node to 0 at it's definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is the struct available to userspace.
So this seems fine though it might increase overall code size a tiny bit.
I do have a caveat: see below:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
@@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct drm_i915_gem_object *obj, goto err_ww; } else if (!IS_ERR(vma)) { node->start = i915_ggtt_offset(vma);
node->flags = 0;Why is this unneeded?
node = {} initializes all of node's elements to 0, including this one.
true, but could the call to insert_mappable_node combined with the retry goto in i915_gem_gtt_prepare set this to non-zero?
Yikes!
I'll add that back.
Thanks for pointing it out.
Tom