All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-03 19:30 ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-03 19:30 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel
  Cc: Saurabh Singh Sengar, Praveen Kumar, Deepak R Varma

The macro definition of gen6_for_all_pdes() expands to a for loop such
that it breaks when the page table is null. Hence there is no need to
again test validity of the page table entry pointers in the pde list.
This change is identified using itnull.cocci semantic patch.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please note: Proposed change is compile tested only.

 drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
index 5aaacc53fa4c..787b9e6d9f59 100644
--- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
@@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
 	u32 pde;
 
 	gen6_for_all_pdes(pt, pd, pde)
-		if (pt)
-			free_pt(&ppgtt->base.vm, pt);
+		free_pt(&ppgtt->base.vm, pt);
 }
 
 static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
@@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
 
 	/* Free all no longer used page tables */
 	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
-		if (!pt || atomic_read(&pt->used))
+		if (atomic_read(&pt->used))
 			continue;
 
 		free_pt(&ppgtt->base.vm, pt);
-- 
2.34.1




^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-03 19:30 ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-03 19:30 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel
  Cc: Praveen Kumar, Deepak R Varma, Saurabh Singh Sengar

The macro definition of gen6_for_all_pdes() expands to a for loop such
that it breaks when the page table is null. Hence there is no need to
again test validity of the page table entry pointers in the pde list.
This change is identified using itnull.cocci semantic patch.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please note: Proposed change is compile tested only.

 drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
index 5aaacc53fa4c..787b9e6d9f59 100644
--- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
@@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
 	u32 pde;
 
 	gen6_for_all_pdes(pt, pd, pde)
-		if (pt)
-			free_pt(&ppgtt->base.vm, pt);
+		free_pt(&ppgtt->base.vm, pt);
 }
 
 static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
@@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
 
 	/* Free all no longer used page tables */
 	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
-		if (!pt || atomic_read(&pt->used))
+		if (atomic_read(&pt->used))
 			continue;
 
 		free_pt(&ppgtt->base.vm, pt);
-- 
2.34.1




^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-03 19:30 ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-03 19:30 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel
  Cc: Praveen Kumar, Deepak R Varma, Saurabh Singh Sengar

The macro definition of gen6_for_all_pdes() expands to a for loop such
that it breaks when the page table is null. Hence there is no need to
again test validity of the page table entry pointers in the pde list.
This change is identified using itnull.cocci semantic patch.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Please note: Proposed change is compile tested only.

 drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
index 5aaacc53fa4c..787b9e6d9f59 100644
--- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
+++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
@@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
 	u32 pde;
 
 	gen6_for_all_pdes(pt, pd, pde)
-		if (pt)
-			free_pt(&ppgtt->base.vm, pt);
+		free_pt(&ppgtt->base.vm, pt);
 }
 
 static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
@@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
 
 	/* Free all no longer used page tables */
 	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
-		if (!pt || atomic_read(&pt->used))
+		if (atomic_read(&pt->used))
 			continue;
 
 		free_pt(&ppgtt->base.vm, pt);
-- 
2.34.1




^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gt: Avoid redundant pointer validity check
  2023-02-03 19:30 ` Deepak R Varma
  (?)
  (?)
@ 2023-02-03 20:33 ` Patchwork
  -1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2023-02-03 20:33 UTC (permalink / raw)
  To: Deepak R Varma; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 3804 bytes --]

== Series Details ==

Series: drm/i915/gt: Avoid redundant pointer validity check
URL   : https://patchwork.freedesktop.org/series/113670/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12692 -> Patchwork_113670v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/index.html

Participating hosts (26 -> 25)
------------------------------

  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_113670v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
    - fi-bsw-n3050:       [PASS][1] -> [FAIL][2] ([i915#6298])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - {bat-rpls-1}:       [ABORT][3] ([i915#6311]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/bat-rpls-1/igt@gem_exec_suspend@basic-s0@smem.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/bat-rpls-1/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@i915_pm_rpm@basic-rte:
    - {bat-adln-1}:       [ABORT][5] ([i915#7977]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/bat-adln-1/igt@i915_pm_rpm@basic-rte.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/bat-adln-1/igt@i915_pm_rpm@basic-rte.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-edp-1:
    - {bat-rplp-1}:       [DMESG-WARN][7] -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/bat-rplp-1/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-edp-1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/bat-rplp-1/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-edp-1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7977]: https://gitlab.freedesktop.org/drm/intel/issues/7977
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978


Build changes
-------------

  * Linux: CI_DRM_12692 -> Patchwork_113670v1

  CI-20190529: 20190529
  CI_DRM_12692: f3800283e8ba9f8e93936a76458e3afa14e38149 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7149: 1c7ea154b625e1fb826f1519b816b4256dd10b62 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113670v1: f3800283e8ba9f8e93936a76458e3afa14e38149 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

2c024e3ad4bb drm/i915/gt: Avoid redundant pointer validity check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/index.html

[-- Attachment #2: Type: text/html, Size: 3893 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/gt: Avoid redundant pointer validity check
  2023-02-03 19:30 ` Deepak R Varma
                   ` (2 preceding siblings ...)
  (?)
@ 2023-02-05  8:18 ` Patchwork
  -1 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2023-02-05  8:18 UTC (permalink / raw)
  To: Deepak R Varma; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 21156 bytes --]

== Series Details ==

Series: drm/i915/gt: Avoid redundant pointer validity check
URL   : https://patchwork.freedesktop.org/series/113670/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12692_full -> Patchwork_113670v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/index.html

Participating hosts (9 -> 10)
------------------------------

  Additional (1): shard-rkl0 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_113670v1_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@syncobj_wait@wait-for-submit-snapshot:
    - {shard-dg1}:        NOTRUN -> [DMESG-WARN][1] +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-dg1-15/igt@syncobj_wait@wait-for-submit-snapshot.html

  * {igt@v3d/v3d_submit_cl@bad-pad}:
    - {shard-dg1}:        NOTRUN -> [SKIP][2] +4 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-dg1-15/igt@v3d/v3d_submit_cl@bad-pad.html

  
Known issues
------------

  Here are the changes found in Patchwork_113670v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-none@rcs0:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-glk5/igt@gem_exec_fair@basic-none@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-glk5/igt@gem_exec_fair@basic-none@rcs0.html

  * igt@gen7_exec_parse@basic-offset:
    - shard-glk:          NOTRUN -> [SKIP][5] ([fdo#109271]) +4 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-glk7/igt@gen7_exec_parse@basic-offset.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@idle@rcs0:
    - {shard-rkl}:        [FAIL][6] ([i915#7742]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-4/igt@drm_fdinfo@idle@rcs0.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-2/igt@drm_fdinfo@idle@rcs0.html

  * igt@gem_ctx_persistence@legacy-engines-hang@blt:
    - {shard-rkl}:        [SKIP][8] ([i915#6252]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-5/igt@gem_ctx_persistence@legacy-engines-hang@blt.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-3/igt@gem_ctx_persistence@legacy-engines-hang@blt.html

  * igt@gem_exec_capture@pi@vcs0:
    - {shard-rkl}:        [ABORT][10] -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-1/igt@gem_exec_capture@pi@vcs0.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-4/igt@gem_exec_capture@pi@vcs0.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-glk:          [FAIL][12] ([i915#2842]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-glk6/igt@gem_exec_fair@basic-none-share@rcs0.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-glk5/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_reloc@basic-wc-read:
    - {shard-rkl}:        [SKIP][14] ([i915#3281]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-2/igt@gem_exec_reloc@basic-wc-read.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-5/igt@gem_exec_reloc@basic-wc-read.html

  * igt@gem_partial_pwrite_pread@reads-uncached:
    - {shard-rkl}:        [SKIP][16] ([i915#3282]) -> [PASS][17] +3 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-2/igt@gem_partial_pwrite_pread@reads-uncached.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-5/igt@gem_partial_pwrite_pread@reads-uncached.html

  * igt@gen9_exec_parse@basic-rejected:
    - {shard-rkl}:        [SKIP][18] ([i915#2527]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-2/igt@gen9_exec_parse@basic-rejected.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-5/igt@gen9_exec_parse@basic-rejected.html

  * igt@i915_pm_rpm@modeset-lpsp:
    - {shard-dg1}:        [SKIP][20] ([i915#1397]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-dg1-18/igt@i915_pm_rpm@modeset-lpsp.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-dg1-14/igt@i915_pm_rpm@modeset-lpsp.html

  * igt@i915_pm_rpm@modeset-lpsp-stress:
    - {shard-rkl}:        [SKIP][22] ([i915#1397]) -> [PASS][23] +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-1/igt@i915_pm_rpm@modeset-lpsp-stress.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@i915_pm_rpm@modeset-lpsp-stress.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-0:
    - {shard-rkl}:        [SKIP][24] ([i915#1845] / [i915#4098]) -> [PASS][25] +29 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-5/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - {shard-tglu}:       [SKIP][26] ([i915#1845] / [i915#7651]) -> [PASS][27] +7 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-tglu-6/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-tglu-1/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size:
    - shard-glk:          [FAIL][28] ([i915#2346]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-glk2/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-glk1/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html

  * igt@kms_fbcon_fbt@psr-suspend:
    - {shard-rkl}:        [SKIP][30] ([fdo#110189] / [i915#3955]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-5/igt@kms_fbcon_fbt@psr-suspend.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@kms_fbcon_fbt@psr-suspend.html

  * igt@kms_fence_pin_leak:
    - {shard-tglu}:       [SKIP][32] ([fdo#109274] / [i915#1845]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-tglu-6/igt@kms_fence_pin_leak.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-tglu-1/igt@kms_fence_pin_leak.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - {shard-rkl}:        [SKIP][34] ([i915#1849] / [i915#4098]) -> [PASS][35] +21 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-1/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - {shard-tglu}:       [SKIP][36] ([i915#1849]) -> [PASS][37] +8 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-tglu-6/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-tglu-1/igt@kms_frontbuffer_tracking@fbc-1p-rte.html

  * igt@kms_plane@plane-position-hole@pipe-b-planes:
    - {shard-rkl}:        [SKIP][38] ([i915#1849]) -> [PASS][39] +2 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-3/igt@kms_plane@plane-position-hole@pipe-b-planes.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@kms_plane@plane-position-hole@pipe-b-planes.html

  * igt@kms_psr@cursor_mmap_cpu:
    - {shard-rkl}:        [SKIP][40] ([i915#1072]) -> [PASS][41] +2 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-3/igt@kms_psr@cursor_mmap_cpu.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-6/igt@kms_psr@cursor_mmap_cpu.html

  * igt@kms_universal_plane@universal-plane-pipe-c-functional:
    - {shard-tglu}:       [SKIP][42] ([fdo#109274]) -> [PASS][43] +1 similar issue
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-tglu-6/igt@kms_universal_plane@universal-plane-pipe-c-functional.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-tglu-1/igt@kms_universal_plane@universal-plane-pipe-c-functional.html

  * igt@kms_vblank@pipe-a-query-forked:
    - {shard-tglu}:       [SKIP][44] ([i915#7651]) -> [PASS][45] +16 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-tglu-6/igt@kms_vblank@pipe-a-query-forked.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-tglu-1/igt@kms_vblank@pipe-a-query-forked.html

  * igt@perf_pmu@idle@rcs0:
    - {shard-dg1}:        [FAIL][46] ([i915#4349]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-dg1-15/igt@perf_pmu@idle@rcs0.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-dg1-14/igt@perf_pmu@idle@rcs0.html
    - {shard-rkl}:        [FAIL][48] ([i915#4349]) -> [PASS][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12692/shard-rkl-1/igt@perf_pmu@idle@rcs0.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/shard-rkl-3/igt@perf_pmu@idle@rcs0.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1755]: https://gitlab.freedesktop.org/drm/intel/issues/1755
  [i915#1769]: https://gitlab.freedesktop.org/drm/intel/issues/1769
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#1850]: https://gitlab.freedesktop.org/drm/intel/issues/1850
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2532]: https://gitlab.freedesktop.org/drm/intel/issues/2532
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3547]: https://gitlab.freedesktop.org/drm/intel/issues/3547
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#3966]: https://gitlab.freedesktop.org/drm/intel/issues/3966
  [i915#4036]: https://gitlab.freedesktop.org/drm/intel/issues/4036
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#426]: https://gitlab.freedesktop.org/drm/intel/issues/426
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4275]: https://gitlab.freedesktop.org/drm/intel/issues/4275
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
  [i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
  [i915#6245]: https://gitlab.freedesktop.org/drm/intel/issues/6245
  [i915#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247
  [i915#6252]: https://gitlab.freedesktop.org/drm/intel/issues/6252
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7037]: https://gitlab.freedesktop.org/drm/intel/issues/7037
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
  [i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7949]: https://gitlab.freedesktop.org/drm/intel/issues/7949
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975


Build changes
-------------

  * Linux: CI_DRM_12692 -> Patchwork_113670v1

  CI-20190529: 20190529
  CI_DRM_12692: f3800283e8ba9f8e93936a76458e3afa14e38149 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7149: 1c7ea154b625e1fb826f1519b816b4256dd10b62 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113670v1: f3800283e8ba9f8e93936a76458e3afa14e38149 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113670v1/index.html

[-- Attachment #2: Type: text/html, Size: 13849 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
  2023-02-03 19:30 ` Deepak R Varma
  (?)
@ 2023-02-06  9:45   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2023-02-06  9:45 UTC (permalink / raw)
  To: Deepak R Varma, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
	Matthew Auld, Thomas Hellstrom
  Cc: Saurabh Singh Sengar, Praveen Kumar


Hi,

Adding Matt & Thomas as potential candidates to review.

Regards,

Tvrtko

On 03/02/2023 19:30, Deepak R Varma wrote:
> The macro definition of gen6_for_all_pdes() expands to a for loop such
> that it breaks when the page table is null. Hence there is no need to
> again test validity of the page table entry pointers in the pde list.
> This change is identified using itnull.cocci semantic patch.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Please note: Proposed change is compile tested only.
> 
>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index 5aaacc53fa4c..787b9e6d9f59 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
>   	u32 pde;
>   
>   	gen6_for_all_pdes(pt, pd, pde)
> -		if (pt)
> -			free_pt(&ppgtt->base.vm, pt);
> +		free_pt(&ppgtt->base.vm, pt);
>   }
>   
>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
>   
>   	/* Free all no longer used page tables */
>   	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> -		if (!pt || atomic_read(&pt->used))
> +		if (atomic_read(&pt->used))
>   			continue;
>   
>   		free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06  9:45   ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2023-02-06  9:45 UTC (permalink / raw)
  To: Deepak R Varma, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
	Matthew Auld, Thomas Hellstrom
  Cc: Praveen Kumar, Saurabh Singh Sengar


Hi,

Adding Matt & Thomas as potential candidates to review.

Regards,

Tvrtko

On 03/02/2023 19:30, Deepak R Varma wrote:
> The macro definition of gen6_for_all_pdes() expands to a for loop such
> that it breaks when the page table is null. Hence there is no need to
> again test validity of the page table entry pointers in the pde list.
> This change is identified using itnull.cocci semantic patch.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Please note: Proposed change is compile tested only.
> 
>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index 5aaacc53fa4c..787b9e6d9f59 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
>   	u32 pde;
>   
>   	gen6_for_all_pdes(pt, pd, pde)
> -		if (pt)
> -			free_pt(&ppgtt->base.vm, pt);
> +		free_pt(&ppgtt->base.vm, pt);
>   }
>   
>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
>   
>   	/* Free all no longer used page tables */
>   	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> -		if (!pt || atomic_read(&pt->used))
> +		if (atomic_read(&pt->used))
>   			continue;
>   
>   		free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06  9:45   ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2023-02-06  9:45 UTC (permalink / raw)
  To: Deepak R Varma, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
	Matthew Auld, Thomas Hellstrom
  Cc: Praveen Kumar, Saurabh Singh Sengar


Hi,

Adding Matt & Thomas as potential candidates to review.

Regards,

Tvrtko

On 03/02/2023 19:30, Deepak R Varma wrote:
> The macro definition of gen6_for_all_pdes() expands to a for loop such
> that it breaks when the page table is null. Hence there is no need to
> again test validity of the page table entry pointers in the pde list.
> This change is identified using itnull.cocci semantic patch.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Please note: Proposed change is compile tested only.
> 
>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index 5aaacc53fa4c..787b9e6d9f59 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt)
>   	u32 pde;
>   
>   	gen6_for_all_pdes(pt, pd, pde)
> -		if (pt)
> -			free_pt(&ppgtt->base.vm, pt);
> +		free_pt(&ppgtt->base.vm, pt);
>   }
>   
>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct i915_address_space *vm,
>   
>   	/* Free all no longer used page tables */
>   	gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> -		if (!pt || atomic_read(&pt->used))
> +		if (atomic_read(&pt->used))
>   			continue;
>   
>   		free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
  2023-02-06  9:45   ` Tvrtko Ursulin
  (?)
@ 2023-02-06 10:33     ` Matthew Auld
  -1 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2023-02-06 10:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, Deepak R Varma, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, David Airlie, Daniel Vetter, intel-gfx, dri-devel,
	linux-kernel, Thomas Hellstrom
  Cc: Praveen Kumar, Saurabh Singh Sengar

On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> Adding Matt & Thomas as potential candidates to review.
> 
> Regards,
> 
> Tvrtko
> 
> On 03/02/2023 19:30, Deepak R Varma wrote:
>> The macro definition of gen6_for_all_pdes() expands to a for loop such
>> that it breaks when the page table is null. Hence there is no need to
>> again test validity of the page table entry pointers in the pde list.
>> This change is identified using itnull.cocci semantic patch.
>>
>> Signed-off-by: Deepak R Varma <drv@mailo.com>
>> ---
>> Please note: Proposed change is compile tested only.
>>
>>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c 
>> b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> index 5aaacc53fa4c..787b9e6d9f59 100644
>> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt 
>> *ppgtt)
>>       u32 pde;
>>       gen6_for_all_pdes(pt, pd, pde)
>> -        if (pt)
>> -            free_pt(&ppgtt->base.vm, pt);
>> +        free_pt(&ppgtt->base.vm, pt);
>>   }
>>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct 
>> i915_address_space *vm,
>>       /* Free all no longer used page tables */
>>       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
>> -        if (!pt || atomic_read(&pt->used))
>> +        if (atomic_read(&pt->used))

Wow, I was really confused trying to remember how this all works.

The gen6_for_all_pdes() does:

(pt = i915_pt_entry(pd, iter), true)

So NULL pt is expected, and does not 'break' here, since 'true' is 
always the value that decides whether to terminate the loop. So this 
patch would lead to NULL ptr deref, AFAICT.



>>               continue;
>>           free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 10:33     ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2023-02-06 10:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, Deepak R Varma, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, David Airlie, Daniel Vetter, intel-gfx, dri-devel,
	linux-kernel, Thomas Hellstrom
  Cc: Praveen Kumar, Saurabh Singh Sengar

On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> Adding Matt & Thomas as potential candidates to review.
> 
> Regards,
> 
> Tvrtko
> 
> On 03/02/2023 19:30, Deepak R Varma wrote:
>> The macro definition of gen6_for_all_pdes() expands to a for loop such
>> that it breaks when the page table is null. Hence there is no need to
>> again test validity of the page table entry pointers in the pde list.
>> This change is identified using itnull.cocci semantic patch.
>>
>> Signed-off-by: Deepak R Varma <drv@mailo.com>
>> ---
>> Please note: Proposed change is compile tested only.
>>
>>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c 
>> b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> index 5aaacc53fa4c..787b9e6d9f59 100644
>> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt 
>> *ppgtt)
>>       u32 pde;
>>       gen6_for_all_pdes(pt, pd, pde)
>> -        if (pt)
>> -            free_pt(&ppgtt->base.vm, pt);
>> +        free_pt(&ppgtt->base.vm, pt);
>>   }
>>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct 
>> i915_address_space *vm,
>>       /* Free all no longer used page tables */
>>       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
>> -        if (!pt || atomic_read(&pt->used))
>> +        if (atomic_read(&pt->used))

Wow, I was really confused trying to remember how this all works.

The gen6_for_all_pdes() does:

(pt = i915_pt_entry(pd, iter), true)

So NULL pt is expected, and does not 'break' here, since 'true' is 
always the value that decides whether to terminate the loop. So this 
patch would lead to NULL ptr deref, AFAICT.



>>               continue;
>>           free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 10:33     ` Matthew Auld
  0 siblings, 0 replies; 17+ messages in thread
From: Matthew Auld @ 2023-02-06 10:33 UTC (permalink / raw)
  To: Tvrtko Ursulin, Deepak R Varma, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, David Airlie, Daniel Vetter, intel-gfx, dri-devel,
	linux-kernel, Thomas Hellstrom
  Cc: Saurabh Singh Sengar, Praveen Kumar

On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> Adding Matt & Thomas as potential candidates to review.
> 
> Regards,
> 
> Tvrtko
> 
> On 03/02/2023 19:30, Deepak R Varma wrote:
>> The macro definition of gen6_for_all_pdes() expands to a for loop such
>> that it breaks when the page table is null. Hence there is no need to
>> again test validity of the page table entry pointers in the pde list.
>> This change is identified using itnull.cocci semantic patch.
>>
>> Signed-off-by: Deepak R Varma <drv@mailo.com>
>> ---
>> Please note: Proposed change is compile tested only.
>>
>>   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c 
>> b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> index 5aaacc53fa4c..787b9e6d9f59 100644
>> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
>> @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt 
>> *ppgtt)
>>       u32 pde;
>>       gen6_for_all_pdes(pt, pd, pde)
>> -        if (pt)
>> -            free_pt(&ppgtt->base.vm, pt);
>> +        free_pt(&ppgtt->base.vm, pt);
>>   }
>>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>> @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct 
>> i915_address_space *vm,
>>       /* Free all no longer used page tables */
>>       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
>> -        if (!pt || atomic_read(&pt->used))
>> +        if (atomic_read(&pt->used))

Wow, I was really confused trying to remember how this all works.

The gen6_for_all_pdes() does:

(pt = i915_pt_entry(pd, iter), true)

So NULL pt is expected, and does not 'break' here, since 'true' is 
always the value that decides whether to terminate the loop. So this 
patch would lead to NULL ptr deref, AFAICT.



>>               continue;
>>           free_pt(&ppgtt->base.vm, pt);

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
  2023-02-06 10:33     ` [Intel-gfx] " Matthew Auld
  (?)
@ 2023-02-06 18:42       ` Deepak R Varma
  -1 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:42 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
	Thomas Hellstrom, Saurabh Singh Sengar, Praveen Kumar,
	Deepak R Varma

On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > 
> > Hi,
> > 
> > Adding Matt & Thomas as potential candidates to review.
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > that it breaks when the page table is null. Hence there is no need to
> > > again test validity of the page table entry pointers in the pde list.
> > > This change is identified using itnull.cocci semantic patch.
> > > 
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > > Please note: Proposed change is compile tested only.
> > > 
> > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > *ppgtt)
> > >       u32 pde;
> > >       gen6_for_all_pdes(pt, pd, pde)
> > > -        if (pt)
> > > -            free_pt(&ppgtt->base.vm, pt);
> > > +        free_pt(&ppgtt->base.vm, pt);
> > >   }
> > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > i915_address_space *vm,
> > >       /* Free all no longer used page tables */
> > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > -        if (!pt || atomic_read(&pt->used))
> > > +        if (atomic_read(&pt->used))
> 
> Wow, I was really confused trying to remember how this all works.
> 
> The gen6_for_all_pdes() does:
> 
> (pt = i915_pt_entry(pd, iter), true)
> 
> So NULL pt is expected, and does not 'break' here, since 'true' is always
> the value that decides whether to terminate the loop. So this patch would
> lead to NULL ptr deref, AFAICT.

Hello Matt,
I understand it now. I was misreading the true as part of the function argument.
Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
the same file is safe? It doesn't appear to have an check on pt validity here.

Thank you,
deepak.

> 
> 
> 
> > >               continue;
> > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 18:42       ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:42 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Saurabh Singh Sengar, Praveen Kumar, intel-gfx,
	linux-kernel, Deepak R Varma, Thomas Hellstrom, dri-devel,
	Rodrigo Vivi

On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > 
> > Hi,
> > 
> > Adding Matt & Thomas as potential candidates to review.
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > that it breaks when the page table is null. Hence there is no need to
> > > again test validity of the page table entry pointers in the pde list.
> > > This change is identified using itnull.cocci semantic patch.
> > > 
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > > Please note: Proposed change is compile tested only.
> > > 
> > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > *ppgtt)
> > >       u32 pde;
> > >       gen6_for_all_pdes(pt, pd, pde)
> > > -        if (pt)
> > > -            free_pt(&ppgtt->base.vm, pt);
> > > +        free_pt(&ppgtt->base.vm, pt);
> > >   }
> > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > i915_address_space *vm,
> > >       /* Free all no longer used page tables */
> > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > -        if (!pt || atomic_read(&pt->used))
> > > +        if (atomic_read(&pt->used))
> 
> Wow, I was really confused trying to remember how this all works.
> 
> The gen6_for_all_pdes() does:
> 
> (pt = i915_pt_entry(pd, iter), true)
> 
> So NULL pt is expected, and does not 'break' here, since 'true' is always
> the value that decides whether to terminate the loop. So this patch would
> lead to NULL ptr deref, AFAICT.

Hello Matt,
I understand it now. I was misreading the true as part of the function argument.
Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
the same file is safe? It doesn't appear to have an check on pt validity here.

Thank you,
deepak.

> 
> 
> 
> > >               continue;
> > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 18:42       ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:42 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Saurabh Singh Sengar, Praveen Kumar, intel-gfx, linux-kernel,
	Deepak R Varma, Thomas Hellstrom, dri-devel, Daniel Vetter,
	Rodrigo Vivi, David Airlie

On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > 
> > Hi,
> > 
> > Adding Matt & Thomas as potential candidates to review.
> > 
> > Regards,
> > 
> > Tvrtko
> > 
> > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > that it breaks when the page table is null. Hence there is no need to
> > > again test validity of the page table entry pointers in the pde list.
> > > This change is identified using itnull.cocci semantic patch.
> > > 
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > > Please note: Proposed change is compile tested only.
> > > 
> > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > *ppgtt)
> > >       u32 pde;
> > >       gen6_for_all_pdes(pt, pd, pde)
> > > -        if (pt)
> > > -            free_pt(&ppgtt->base.vm, pt);
> > > +        free_pt(&ppgtt->base.vm, pt);
> > >   }
> > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > i915_address_space *vm,
> > >       /* Free all no longer used page tables */
> > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > -        if (!pt || atomic_read(&pt->used))
> > > +        if (atomic_read(&pt->used))
> 
> Wow, I was really confused trying to remember how this all works.
> 
> The gen6_for_all_pdes() does:
> 
> (pt = i915_pt_entry(pd, iter), true)
> 
> So NULL pt is expected, and does not 'break' here, since 'true' is always
> the value that decides whether to terminate the loop. So this patch would
> lead to NULL ptr deref, AFAICT.

Hello Matt,
I understand it now. I was misreading the true as part of the function argument.
Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
the same file is safe? It doesn't appear to have an check on pt validity here.

Thank you,
deepak.

> 
> 
> 
> > >               continue;
> > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
  2023-02-06 18:42       ` Deepak R Varma
  (?)
