[PATCH] Fix pointer tests in error-handling paths

[PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

From: Dave Gordon <david.s.gordon@intel.com>

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

  2682708 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5cb57f642ac1..8fd00d279447 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1655,7 +1655,7 @@ err:
 	 * must be freed again. If it was submitted then it is being tracked
 	 * on the active request list and no clean up is required here.
 	 */
-	if (ret && req)
+	if (ret && !IS_ERR_OR_NULL(req))
 		i915_gem_request_cancel(req);
 
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8104511ad302..b88cdac747eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 cleanup_unpin:
 	intel_unpin_fb_obj(fb, crtc->primary->state);
 cleanup_pending:
-	if (request)
+	if (!IS_ERR_OR_NULL(request))
 		i915_gem_request_cancel(request);
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.9.1

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 28/01/16 10:48, Tvrtko Ursulin wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
>
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
>
>    2682708 drm/i915: simplify allocation of driver-internal requests
>
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Re-sent for CI since we need this fix in ASAP.

Regards,

Tvrtko

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

✓ Fi.CI.BAT: success for Fix pointer tests in error-handling paths

[Patchwork]

== Summary ==

Series 2897v1 Fix pointer tests in error-handling paths
http://patchwork.freedesktop.org/api/1.0/series/2897/revisions/1/mbox/


bdw-ultra        total:159  pass:153  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:159  pass:135  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:159  pass:142  dwarn:0   dfail:0   fail:0   skip:17 
hsw-brixbox      total:159  pass:152  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:159  pass:155  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:159  pass:114  dwarn:0   dfail:0   fail:1   skip:44 
ivb-t430s        total:159  pass:151  dwarn:0   dfail:0   fail:0   skip:8  
skl-i5k-2        total:159  pass:150  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:159  pass:141  dwarn:0   dfail:0   fail:0   skip:18 
snb-x220t        total:159  pass:141  dwarn:0   dfail:0   fail:1   skip:17 

Results at /archive/results/CI_IGT_test/Patchwork_1310/

b3f8ad64bc71f6236f05c2e9f4ad49a61745869a drm-intel-nightly: 2016y-01m-28d-10h-26m-23s UTC integration manifest
17a070b9575bfb61ec47bba7fef32a5e3375bf7d Fix pointer tests in error-handling paths

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Dave Gordon]

On 22/01/16 13:34, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote:
>>
>> On 22/01/16 13:01, Chris Wilson wrote:
>>> On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
>>>> In the error-handling paths of i915_gem_do_execbuffer() and
>>>> intel_crtc_page_flip(), the local pointer-to-request variables
>>>> were expected to be either valid pointers or NULL. Since
>>>>
>>>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>>>
>>>> they could also be ERR_PTR() values, so the tests need to be
>>>> updated to accommodate this case.
>>>>
>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Quick quiz, name at least testcase that exercised this code?
>>
>> gem_close_race did it for me, but can you explain the weird ERR_PTR
>> of *ffca ? :)
>
> -54
>
> That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc.
> -Chris

Assuming that's the fault address, it will actually be the sum of the 
errno mistakenly passed to i915_gem_request_cancel(), plus the offset of 
'ringbuf' in the drm_i915_gem_request structure:

void i915_gem_request_cancel(struct drm_i915_gem_request *req)
{
         intel_ring_reserved_space_cancel(req->ringbuf);

         i915_gem_request_unreference(req);
}

As it happens, 'ringbuf' is at offset 0x38 (56), so the errno was -2,
which is the expected error -ENOENT :)

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 25/01/16 16:28, Daniel Vetter wrote:
> On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote:
>>
>> On 22/01/16 12:19, Dave Gordon wrote:
>>> In the error-handling paths of i915_gem_do_execbuffer() and
>>> intel_crtc_page_flip(), the local pointer-to-request variables
>>> were expected to be either valid pointers or NULL. Since
>>>
>>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>>
>>> they could also be ERR_PTR() values, so the tests need to be
>>> updated to accommodate this case.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>>>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Not sure if CI will pick up a new patch in an old series.
>
> I think it'll treat this one as a replacement for patch 1/4 and then ofc
> totally fall over. So would need a resend of the entire pile.

The rest of the pile has been merged already so I think just this patch 
on its own then (not as in-reply-to anything).

Regards,

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Daniel Vetter]

On Fri, Jan 22, 2016 at 01:07:48PM +0000, Tvrtko Ursulin wrote:
> 
> On 22/01/16 12:19, Dave Gordon wrote:
> >In the error-handling paths of i915_gem_do_execbuffer() and
> >intel_crtc_page_flip(), the local pointer-to-request variables
> >were expected to be either valid pointers or NULL. Since
> >
> >   2682708 drm/i915: simplify allocation of driver-internal requests
> >
> >they could also be ERR_PTR() values, so the tests need to be
> >updated to accommodate this case.
> >
> >Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> >  drivers/gpu/drm/i915/intel_display.c       | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Not sure if CI will pick up a new patch in an old series.

I think it'll treat this one as a replacement for patch 1/4 and then ofc
totally fall over. So would need a resend of the entire pile.
-Daniel

