[bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Dan Carpenter]

Hello Himal Prasad Ghimiray,

Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
ranges") from May 13, 2025 (linux-next), leads to the following
Smatch static checker warning:

	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'

drivers/gpu/drm/xe/xe_vm.c
    2917 
    2918                 err = xe_svm_range_get_pages(vm, svm_range, &ctx);
    2919                 if (err) {
    2920                         if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM)
    2921                                 err = -ENODATA;
--> 2922                         drm_dbg(&vm->xe->drm, "Get pages failed, asid=%u, gpusvm=%p, errno=%pe\n",
    2923                                 vm->usm.asid, &vm->svm.gpusvm, ERR_PTR(err));

The comments on walk_page_range() say it can return > 0 on success but
the comments on hmm_range_fault() say it can never return > 0.  Smatch
does a naive reading of the code and thinks that it can return > 0.

Presumably the comments are correct but the code is too tricky for me.

I can easily silence this in Smatch by adding deleting the positive
returns from hmm_range_fault() from the cross function DB.  Can someone
confirm that's the correct thing to do?

    2924                         return err;
    2925                 }
    2926                 xe_svm_range_debug(svm_range, "PREFETCH - RANGE GET PAGES DONE");
    2927         }
    2928 
    2929         return err;
    2930 }

regards,
dan carpenter

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Ghimiray, Himal Prasad]

On 26-05-2025 20:36, Dan Carpenter wrote:
> Hello Himal Prasad Ghimiray,
> 
> Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
> ranges") from May 13, 2025 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
> 	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'

Hi Dan,

Thanks for pointing this out. I see there's a gap in how 
hmm_range_fault() adheres to its documented behavior. I believe the 
function should sanitize positive return values from walk_page_range() 
to ensure consistency.

Jason can comment further on same.
  >
> drivers/gpu/drm/xe/xe_vm.c
>      2917
>      2918                 err = xe_svm_range_get_pages(vm, svm_range, &ctx);
>      2919                 if (err) {
>      2920                         if (err == -EOPNOTSUPP || err == -EFAULT || err == -EPERM)
>      2921                                 err = -ENODATA;
> --> 2922                         drm_dbg(&vm->xe->drm, "Get pages failed, asid=%u, gpusvm=%p, errno=%pe\n",
>      2923                                 vm->usm.asid, &vm->svm.gpusvm, ERR_PTR(err));
> 
> The comments on walk_page_range() say it can return > 0 on success but
> the comments on hmm_range_fault() say it can never return > 0.  Smatch
> does a naive reading of the code and thinks that it can return > 0.
> 
> Presumably the comments are correct but the code is too tricky for me.
> 
> I can easily silence this in Smatch by adding deleting the positive
> returns from hmm_range_fault() from the cross function DB.  Can someone
> confirm that's the correct thing to do?
> 
>      2924                         return err;
>      2925                 }
>      2926                 xe_svm_range_debug(svm_range, "PREFETCH - RANGE GET PAGES DONE");
>      2927         }
>      2928
>      2929         return err;
>      2930 }
> 
> regards,
> dan carpenter

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Jason Gunthorpe]

On Mon, May 26, 2025 at 10:15:17PM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 26-05-2025 20:36, Dan Carpenter wrote:
> > Hello Himal Prasad Ghimiray,
> > 
> > Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
> > ranges") from May 13, 2025 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
> > 	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'
> 
> Hi Dan,
> 
> Thanks for pointing this out. I see there's a gap in how hmm_range_fault()
> adheres to its documented behavior. I believe the function should sanitize
> positive return values from walk_page_range() to ensure consistency.
> 
> Jason can comment further on same.

Yeah, I don't think it should return positive error code, whatever is
doing that should be fixed. Can you send a patch?

Jason

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Simona Vetter]

On Tue, Jun 03, 2025 at 07:29:52PM -0300, Jason Gunthorpe wrote:
> On Mon, May 26, 2025 at 10:15:17PM +0530, Ghimiray, Himal Prasad wrote:
> > 
> > 
> > On 26-05-2025 20:36, Dan Carpenter wrote:
> > > Hello Himal Prasad Ghimiray,
> > > 
> > > Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
> > > ranges") from May 13, 2025 (linux-next), leads to the following
> > > Smatch static checker warning:
> > > 
> > > 	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
> > > 	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'
> > 
> > Hi Dan,
> > 
> > Thanks for pointing this out. I see there's a gap in how hmm_range_fault()
> > adheres to its documented behavior. I believe the function should sanitize
> > positive return values from walk_page_range() to ensure consistency.
> > 
> > Jason can comment further on same.
> 
> Yeah, I don't think it should return positive error code, whatever is
> doing that should be fixed. Can you send a patch?