@ 2023-02-06 18:53         ` Deepak R Varma
  -1 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:53 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	David Airlie, Daniel Vetter, intel-gfx, dri-devel, linux-kernel,
	Thomas Hellstrom, Saurabh Singh Sengar, Praveen Kumar

On Tue, Feb 07, 2023 at 12:12:18AM +0530, Deepak R Varma wrote:
> On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> > On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > > 
> > > Hi,
> > > 
> > > Adding Matt & Thomas as potential candidates to review.
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > > that it breaks when the page table is null. Hence there is no need to
> > > > again test validity of the page table entry pointers in the pde list.
> > > > This change is identified using itnull.cocci semantic patch.
> > > > 
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > > Please note: Proposed change is compile tested only.
> > > > 
> > > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > > *ppgtt)
> > > >       u32 pde;
> > > >       gen6_for_all_pdes(pt, pd, pde)
> > > > -        if (pt)
> > > > -            free_pt(&ppgtt->base.vm, pt);
> > > > +        free_pt(&ppgtt->base.vm, pt);
> > > >   }
> > > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > > i915_address_space *vm,
> > > >       /* Free all no longer used page tables */
> > > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > > -        if (!pt || atomic_read(&pt->used))
> > > > +        if (atomic_read(&pt->used))
> > 
> > Wow, I was really confused trying to remember how this all works.
> > 
> > The gen6_for_all_pdes() does:
> > 
> > (pt = i915_pt_entry(pd, iter), true)
> > 
> > So NULL pt is expected, and does not 'break' here, since 'true' is always
> > the value that decides whether to terminate the loop. So this patch would
> > lead to NULL ptr deref, AFAICT.
> 
> Hello Matt,
> I understand it now. I was misreading the true as part of the function argument.
> Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
> the same file is safe? It doesn't appear to have an check on pt validity here.

Please ignore the question. I understand it now. My apologies for inconvenience.
The patch is invalid and can be dropped.

deepak.

> 
> Thank you,
> deepak.
> 
> > 
> > 
> > 
> > > >               continue;
> > > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 18:53         ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:53 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Saurabh Singh Sengar, Praveen Kumar, intel-gfx,
	linux-kernel, Thomas Hellstrom, dri-devel, Rodrigo Vivi

On Tue, Feb 07, 2023 at 12:12:18AM +0530, Deepak R Varma wrote:
> On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> > On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > > 
> > > Hi,
> > > 
> > > Adding Matt & Thomas as potential candidates to review.
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > > that it breaks when the page table is null. Hence there is no need to
> > > > again test validity of the page table entry pointers in the pde list.
> > > > This change is identified using itnull.cocci semantic patch.
> > > > 
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > > Please note: Proposed change is compile tested only.
> > > > 
> > > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > > *ppgtt)
> > > >       u32 pde;
> > > >       gen6_for_all_pdes(pt, pd, pde)
> > > > -        if (pt)
> > > > -            free_pt(&ppgtt->base.vm, pt);
> > > > +        free_pt(&ppgtt->base.vm, pt);
> > > >   }
> > > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > > i915_address_space *vm,
> > > >       /* Free all no longer used page tables */
> > > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > > -        if (!pt || atomic_read(&pt->used))
> > > > +        if (atomic_read(&pt->used))
> > 
> > Wow, I was really confused trying to remember how this all works.
> > 
> > The gen6_for_all_pdes() does:
> > 
> > (pt = i915_pt_entry(pd, iter), true)
> > 
> > So NULL pt is expected, and does not 'break' here, since 'true' is always
> > the value that decides whether to terminate the loop. So this patch would
> > lead to NULL ptr deref, AFAICT.
> 
> Hello Matt,
> I understand it now. I was misreading the true as part of the function argument.
> Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
> the same file is safe? It doesn't appear to have an check on pt validity here.

Please ignore the question. I understand it now. My apologies for inconvenience.
The patch is invalid and can be dropped.

deepak.

> 
> Thank you,
> deepak.
> 
> > 
> > 
> > 
> > > >               continue;
> > > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gt: Avoid redundant pointer validity check
@ 2023-02-06 18:53         ` Deepak R Varma
  0 siblings, 0 replies; 17+ messages in thread
