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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux