Re: [Intel-gfx] [PATCH] drm/i915/ttm: Fix access_memory null pointer exception

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
> Suggested-by: John C Harrison <John.C.Harrison@intel.com>
> CC: Matthew Auld <matthew.auld@intel.com>
> CC: Andrzej Hajda <andrzej.hajda@intel.com>
> CC: Nirmoy Das <nirmoy.das@intel.com>
> CC: Andi Shyti <andi.shyti@linux.intel.com>
> ---
>   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