Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

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

On 07/02/2015 12:00 PM, Chris Wilson wrote:
>>> +		/* This is a slow read/write as it tries to read from
>>> +		 * and write to user memory which may result into page
>>> +		 * faults
>>> +		 */
>>> +		ret = slow_user_access(dev_priv->gtt.mappable, page_base,
>>> +				       page_offset, user_data,
>>> +				       page_length, write);
>>> +
>>> +		if (ret) {
>>> +			ret = -EINVAL;
>
> Would be better to return the right errno from slow_user_access or if it
> is a bool, treat is a bool!

It returns a number of unwritten bytes.

Actually just spotted the return type for slow_user_access is also 
wrong, it is an int, to which it implicitly casts an unsigned long.

And it is customary to return -EFAULT on rather than -EINVAL so I 
suggest changing that as well.

>>> +			break;
>>> +		}
>>> +
>>> +		remain -= page_length;
>>> +		user_data += page_length;
>>> +		offset += page_length;
>>> +	}
>>> +
>>> +	mutex_lock(&dev->struct_mutex);
>>
>> Caller had it interruptible.
>
> On second access, it is preferrable not to risk interruption -
> especially when catching up with state to return to userspace.


Second access is only in pwrite case and more so, it makes no sense to 
make such decisions on behalf of the caller. If it is a real concern, 
caller should do it.

Yes I see that there is a problem where this helper doesn't know what 
type of mutex caller had, but, it is still bad imho and could be fixed 
in a different way.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx