Hi,
A couple nit picks for the benefit of coding style.
On 13/10/2022 18:56, Jonathan Cavitt wrote:
i915_ttm_to_gem can return a NULL pointer, which is
dereferenced in i915_ttm_access_memory without first
checking if it is NULL. Inspecting
i915_ttm_io_mem_reserve, it appears the correct
behavior in this case is to return -EINVAL.
Too narrow wrap - see kernel's submitting-patches.rst:
- The body of the explanation, line wrapped at 75 columns, which will
be copied to the permanent changelog to describe this patch.
Fixes: 26b15eb0 ("drm/i915/ttm: implement access_memory")
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
Suggested-by: John C Harrison <John.C.Harrison@xxxxxxxxx>
CC: Matthew Auld <matthew.auld@xxxxxxxxx>
CC: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
CC: Nirmoy Das <nirmoy.das@xxxxxxxxx>
CC: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index d63f30efd631..b569624f2ed9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -704,11 +704,16 @@ static int i915_ttm_access_memory(struct ttm_buffer_object *bo,
int len, int write)
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- resource_size_t iomap = obj->mm.region->iomap.base -
- obj->mm.region->region.start;
+ resource_size_t iomap;
unsigned long page = offset >> PAGE_SHIFT;
unsigned long bytes_left = len;
We tend to re-order the declarations from long to narrow, where not too cumbersome, just for that extra readability.
+ if (!obj)
+ return -EINVAL;
Someone perhaps can update the vfunc kerneldoc since it appears to be overly strict in allowed return codes.
include/drm/ttm/ttm_device.h:
/**
* Read/write memory buffers for ptrace access
*
* @bo: the BO to access
* @offset: the offset from the start of the BO
* @buf: pointer to source/destination buffer
* @len: number of bytes to copy
* @write: whether to read (0) from or write (non-0) to BO
*
* If successful, this function should return the number of
* bytes copied, -EIO otherwise. If the number of bytes
* returned is < len, the function may be called again with
* the remainder of the buffer to copy.
*/
int (*access_memory)(struct ttm_buffer_object *bo, unsigned long offset,
void *buf, int len, int write);
Regards,
Tvrtko
+
+ iomap = obj->mm.region->iomap.base -
+ obj->mm.region->region.start;
+
/*
* TODO: For now just let it fail if the resource is non-mappable,
* otherwise we need to perform the memcpy from the gpu here, without