> 
> Anyway:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
> 
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >index 2dc08ce..a7bd555 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
> >  	 * must be freed again. If it was submitted then it is being tracked
> >  	 * on the active request list and no clean up is required here.
> >  	 */
> >-	if (ret && req)
> >+	if (ret && !IS_ERR_OR_NULL(req))
> >  		i915_gem_request_cancel(req);
> >
> >  	mutex_unlock(&dev->struct_mutex);
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 8104511..b88cdac 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  cleanup_unpin:
> >  	intel_unpin_fb_obj(fb, crtc->primary->state);
> >  cleanup_pending:
> >-	if (request)
> >+	if (!IS_ERR_OR_NULL(request))
> >  		i915_gem_request_cancel(request);
> >  	atomic_dec(&intel_crtc->unpin_work_count);
> >  	mutex_unlock(&dev->struct_mutex);
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Fix pointer tests in error-handling paths

[Chris Wilson]

On Fri, Jan 22, 2016 at 01:17:44PM +0000, Tvrtko Ursulin wrote:
> 
> On 22/01/16 13:01, Chris Wilson wrote:
> >On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
> >>In the error-handling paths of i915_gem_do_execbuffer() and
> >>intel_crtc_page_flip(), the local pointer-to-request variables
> >>were expected to be either valid pointers or NULL. Since
> >>
> >>   2682708 drm/i915: simplify allocation of driver-internal requests
> >>
> >>they could also be ERR_PTR() values, so the tests need to be
> >>updated to accommodate this case.
> >>
> >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >Quick quiz, name at least testcase that exercised this code?
> 
> gem_close_race did it for me, but can you explain the weird ERR_PTR
> of *ffca ? :)

-54

That's odd. gem_close_race should be a mix of ENOENT, EINVAL iirc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 22/01/16 13:01, Chris Wilson wrote:
> On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
>> In the error-handling paths of i915_gem_do_execbuffer() and
>> intel_crtc_page_flip(), the local pointer-to-request variables
>> were expected to be either valid pointers or NULL. Since
>>
>>    2682708 drm/i915: simplify allocation of driver-internal requests
>>
>> they could also be ERR_PTR() values, so the tests need to be
>> updated to accommodate this case.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Quick quiz, name at least testcase that exercised this code?

gem_close_race did it for me, but can you explain the weird ERR_PTR of 
*ffca ? :)

Regards,

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

Re: [PATCH] Fix pointer tests in error-handling paths

[Tvrtko Ursulin]

On 22/01/16 12:19, Dave Gordon wrote:
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
>
>    2682708 drm/i915: simplify allocation of driver-internal requests
>
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>   drivers/gpu/drm/i915/intel_display.c       | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Not sure if CI will pick up a new patch in an old series.

Anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2dc08ce..a7bd555 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
>   	 * must be freed again. If it was submitted then it is being tracked
>   	 * on the active request list and no clean up is required here.
>   	 */
> -	if (ret && req)
> +	if (ret && !IS_ERR_OR_NULL(req))
>   		i915_gem_request_cancel(req);
>
>   	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8104511..b88cdac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>   cleanup_unpin:
>   	intel_unpin_fb_obj(fb, crtc->primary->state);
>   cleanup_pending:
> -	if (request)
> +	if (!IS_ERR_OR_NULL(request))
>   		i915_gem_request_cancel(request);
>   	atomic_dec(&intel_crtc->unpin_work_count);
>   	mutex_unlock(&dev->struct_mutex);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Fix pointer tests in error-handling paths

[Chris Wilson]

On Fri, Jan 22, 2016 at 12:19:32PM +0000, Dave Gordon wrote:
> In the error-handling paths of i915_gem_do_execbuffer() and
> intel_crtc_page_flip(), the local pointer-to-request variables
> were expected to be either valid pointers or NULL. Since
> 
>   2682708 drm/i915: simplify allocation of driver-internal requests
> 
> they could also be ERR_PTR() values, so the tests need to be
> updated to accommodate this case.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Quick quiz, name at least testcase that exercised this code?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] Fix pointer tests in error-handling paths

[Dave Gordon]

In the error-handling paths of i915_gem_do_execbuffer() and
intel_crtc_page_flip(), the local pointer-to-request variables
were expected to be either valid pointers or NULL. Since

  2682708 drm/i915: simplify allocation of driver-internal requests

they could also be ERR_PTR() values, so the tests need to be
updated to accommodate this case.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/intel_display.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2dc08ce..a7bd555 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1654,7 +1654,7 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	 * must be freed again. If it was submitted then it is being tracked
 	 * on the active request list and no clean up is required here.
 	 */
-	if (ret && req)
+	if (ret && !IS_ERR_OR_NULL(req))
 		i915_gem_request_cancel(req);
 
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8104511..b88cdac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11726,7 +11726,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 cleanup_unpin:
 	intel_unpin_fb_obj(fb, crtc->primary->state);
 cleanup_pending:
-	if (request)
+	if (!IS_ERR_OR_NULL(request))
 		i915_gem_request_cancel(request);
 	atomic_dec(&intel_crtc->unpin_work_count);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.9.1

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