Not sure that's what's going on, from the comment and reading the code
(albeit non-exhaustively) I think you can only get positive error return
values from walk_page_range if the ops you provide do so. The hmm ones
don't, so I think this should be ok without any code changes?

Maybe a WARN_ON and patching that up for paranoia, but I don't see how
this can happen.

Cheers, Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Jason Gunthorpe]

On Wed, Jun 04, 2025 at 04:54:43PM +0200, Simona Vetter wrote:
> On Tue, Jun 03, 2025 at 07:29:52PM -0300, Jason Gunthorpe wrote:
> > On Mon, May 26, 2025 at 10:15:17PM +0530, Ghimiray, Himal Prasad wrote:
> > > 
> > > 
> > > On 26-05-2025 20:36, Dan Carpenter wrote:
> > > > Hello Himal Prasad Ghimiray,
> > > > 
> > > > Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
> > > > ranges") from May 13, 2025 (linux-next), leads to the following
> > > > Smatch static checker warning:
> > > > 
> > > > 	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
> > > > 	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'
> > > 
> > > Hi Dan,
> > > 
> > > Thanks for pointing this out. I see there's a gap in how hmm_range_fault()
> > > adheres to its documented behavior. I believe the function should sanitize
> > > positive return values from walk_page_range() to ensure consistency.
> > > 
> > > Jason can comment further on same.
> > 
> > Yeah, I don't think it should return positive error code, whatever is
> > doing that should be fixed. Can you send a patch?
> 
> Not sure that's what's going on, from the comment and reading the code
> (albeit non-exhaustively) I think you can only get positive error return
> values from walk_page_range if the ops you provide do so. The hmm ones
> don't, so I think this should be ok without any code changes?
> 
> Maybe a WARN_ON and patching that up for paranoia, but I don't see how
> this can happen.

Yeah, that is how I suspect it was too, but I vaugely recall something
like this coming up before in hmm and the right answer was to fix the
thing generating the wrong return code, not try to fix it higher.

A WARN_ON would be reasonable

Jason

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Dan Carpenter]

On Wed, Jun 04, 2025 at 04:54:43PM +0200, Simona Vetter wrote:
> On Tue, Jun 03, 2025 at 07:29:52PM -0300, Jason Gunthorpe wrote:
> > On Mon, May 26, 2025 at 10:15:17PM +0530, Ghimiray, Himal Prasad wrote:
> > > 
> > > 
> > > On 26-05-2025 20:36, Dan Carpenter wrote:
> > > > Hello Himal Prasad Ghimiray,
> > > > 
> > > > Commit 09ba0a8f06cd ("drm/xe/svm: Implement prefetch support for SVM
> > > > ranges") from May 13, 2025 (linux-next), leads to the following
> > > > Smatch static checker warning:
> > > > 
> > > > 	drivers/gpu/drm/xe/xe_vm.c:2922 prefetch_ranges()
> > > > 	warn: passing positive error code 's32min-(-96),(-94)-(-15),(-13)-(-12),(-10)-(-2),1' to 'ERR_PTR'
> > > 
> > > Hi Dan,
> > > 
> > > Thanks for pointing this out. I see there's a gap in how hmm_range_fault()
> > > adheres to its documented behavior. I believe the function should sanitize
> > > positive return values from walk_page_range() to ensure consistency.
> > > 
> > > Jason can comment further on same.
> > 
> > Yeah, I don't think it should return positive error code, whatever is
> > doing that should be fixed. Can you send a patch?
> 
> Not sure that's what's going on, from the comment and reading the code
> (albeit non-exhaustively) I think you can only get positive error return
> values from walk_page_range if the ops you provide do so. The hmm ones
> don't, so I think this should be ok without any code changes?
> 
> Maybe a WARN_ON and patching that up for paranoia, but I don't see how
> this can happen.
> 

Thanks.

A comment is enough probably.  A WARN_ON() just bloats the code and
it doesn't silence the warning.  I'm going to have to add a line to
the smatch_data/db/kernel.return_fixes to tell smatch that
hmm_range_fault() doesn't return postives.

regards,
dan carpenter

Re: [bug report] drm/xe/svm: Implement prefetch support for SVM ranges

[Dan Carpenter]

On Wed, Jun 04, 2025 at 11:56:57AM -0300, Jason Gunthorpe wrote:
> 
> A WARN_ON would be reasonable

Or we could add a WARN_ON().  That's also fine.

regards,
dan carpenter