From: Deepak R Varma @ 2023-02-06 18:53 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Saurabh Singh Sengar, Praveen Kumar, intel-gfx, linux-kernel,
	Thomas Hellstrom, dri-devel, Daniel Vetter, Rodrigo Vivi,
	David Airlie

On Tue, Feb 07, 2023 at 12:12:18AM +0530, Deepak R Varma wrote:
> On Mon, Feb 06, 2023 at 10:33:13AM +0000, Matthew Auld wrote:
> > On 06/02/2023 09:45, Tvrtko Ursulin wrote:
> > > 
> > > Hi,
> > > 
> > > Adding Matt & Thomas as potential candidates to review.
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > On 03/02/2023 19:30, Deepak R Varma wrote:
> > > > The macro definition of gen6_for_all_pdes() expands to a for loop such
> > > > that it breaks when the page table is null. Hence there is no need to
> > > > again test validity of the page table entry pointers in the pde list.
> > > > This change is identified using itnull.cocci semantic patch.
> > > > 
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > > Please note: Proposed change is compile tested only.
> > > > 
> > > >   drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++---
> > > >   1 file changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > index 5aaacc53fa4c..787b9e6d9f59 100644
> > > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> > > > @@ -258,8 +258,7 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt
> > > > *ppgtt)
> > > >       u32 pde;
> > > >       gen6_for_all_pdes(pt, pd, pde)
> > > > -        if (pt)
> > > > -            free_pt(&ppgtt->base.vm, pt);
> > > > +        free_pt(&ppgtt->base.vm, pt);
> > > >   }
> > > >   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> > > > @@ -304,7 +303,7 @@ static void pd_vma_unbind(struct
> > > > i915_address_space *vm,
> > > >       /* Free all no longer used page tables */
> > > >       gen6_for_all_pdes(pt, ppgtt->base.pd, pde) {
> > > > -        if (!pt || atomic_read(&pt->used))
> > > > +        if (atomic_read(&pt->used))
> > 
> > Wow, I was really confused trying to remember how this all works.
> > 
> > The gen6_for_all_pdes() does:
> > 
> > (pt = i915_pt_entry(pd, iter), true)
> > 
> > So NULL pt is expected, and does not 'break' here, since 'true' is always
> > the value that decides whether to terminate the loop. So this patch would
> > lead to NULL ptr deref, AFAICT.
> 
> Hello Matt,
> I understand it now. I was misreading the true as part of the function argument.
> Could you please also comment if the implementation of gen6_ppgtt_free_pd() in
> the same file is safe? It doesn't appear to have an check on pt validity here.

Please ignore the question. I understand it now. My apologies for inconvenience.
The patch is invalid and can be dropped.

deepak.

> 
> Thank you,
> deepak.
> 
> > 
> > 
> > 
> > > >               continue;
> > > >           free_pt(&ppgtt->base.vm, pt);



^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-02-06 18:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-03 19:30 [PATCH] drm/i915/gt: Avoid redundant pointer validity check Deepak R Varma
2023-02-03 19:30 ` [Intel-gfx] " Deepak R Varma
2023-02-03 19:30 ` Deepak R Varma
2023-02-03 20:33 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-02-05  8:18 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-02-06  9:45 ` [PATCH] " Tvrtko Ursulin
2023-02-06  9:45   ` [Intel-gfx] " Tvrtko Ursulin
2023-02-06  9:45   ` Tvrtko Ursulin
2023-02-06 10:33   ` Matthew Auld
2023-02-06 10:33     ` Matthew Auld
2023-02-06 10:33     ` [Intel-gfx] " Matthew Auld
2023-02-06 18:42     ` Deepak R Varma
2023-02-06 18:42       ` [Intel-gfx] " Deepak R Varma
2023-02-06 18:42       ` Deepak R Varma
2023-02-06 18:53       ` Deepak R Varma
2023-02-06 18:53         ` [Intel-gfx] " Deepak R Varma
2023-02-06 18:53         ` Deepak R Varma

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.