On Thu, Feb 03, 2022 at 12:44:57PM -0800, John Hubbard wrote:
On 2/3/22 05:01, Jason Gunthorpe wrote:
...
In the new branch if (pages), you set page = ERR_PTR(-EFAULT) and goto
out. However, at the label out, the value of page is not used, but the
return uses the variables i and ret.
Yes, I think that the complaint is accurate. The intent of this code is
to return either number of pages so far (i) or ret (which should be zero
in this case), because we are just stopping early, rather than calling
this an actual error.
IIRC GUP shouldn't return 0, it should return an error code, not zero.
Jason
Errors work for single pages, but GUP is a multi-page API call. If it
returned an error part way through the list of pages, then callers would
have no way of knowing how many pages to release.
Yes, but that is returning a positive error code, I said it should not
return zero.
When it hits an error with pages already loaded it returns that number
and the caller will then do gup once more with the VA pointing at the
problematic page. Then GUP can return the error code because it has 0
pages on the next iteration.
It should not return 0 here when it got an error.
* Returns either number of pages pinned (which may be less than the
* number requested), or an error. Details about the return value:
*
* -- If nr_pages is 0, returns 0.
* -- If nr_pages is >0, but no pages were pinned, returns -errno.
* -- If nr_pages is >0, and some pages were pinned, returns the number of
* pages pinned. Again, this may be less than nr_pages.
* -- 0 return value is possible when the fault would need to be retried.
I actually don't know of any place that handles the 0 return code, or
what 'fault would need to be retried' is supposed to mean for the
caller ...