[PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andrzej Hajda]

The pattern of setting variable with new value and returning old
one is very common in kernel. Usually atomicity of the operation
is not required, so xchg seems to be suboptimal and confusing in
such cases. Since name xchg is already in use and __xchg is used
in architecture code, proposition is to name the macro exchange.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
Hi,

I hope there will be place for such tiny helper in kernel.
Quick cocci analyze shows there is probably few thousands places
where it could be used, of course I do not intend to do it :).

I was not sure where to put this macro, I hope near swap definition
is the most suitable place.

Moreover sorry if to/cc is not correct - get_maintainers.pl was
more confused than me, to who address this patch.

Regards
Andrzej
---
 include/linux/minmax.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 5433c08fcc6858..17d48769203bd5 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -144,4 +144,18 @@
 #define swap(a, b) \
 	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
 
+/**
+ * exchange - set variable pointed by @ptr to @val, return old value
+ * @ptr: pointer to affected variable
+ * @val: value to be written
+ *
+ * This is non-atomic variant of xchg.
+ */
+#define exchange(ptr, val) ({		\
+	typeof(ptr) __ptr = ptr;	\
+	typeof(*__ptr) __t = *__ptr;	\
+	*(__ptr) = (val);		\
+	__t;				\
+})
+
 #endif	/* _LINUX_MINMAX_H */
-- 
2.34.1

[PATCH 2/5] drm/i915/display: kill fetch_and_zero usage

[Andrzej Hajda]

Better use recently introduced kernel core helper.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/display/icl_dsi.c        |  2 +-
 drivers/gpu/drm/i915/display/intel_ddi.c      |  6 ++---
 drivers/gpu/drm/i915/display/intel_display.c  |  4 ++--
 .../drm/i915/display/intel_display_power.c    | 22 +++++++++----------
 drivers/gpu/drm/i915/display/intel_dmc.c      |  2 +-
 drivers/gpu/drm/i915/display/intel_fb_pin.c   |  6 ++---
 drivers/gpu/drm/i915/display/intel_fbdev.c    |  3 ++-
 drivers/gpu/drm/i915/display/intel_overlay.c  |  4 ++--
 drivers/gpu/drm/i915/display/intel_pps.c      |  4 ++--
 drivers/gpu/drm/i915/display/intel_tc.c       |  4 ++--
 10 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index d16b30a2dded33..629b51ef7bfcce 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -1425,7 +1425,7 @@ static void gen11_dsi_disable_io_power(struct intel_encoder *encoder)
 	for_each_dsi_port(port, intel_dsi->ports) {
 		intel_wakeref_t wakeref;
 
-		wakeref = fetch_and_zero(&intel_dsi->io_wakeref[port]);
+		wakeref = exchange(&intel_dsi->io_wakeref[port], 0);
 		intel_display_power_put(dev_priv,
 					port == PORT_A ?
 					POWER_DOMAIN_PORT_DDI_IO_A :
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 5f9a2410fc4c35..9486768fb9d38e 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -902,7 +902,7 @@ main_link_aux_power_domain_put(struct intel_digital_port *dig_port,
 		intel_ddi_main_link_aux_domain(dig_port, crtc_state);
 	intel_wakeref_t wf;
 
-	wf = fetch_and_zero(&dig_port->aux_wakeref);
+	wf = exchange(&dig_port->aux_wakeref, 0);
 	if (!wf)
 		return;
 
@@ -2678,7 +2678,7 @@ static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
 	if (!intel_tc_port_in_tbt_alt_mode(dig_port))
 		intel_display_power_put(dev_priv,
 					dig_port->ddi_io_power_domain,
-					fetch_and_zero(&dig_port->ddi_io_wakeref));
+					exchange(&dig_port->ddi_io_wakeref, 0));
 
 	intel_ddi_disable_clock(encoder);
 }
@@ -2705,7 +2705,7 @@ static void intel_ddi_post_disable_hdmi(struct intel_atomic_state *state,
 
 	intel_display_power_put(dev_priv,
 				dig_port->ddi_io_power_domain,
-				fetch_and_zero(&dig_port->ddi_io_wakeref));
+				exchange(&dig_port->ddi_io_wakeref, 0));
 
 	intel_ddi_disable_clock(encoder);
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 32b25715718644..fd9f7ab71ee84c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -964,7 +964,7 @@ void intel_display_finish_reset(struct drm_i915_private *i915)
 	if (!test_bit(I915_RESET_MODESET, &to_gt(i915)->reset.flags))
 		return;
 
-	state = fetch_and_zero(&i915->display.restore.modeset_state);
+	state = exchange(&i915->display.restore.modeset_state, NULL);
 	if (!state)
 		goto unlock;
 
@@ -7591,7 +7591,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		 * cleanup. So copy and reset the dsb structure to sync with
 		 * commit_done and later do dsb cleanup in cleanup_work.
 		 */
-		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
+		old_crtc_state->dsb = exchange(&new_crtc_state->dsb, 0);
 	}
 
 	/* Underruns don't always raise interrupts, so check manually */
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 3adba64937de68..34a155bf825c87 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -474,7 +474,7 @@ intel_display_power_grab_async_put_ref(struct drm_i915_private *dev_priv,
 
 	cancel_delayed_work(&power_domains->async_put_work);
 	intel_runtime_pm_put_raw(&dev_priv->runtime_pm,
-				 fetch_and_zero(&power_domains->async_put_wakeref));
+				 exchange(&power_domains->async_put_wakeref, 0));
 out_verify:
 	verify_async_put_domains_state(power_domains);
 
@@ -660,7 +660,7 @@ intel_display_power_put_async_work(struct work_struct *work)
 	 * Bail out if all the domain refs pending to be released were grabbed
 	 * by subsequent gets or a flush_work.
 	 */
-	old_work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+	old_work_wakeref = exchange(&power_domains->async_put_wakeref, 0);
 	if (!old_work_wakeref)
 		goto out_verify;
 
@@ -675,7 +675,7 @@ intel_display_power_put_async_work(struct work_struct *work)
 		bitmap_zero(power_domains->async_put_domains[1].bits,
 			    POWER_DOMAIN_NUM);
 		queue_async_put_domains_work(power_domains,
-					     fetch_and_zero(&new_work_wakeref));
+					     exchange(&new_work_wakeref, 0));
 	} else {
 		/*
 		 * Cancel the work that got queued after this one got dequeued,
@@ -729,7 +729,7 @@ void __intel_display_power_put_async(struct drm_i915_private *i915,
 	} else {
 		set_bit(domain, power_domains->async_put_domains[0].bits);
 		queue_async_put_domains_work(power_domains,
-					     fetch_and_zero(&work_wakeref));
+					     exchange(&work_wakeref, 0));
 	}
 
 out_verify:
@@ -763,7 +763,7 @@ void intel_display_power_flush_work(struct drm_i915_private *i915)
 
 	mutex_lock(&power_domains->lock);
 
-	work_wakeref = fetch_and_zero(&power_domains->async_put_wakeref);
+	work_wakeref = exchange(&power_domains->async_put_wakeref, 0);
 	if (!work_wakeref)
 		goto out_verify;
 
@@ -891,7 +891,7 @@ intel_display_power_put_mask_in_set(struct drm_i915_private *i915,
 		intel_wakeref_t __maybe_unused wf = -1;
 
 #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
-		wf = fetch_and_zero(&power_domain_set->wakerefs[domain]);
+		wf = exchange(&power_domain_set->wakerefs[domain], 0);
 #endif
 		intel_display_power_put(i915, domain, wf);
 		clear_bit(domain, power_domain_set->mask.bits);
@@ -1943,12 +1943,12 @@ void intel_power_domains_init_hw(struct drm_i915_private *i915, bool resume)
 void intel_power_domains_driver_remove(struct drm_i915_private *i915)
 {
 	intel_wakeref_t wakeref __maybe_unused =
-		fetch_and_zero(&i915->display.power.domains.init_wakeref);
+		exchange(&i915->display.power.domains.init_wakeref, 0);
 
 	/* Remove the refcount we took to keep power well support disabled. */
 	if (!i915->params.disable_power_well)
 		intel_display_power_put(i915, POWER_DOMAIN_INIT,
-					fetch_and_zero(&i915->display.power.domains.disable_wakeref));
+					exchange(&i915->display.power.domains.disable_wakeref, 0));
 
 	intel_display_power_flush_work_sync(i915);
 
@@ -2004,7 +2004,7 @@ void intel_power_domains_sanitize_state(struct drm_i915_private *i915)
 void intel_power_domains_enable(struct drm_i915_private *i915)
 {
 	intel_wakeref_t wakeref __maybe_unused =
-		fetch_and_zero(&i915->display.power.domains.init_wakeref);
+		exchange(&i915->display.power.domains.init_wakeref, 0);
 
 	intel_display_power_put(i915, POWER_DOMAIN_INIT, wakeref);
 	intel_power_domains_verify_state(i915);
@@ -2044,7 +2044,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
 {
 	struct i915_power_domains *power_domains = &i915->display.power.domains;
 	intel_wakeref_t wakeref __maybe_unused =
-		fetch_and_zero(&power_domains->init_wakeref);
+		exchange(&power_domains->init_wakeref, 0);
 
 	intel_display_power_put(i915, POWER_DOMAIN_INIT, wakeref);
 
@@ -2069,7 +2069,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
 	 */
 	if (!i915->params.disable_power_well)
 		intel_display_power_put(i915, POWER_DOMAIN_INIT,
-					fetch_and_zero(&i915->display.power.domains.disable_wakeref));
+					exchange(&i915->display.power.domains.disable_wakeref, 0));
 
 	intel_display_power_flush_work(i915);
 	intel_power_domains_verify_state(i915);
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index eff3add706117c..17399955024bd0 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -838,7 +838,7 @@ static void intel_dmc_runtime_pm_get(struct drm_i915_private *dev_priv)
 static void intel_dmc_runtime_pm_put(struct drm_i915_private *dev_priv)
 {
 	intel_wakeref_t wakeref __maybe_unused =
-		fetch_and_zero(&dev_priv->display.dmc.wakeref);
+		exchange(&dev_priv->display.dmc.wakeref, 0);
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT, wakeref);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index 1aca7552a85d03..70661b40f0f979 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -285,17 +285,17 @@ void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state)
 	struct i915_vma *vma;
 
 	if (!intel_fb_uses_dpt(fb)) {
-		vma = fetch_and_zero(&old_plane_state->ggtt_vma);
+		vma = exchange(&old_plane_state->ggtt_vma, NULL);
 		if (vma)
 			intel_unpin_fb_vma(vma, old_plane_state->flags);
 	} else {
 		struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 
-		vma = fetch_and_zero(&old_plane_state->dpt_vma);
+		vma = exchange(&old_plane_state->dpt_vma, NULL);
 		if (vma)
 			intel_unpin_fb_vma(vma, old_plane_state->flags);
 
-		vma = fetch_and_zero(&old_plane_state->ggtt_vma);
+		vma = exchange(&old_plane_state->ggtt_vma, NULL);
 		if (vma)
 			intel_dpt_unpin(intel_fb->dpt_vm);
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 03ed4607a46d21..d59b4cc6b36f33 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -591,7 +591,8 @@ void intel_fbdev_unregister(struct drm_i915_private *dev_priv)
 
 void intel_fbdev_fini(struct drm_i915_private *dev_priv)
 {
-	struct intel_fbdev *ifbdev = fetch_and_zero(&dev_priv->display.fbdev.fbdev);
+	struct intel_fbdev *ifbdev = exchange(&dev_priv->display.fbdev.fbdev,
+					      NULL);
 
 	if (!ifbdev)
 		return;
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
index c12bdca8da9ba6..89b39b933be7e3 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -355,7 +355,7 @@ static void intel_overlay_release_old_vma(struct intel_overlay *overlay)
 {
 	struct i915_vma *vma;
 
-	vma = fetch_and_zero(&overlay->old_vma);
+	vma = exchange(&overlay->old_vma, NULL);
 	if (drm_WARN_ON(&overlay->i915->drm, !vma))
 		return;
 
@@ -1428,7 +1428,7 @@ void intel_overlay_cleanup(struct drm_i915_private *dev_priv)
 {
 	struct intel_overlay *overlay;
 
-	overlay = fetch_and_zero(&dev_priv->display.overlay);
+	overlay = exchange(&dev_priv->display.overlay, NULL);
 	if (!overlay)
 		return;
 
diff --git a/drivers/gpu/drm/i915/display/intel_pps.c b/drivers/gpu/drm/i915/display/intel_pps.c
index 9bbf41a076f728..b28a6f955a57e8 100644
--- a/drivers/gpu/drm/i915/display/intel_pps.c
+++ b/drivers/gpu/drm/i915/display/intel_pps.c
@@ -690,7 +690,7 @@ static void intel_pps_vdd_off_sync_unlocked(struct intel_dp *intel_dp)
 
 	intel_display_power_put(dev_priv,
 				intel_aux_power_domain(dig_port),
-				fetch_and_zero(&intel_dp->pps.vdd_wakeref));
+				exchange(&intel_dp->pps.vdd_wakeref, 0));
 }
 
 void intel_pps_vdd_off_sync(struct intel_dp *intel_dp)
@@ -866,7 +866,7 @@ void intel_pps_off_unlocked(struct intel_dp *intel_dp)
 	/* We got a reference when we enabled the VDD. */
 	intel_display_power_put(dev_priv,
 				intel_aux_power_domain(dig_port),
-				fetch_and_zero(&intel_dp->pps.vdd_wakeref));
+				exchange(&intel_dp->pps.vdd_wakeref, 0));
 }
 
 void intel_pps_off(struct intel_dp *intel_dp)
diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
index 70624b4b2d38c1..7701daef66ff5c 100644
--- a/drivers/gpu/drm/i915/display/intel_tc.c
+++ b/drivers/gpu/drm/i915/display/intel_tc.c
@@ -671,7 +671,7 @@ static void intel_tc_port_update_mode(struct intel_digital_port *dig_port,
 
 	/* Get power domain matching the new mode after reset. */
 	tc_cold_unblock(dig_port, dig_port->tc_lock_power_domain,
-			fetch_and_zero(&dig_port->tc_lock_wakeref));
+			exchange(&dig_port->tc_lock_wakeref, 0));
 	if (dig_port->tc_mode != TC_PORT_DISCONNECTED)
 		dig_port->tc_lock_wakeref = tc_cold_block(dig_port,
 							  &dig_port->tc_lock_power_domain);
@@ -767,7 +767,7 @@ void intel_tc_port_sanitize_mode(struct intel_digital_port *dig_port)
 		icl_tc_phy_disconnect(dig_port);
 
 		tc_cold_unblock(dig_port, dig_port->tc_lock_power_domain,
-				fetch_and_zero(&dig_port->tc_lock_wakeref));
+				exchange(&dig_port->tc_lock_wakeref, 0));
 	}
 
 	drm_dbg_kms(&i915->drm, "Port %s: sanitize mode (%s)\n",
-- 
2.34.1

[PATCH 3/5] drm/i915/gt: kill fetch_and_zero usage

[Andrzej Hajda]

Better use recently introduced kernel core helper.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c            | 2 +-
 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c     | 4 ++--
 drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++--
 drivers/gpu/drm/i915/gt/intel_ggtt.c                 | 4 ++--
 drivers/gpu/drm/i915/gt/intel_gsc.c                  | 2 +-
 drivers/gpu/drm/i915/gt/intel_gt.c                   | 4 ++--
 drivers/gpu/drm/i915/gt/intel_gt_pm.c                | 2 +-
 drivers/gpu/drm/i915/gt/intel_lrc.c                  | 6 +++---
 drivers/gpu/drm/i915/gt/intel_migrate.c              | 2 +-
 drivers/gpu/drm/i915/gt/intel_rc6.c                  | 2 +-
 drivers/gpu/drm/i915/gt/intel_rps.c                  | 2 +-
 drivers/gpu/drm/i915/gt/selftest_context.c           | 2 +-
 drivers/gpu/drm/i915/gt/selftest_ring_submission.c   | 2 +-
 drivers/gpu/drm/i915/gt/selftest_timeline.c          | 2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c                | 2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             | 2 +-
 16 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c33e0d72d6702b..de318d96d52abd 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1024,7 +1024,7 @@ static void cleanup_status_page(struct intel_engine_cs *engine)
 	/* Prevent writes into HWSP after returning the page to the system */
 	intel_engine_set_hwsp_writemask(engine, ~0u);
 
-	vma = fetch_and_zero(&engine->status_page.vma);
+	vma = exchange(&engine->status_page.vma, NULL);
 	if (!vma)
 		return;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index 9a527e1f5be655..6029fafaaa674f 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -229,7 +229,7 @@ static void heartbeat(struct work_struct *wrk)
 	mutex_unlock(&ce->timeline->mutex);
 out:
 	if (!engine->i915->params.enable_hangcheck || !next_heartbeat(engine))
-		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
+		i915_request_put(exchange(&engine->heartbeat.systole, 0));
 	intel_engine_pm_put(engine);
 }
 
@@ -244,7 +244,7 @@ void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
 void intel_engine_park_heartbeat(struct intel_engine_cs *engine)
 {
 	if (cancel_delayed_work(&engine->heartbeat.work))
-		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
+		i915_request_put(exchange(&engine->heartbeat.systole, 0));
 }
 
 void intel_gt_unpark_heartbeats(struct intel_gt *gt)
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 49a8f10d76c77b..29e78078d55a8b 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -3197,7 +3197,7 @@ static void execlists_reset_cancel(struct intel_engine_cs *engine)
 		RB_CLEAR_NODE(rb);
 
 		spin_lock(&ve->base.sched_engine->lock);
-		rq = fetch_and_zero(&ve->request);
+		rq = exchange(&ve->request, NULL);
 		if (rq) {
 			if (i915_request_mark_eio(rq)) {
 				rq->engine = engine;
@@ -3602,7 +3602,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
 
 		spin_lock_irq(&ve->base.sched_engine->lock);
 
-		old = fetch_and_zero(&ve->request);
+		old = exchange(&ve->request, NULL);
 		if (old) {
 			GEM_BUG_ON(!__i915_request_is_complete(old));
 			__i915_request_submit(old);
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 0c7fe360f87331..2eb0173c6e968c 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -684,7 +684,7 @@ static void fini_aliasing_ppgtt(struct i915_ggtt *ggtt)
 {
 	struct i915_ppgtt *ppgtt;
 
-	ppgtt = fetch_and_zero(&ggtt->alias);
+	ppgtt = exchange(&ggtt->alias, NULL);
 	if (!ppgtt)
 		return;
 
@@ -1238,7 +1238,7 @@ bool i915_ggtt_resume_vm(struct i915_address_space *vm)
 				   was_bound);
 
 		if (obj) { /* only used during resume => exclusive access */
-			write_domain_objs |= fetch_and_zero(&obj->write_domain);
+			write_domain_objs |= exchange(&obj->write_domain, 0);
 			obj->read_domains |= I915_GEM_DOMAIN_GTT;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index bcc3605158dbde..7226b42bb70b2a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -70,7 +70,7 @@ gsc_ext_om_alloc(struct intel_gsc *gsc, struct intel_gsc_intf *intf, size_t size
 
 static void gsc_ext_om_destroy(struct intel_gsc_intf *intf)
 {
-	struct drm_i915_gem_object *obj = fetch_and_zero(&intf->gem_obj);
+	struct drm_i915_gem_object *obj = exchange(&intf->gem_obj, NULL);
 
 	if (!obj)
 		return;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 4e7af9bc73ad05..a277bd47db813e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -757,7 +757,7 @@ int intel_gt_init(struct intel_gt *gt)
 	intel_uc_fini(&gt->uc);
 err_engines:
 	intel_engines_release(gt);
-	i915_vm_put(fetch_and_zero(&gt->vm));
+	i915_vm_put(exchange(&gt->vm, 0));
 err_pm:
 	intel_gt_pm_fini(gt);
 	intel_gt_fini_scratch(gt);
@@ -806,7 +806,7 @@ void intel_gt_driver_release(struct intel_gt *gt)
 {
 	struct i915_address_space *vm;
 
-	vm = fetch_and_zero(&gt->vm);
+	vm = exchange(&gt->vm, NULL);
 	if (vm) /* FIXME being called twice on error paths :( */
 		i915_vm_put(vm);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 16db85fab0b19b..f066936994a9e2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -123,7 +123,7 @@ static int __gt_unpark(struct intel_wakeref *wf)
 static int __gt_park(struct intel_wakeref *wf)
 {
 	struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
-	intel_wakeref_t wakeref = fetch_and_zero(&gt->awake);
+	intel_wakeref_t wakeref = exchange(&gt->awake, 0);
 	struct drm_i915_private *i915 = gt->i915;
 
 	GT_TRACE(gt, "\n");
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 7771a19008c604..9a2bfb6d14196c 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1144,7 +1144,7 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)
 static struct intel_timeline *
 pinned_timeline(struct intel_context *ce, struct intel_engine_cs *engine)
 {
-	struct intel_timeline *tl = fetch_and_zero(&ce->timeline);
+	struct intel_timeline *tl = exchange(&ce->timeline, NULL);
 
 	return intel_timeline_create_from_engine(engine, page_unmask_bits(tl));
 }
@@ -1261,8 +1261,8 @@ void lrc_fini(struct intel_context *ce)
 	if (!ce->state)
 		return;
 
-	intel_ring_put(fetch_and_zero(&ce->ring));
-	i915_vma_put(fetch_and_zero(&ce->state));
+	intel_ring_put(exchange(&ce->ring, 0));
+	i915_vma_put(exchange(&ce->state, 0));
 }
 
 void lrc_destroy(struct kref *kref)
diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c b/drivers/gpu/drm/i915/gt/intel_migrate.c
index b405a04135ca21..2c076a51b66b30 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -1116,7 +1116,7 @@ void intel_migrate_fini(struct intel_migrate *m)
 {
 	struct intel_context *ce;
 
-	ce = fetch_and_zero(&m->context);
+	ce = exchange(&m->context, NULL);
 	if (!ce)
 		return;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
index 2ee4051e4d9613..2451ebddb0f982 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -702,7 +702,7 @@ void intel_rc6_fini(struct intel_rc6 *rc6)
 
 	intel_rc6_disable(rc6);
 
-	pctx = fetch_and_zero(&rc6->pctx);
+	pctx = exchange(&rc6->pctx, NULL);
 	if (pctx)
 		i915_gem_object_put(pctx);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 9ad3bc7201cbaa..a102d8768e1d7b 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -1831,7 +1831,7 @@ static void rps_work(struct work_struct *work)
 	u32 pm_iir = 0;
 
 	spin_lock_irq(gt->irq_lock);
-	pm_iir = fetch_and_zero(&rps->pm_iir) & rps->pm_events;
+	pm_iir = exchange(&rps->pm_iir, 0) & rps->pm_events;
 	client_boost = atomic_read(&rps->num_waiters);
 	spin_unlock_irq(gt->irq_lock);
 
diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
index 76fbae358072df..ca0a38de696eec 100644
--- a/drivers/gpu/drm/i915/gt/selftest_context.c
+++ b/drivers/gpu/drm/i915/gt/selftest_context.c
@@ -171,7 +171,7 @@ static int live_context_size(void *arg)
 		 * active state is sufficient, we are only checking that we
 		 * don't use more than we planned.
 		 */
-		saved = fetch_and_zero(&engine->default_state);
+		saved = exchange(&engine->default_state, NULL);
 
 		/* Overlaps with the execlists redzone */
 		engine->context_size += I915_GTT_PAGE_SIZE;
diff --git a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
index 87ceb0f374b673..9e901f1d5d76a9 100644
--- a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
@@ -269,7 +269,7 @@ static int live_ctx_switch_wa(void *arg)
 		if (IS_GRAPHICS_VER(gt->i915, 4, 5))
 			continue; /* MI_STORE_DWORD is privileged! */
 
-		saved_wa = fetch_and_zero(&engine->wa_ctx.vma);
+		saved_wa = exchange(&engine->wa_ctx.vma, NULL);
 
 		intel_engine_pm_get(engine);
 		err = __live_ctx_switch_wa(engine);
diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c
index 522d0190509ccc..d74b13b1b38a6e 100644
--- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
+++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
@@ -892,7 +892,7 @@ static int create_watcher(struct hwsp_watcher *w,
 static int check_watcher(struct hwsp_watcher *w, const char *name,
 			 bool (*op)(u32 hwsp, u32 seqno))
 {
-	struct i915_request *rq = fetch_and_zero(&w->rq);
+	struct i915_request *rq = exchange(&w->rq, NULL);
 	u32 offset, end;
 	int err;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4f4b519e12c1b7..0085b1727dd47a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -166,7 +166,7 @@ static void __uc_capture_load_err_log(struct intel_uc *uc)
 
 static void __uc_free_load_err_log(struct intel_uc *uc)
 {
-	struct drm_i915_gem_object *log = fetch_and_zero(&uc->load_err_log);
+	struct drm_i915_gem_object *log = exchange(&uc->load_err_log, NULL);
 
 	if (log)
 		i915_gem_object_put(log);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 6c83a8b66c9e32..44ff6da26bd698 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -1055,7 +1055,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw)
 	if (!intel_uc_fw_is_available(uc_fw))
 		return;
 
-	i915_gem_object_put(fetch_and_zero(&uc_fw->obj));
+	i915_gem_object_put(exchange(&uc_fw->obj, 0));
 
 	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_SELECTED);
 }
-- 
2.34.1

RE: [Intel-gfx] [PATCH 3/5] drm/i915/gt: kill fetch_and_zero usage

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Andrzej Hajda
> Sent: Friday, December 9, 2022 9:19 PM
> To: linux-kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Cc: Hajda, Andrzej <andrzej.hajda@intel.com>; Arnd Bergmann
> <arnd@arndb.de>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Andrew Morton
> <akpm@linux-foundation.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>
> Subject: [Intel-gfx] [PATCH 3/5] drm/i915/gt: kill fetch_and_zero usage
> 
> Better use recently introduced kernel core helper.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c            | 2 +-
>  drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c     | 4 ++--
>  drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++--
>  drivers/gpu/drm/i915/gt/intel_ggtt.c                 | 4 ++--
>  drivers/gpu/drm/i915/gt/intel_gsc.c                  | 2 +-
>  drivers/gpu/drm/i915/gt/intel_gt.c                   | 4 ++--
>  drivers/gpu/drm/i915/gt/intel_gt_pm.c                | 2 +-
>  drivers/gpu/drm/i915/gt/intel_lrc.c                  | 6 +++---
>  drivers/gpu/drm/i915/gt/intel_migrate.c              | 2 +-
>  drivers/gpu/drm/i915/gt/intel_rc6.c                  | 2 +-
>  drivers/gpu/drm/i915/gt/intel_rps.c                  | 2 +-
>  drivers/gpu/drm/i915/gt/selftest_context.c           | 2 +-
>  drivers/gpu/drm/i915/gt/selftest_ring_submission.c   | 2 +-
>  drivers/gpu/drm/i915/gt/selftest_timeline.c          | 2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c                | 2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             | 2 +-
>  16 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index c33e0d72d6702b..de318d96d52abd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1024,7 +1024,7 @@ static void cleanup_status_page(struct
> intel_engine_cs *engine)
>  	/* Prevent writes into HWSP after returning the page to the system */
>  	intel_engine_set_hwsp_writemask(engine, ~0u);
> 
> -	vma = fetch_and_zero(&engine->status_page.vma);
> +	vma = exchange(&engine->status_page.vma, NULL);
>  	if (!vma)
>  		return;
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index 9a527e1f5be655..6029fafaaa674f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -229,7 +229,7 @@ static void heartbeat(struct work_struct *wrk)
>  	mutex_unlock(&ce->timeline->mutex);
>  out:
>  	if (!engine->i915->params.enable_hangcheck ||
> !next_heartbeat(engine))
> -		i915_request_put(fetch_and_zero(&engine-
> >heartbeat.systole));
> +		i915_request_put(exchange(&engine->heartbeat.systole, 0));
>  	intel_engine_pm_put(engine);
>  }
> 
> @@ -244,7 +244,7 @@ void intel_engine_unpark_heartbeat(struct
> intel_engine_cs *engine)  void intel_engine_park_heartbeat(struct
> intel_engine_cs *engine)  {
>  	if (cancel_delayed_work(&engine->heartbeat.work))
> -		i915_request_put(fetch_and_zero(&engine-
> >heartbeat.systole));
> +		i915_request_put(exchange(&engine->heartbeat.systole, 0));
>  }
> 
>  void intel_gt_unpark_heartbeats(struct intel_gt *gt) diff --git
> a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 49a8f10d76c77b..29e78078d55a8b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3197,7 +3197,7 @@ static void execlists_reset_cancel(struct
> intel_engine_cs *engine)
>  		RB_CLEAR_NODE(rb);
> 
>  		spin_lock(&ve->base.sched_engine->lock);
> -		rq = fetch_and_zero(&ve->request);
> +		rq = exchange(&ve->request, NULL);
>  		if (rq) {
>  			if (i915_request_mark_eio(rq)) {
>  				rq->engine = engine;
> @@ -3602,7 +3602,7 @@ static void rcu_virtual_context_destroy(struct
> work_struct *wrk)
> 
>  		spin_lock_irq(&ve->base.sched_engine->lock);
> 
> -		old = fetch_and_zero(&ve->request);
> +		old = exchange(&ve->request, NULL);
>  		if (old) {
>  			GEM_BUG_ON(!__i915_request_is_complete(old));
>  			__i915_request_submit(old);
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 0c7fe360f87331..2eb0173c6e968c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -684,7 +684,7 @@ static void fini_aliasing_ppgtt(struct i915_ggtt *ggtt)  {
>  	struct i915_ppgtt *ppgtt;
> 
> -	ppgtt = fetch_and_zero(&ggtt->alias);
> +	ppgtt = exchange(&ggtt->alias, NULL);
>  	if (!ppgtt)
>  		return;
> 
> @@ -1238,7 +1238,7 @@ bool i915_ggtt_resume_vm(struct
> i915_address_space *vm)
>  				   was_bound);
> 
>  		if (obj) { /* only used during resume => exclusive access */
> -			write_domain_objs |= fetch_and_zero(&obj-
> >write_domain);
> +			write_domain_objs |= exchange(&obj-
> >write_domain, 0);
>  			obj->read_domains |= I915_GEM_DOMAIN_GTT;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c
> b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index bcc3605158dbde..7226b42bb70b2a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -70,7 +70,7 @@ gsc_ext_om_alloc(struct intel_gsc *gsc, struct
> intel_gsc_intf *intf, size_t size
> 
>  static void gsc_ext_om_destroy(struct intel_gsc_intf *intf)  {
> -	struct drm_i915_gem_object *obj = fetch_and_zero(&intf->gem_obj);
> +	struct drm_i915_gem_object *obj = exchange(&intf->gem_obj, NULL);
> 
>  	if (!obj)
>  		return;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 4e7af9bc73ad05..a277bd47db813e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -757,7 +757,7 @@ int intel_gt_init(struct intel_gt *gt)
>  	intel_uc_fini(&gt->uc);
>  err_engines:
>  	intel_engines_release(gt);
> -	i915_vm_put(fetch_and_zero(&gt->vm));
> +	i915_vm_put(exchange(&gt->vm, 0));
>  err_pm:
>  	intel_gt_pm_fini(gt);
>  	intel_gt_fini_scratch(gt);
> @@ -806,7 +806,7 @@ void intel_gt_driver_release(struct intel_gt *gt)  {
>  	struct i915_address_space *vm;
> 
> -	vm = fetch_and_zero(&gt->vm);
> +	vm = exchange(&gt->vm, NULL);
>  	if (vm) /* FIXME being called twice on error paths :( */
>  		i915_vm_put(vm);
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index 16db85fab0b19b..f066936994a9e2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -123,7 +123,7 @@ static int __gt_unpark(struct intel_wakeref *wf)
> static int __gt_park(struct intel_wakeref *wf)  {
>  	struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
> -	intel_wakeref_t wakeref = fetch_and_zero(&gt->awake);
> +	intel_wakeref_t wakeref = exchange(&gt->awake, 0);
>  	struct drm_i915_private *i915 = gt->i915;
> 
>  	GT_TRACE(gt, "\n");
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 7771a19008c604..9a2bfb6d14196c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1144,7 +1144,7 @@ __lrc_alloc_state(struct intel_context *ce, struct
> intel_engine_cs *engine)  static struct intel_timeline *  pinned_timeline(struct
> intel_context *ce, struct intel_engine_cs *engine)  {
> -	struct intel_timeline *tl = fetch_and_zero(&ce->timeline);
> +	struct intel_timeline *tl = exchange(&ce->timeline, NULL);
> 
>  	return intel_timeline_create_from_engine(engine,
> page_unmask_bits(tl));  } @@ -1261,8 +1261,8 @@ void lrc_fini(struct
> intel_context *ce)
>  	if (!ce->state)
>  		return;
> 
> -	intel_ring_put(fetch_and_zero(&ce->ring));
> -	i915_vma_put(fetch_and_zero(&ce->state));
> +	intel_ring_put(exchange(&ce->ring, 0));
> +	i915_vma_put(exchange(&ce->state, 0));
>  }
> 
>  void lrc_destroy(struct kref *kref)
> diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c
> b/drivers/gpu/drm/i915/gt/intel_migrate.c
> index b405a04135ca21..2c076a51b66b30 100644
> --- a/drivers/gpu/drm/i915/gt/intel_migrate.c
> +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
> @@ -1116,7 +1116,7 @@ void intel_migrate_fini(struct intel_migrate *m)  {
>  	struct intel_context *ce;
> 
> -	ce = fetch_and_zero(&m->context);
> +	ce = exchange(&m->context, NULL);
>  	if (!ce)
>  		return;
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
> b/drivers/gpu/drm/i915/gt/intel_rc6.c
> index 2ee4051e4d9613..2451ebddb0f982 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> @@ -702,7 +702,7 @@ void intel_rc6_fini(struct intel_rc6 *rc6)
> 
>  	intel_rc6_disable(rc6);
> 
> -	pctx = fetch_and_zero(&rc6->pctx);
> +	pctx = exchange(&rc6->pctx, NULL);
>  	if (pctx)
>  		i915_gem_object_put(pctx);
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> b/drivers/gpu/drm/i915/gt/intel_rps.c
> index 9ad3bc7201cbaa..a102d8768e1d7b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -1831,7 +1831,7 @@ static void rps_work(struct work_struct *work)
>  	u32 pm_iir = 0;
> 
>  	spin_lock_irq(gt->irq_lock);
> -	pm_iir = fetch_and_zero(&rps->pm_iir) & rps->pm_events;
> +	pm_iir = exchange(&rps->pm_iir, 0) & rps->pm_events;
>  	client_boost = atomic_read(&rps->num_waiters);
>  	spin_unlock_irq(gt->irq_lock);
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c
> b/drivers/gpu/drm/i915/gt/selftest_context.c
> index 76fbae358072df..ca0a38de696eec 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> @@ -171,7 +171,7 @@ static int live_context_size(void *arg)
>  		 * active state is sufficient, we are only checking that we
>  		 * don't use more than we planned.
>  		 */
> -		saved = fetch_and_zero(&engine->default_state);
> +		saved = exchange(&engine->default_state, NULL);
> 
>  		/* Overlaps with the execlists redzone */
>  		engine->context_size += I915_GTT_PAGE_SIZE; diff --git
> a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
> b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
> index 87ceb0f374b673..9e901f1d5d76a9 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
> @@ -269,7 +269,7 @@ static int live_ctx_switch_wa(void *arg)
>  		if (IS_GRAPHICS_VER(gt->i915, 4, 5))
>  			continue; /* MI_STORE_DWORD is privileged! */
> 
> -		saved_wa = fetch_and_zero(&engine->wa_ctx.vma);
> +		saved_wa = exchange(&engine->wa_ctx.vma, NULL);
> 
>  		intel_engine_pm_get(engine);
>  		err = __live_ctx_switch_wa(engine);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c
> b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> index 522d0190509ccc..d74b13b1b38a6e 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
> @@ -892,7 +892,7 @@ static int create_watcher(struct hwsp_watcher *w,
> static int check_watcher(struct hwsp_watcher *w, const char *name,
>  			 bool (*op)(u32 hwsp, u32 seqno))
>  {
> -	struct i915_request *rq = fetch_and_zero(&w->rq);
> +	struct i915_request *rq = exchange(&w->rq, NULL);
>  	u32 offset, end;
>  	int err;
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 4f4b519e12c1b7..0085b1727dd47a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -166,7 +166,7 @@ static void __uc_capture_load_err_log(struct intel_uc
> *uc)
> 
>  static void __uc_free_load_err_log(struct intel_uc *uc)  {
> -	struct drm_i915_gem_object *log = fetch_and_zero(&uc-
> >load_err_log);
> +	struct drm_i915_gem_object *log = exchange(&uc->load_err_log,
> NULL);
> 
>  	if (log)
>  		i915_gem_object_put(log);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 6c83a8b66c9e32..44ff6da26bd698 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -1055,7 +1055,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw
> *uc_fw)
>  	if (!intel_uc_fw_is_available(uc_fw))
>  		return;
> 
> -	i915_gem_object_put(fetch_and_zero(&uc_fw->obj));
> +	i915_gem_object_put(exchange(&uc_fw->obj, 0));

Should this be set to NULL instead?

Thanks,
Tejas
> 
>  	intel_uc_fw_change_status(uc_fw,
> INTEL_UC_FIRMWARE_SELECTED);  }
> --
> 2.34.1

Re: [Intel-gfx] [PATCH 3/5] drm/i915/gt: kill fetch_and_zero usage

[Andrzej Hajda]

On 12.12.2022 10:14, Upadhyay, Tejas wrote:
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Andrzej Hajda
>> Sent: Friday, December 9, 2022 9:19 PM
>> To: linux-kernel@vger.kernel.org; intel-gfx@lists.freedesktop.org; dri-
>> devel@lists.freedesktop.org
>> Cc: Hajda, Andrzej <andrzej.hajda@intel.com>; Arnd Bergmann
>> <arnd@arndb.de>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Andrew Morton
>> <akpm@linux-foundation.org>; Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com>
>> Subject: [Intel-gfx] [PATCH 3/5] drm/i915/gt: kill fetch_and_zero usage
>>
>> Better use recently introduced kernel core helper.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c            | 2 +-
>>   drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c     | 4 ++--
>>   drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++--
>>   drivers/gpu/drm/i915/gt/intel_ggtt.c                 | 4 ++--
>>   drivers/gpu/drm/i915/gt/intel_gsc.c                  | 2 +-
>>   drivers/gpu/drm/i915/gt/intel_gt.c                   | 4 ++--
>>   drivers/gpu/drm/i915/gt/intel_gt_pm.c                | 2 +-
>>   drivers/gpu/drm/i915/gt/intel_lrc.c                  | 6 +++---
>>   drivers/gpu/drm/i915/gt/intel_migrate.c              | 2 +-
>>   drivers/gpu/drm/i915/gt/intel_rc6.c                  | 2 +-
>>   drivers/gpu/drm/i915/gt/intel_rps.c                  | 2 +-
>>   drivers/gpu/drm/i915/gt/selftest_context.c           | 2 +-
>>   drivers/gpu/drm/i915/gt/selftest_ring_submission.c   | 2 +-
>>   drivers/gpu/drm/i915/gt/selftest_timeline.c          | 2 +-
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c                | 2 +-
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c             | 2 +-
>>   16 files changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index c33e0d72d6702b..de318d96d52abd 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -1024,7 +1024,7 @@ static void cleanup_status_page(struct
>> intel_engine_cs *engine)
>>   	/* Prevent writes into HWSP after returning the page to the system */
>>   	intel_engine_set_hwsp_writemask(engine, ~0u);
>>
>> -	vma = fetch_and_zero(&engine->status_page.vma);
>> +	vma = exchange(&engine->status_page.vma, NULL);
>>   	if (!vma)
>>   		return;
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> index 9a527e1f5be655..6029fafaaa674f 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> @@ -229,7 +229,7 @@ static void heartbeat(struct work_struct *wrk)
>>   	mutex_unlock(&ce->timeline->mutex);
>>   out:
>>   	if (!engine->i915->params.enable_hangcheck ||
>> !next_heartbeat(engine))
>> -		i915_request_put(fetch_and_zero(&engine-
>>> heartbeat.systole));
>> +		i915_request_put(exchange(&engine->heartbeat.systole, 0));
>>   	intel_engine_pm_put(engine);
>>   }
>>
>> @@ -244,7 +244,7 @@ void intel_engine_unpark_heartbeat(struct
>> intel_engine_cs *engine)  void intel_engine_park_heartbeat(struct
>> intel_engine_cs *engine)  {
>>   	if (cancel_delayed_work(&engine->heartbeat.work))
>> -		i915_request_put(fetch_and_zero(&engine-
>>> heartbeat.systole));
>> +		i915_request_put(exchange(&engine->heartbeat.systole, 0));
>>   }
>>
>>   void intel_gt_unpark_heartbeats(struct intel_gt *gt) diff --git
>> a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> index 49a8f10d76c77b..29e78078d55a8b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> @@ -3197,7 +3197,7 @@ static void execlists_reset_cancel(struct
>> intel_engine_cs *engine)
>>   		RB_CLEAR_NODE(rb);
>>
>>   		spin_lock(&ve->base.sched_engine->lock);
>> -		rq = fetch_and_zero(&ve->request);
>> +		rq = exchange(&ve->request, NULL);
>>   		if (rq) {
>>   			if (i915_request_mark_eio(rq)) {
>>   				rq->engine = engine;
>> @@ -3602,7 +3602,7 @@ static void rcu_virtual_context_destroy(struct
>> work_struct *wrk)
>>
>>   		spin_lock_irq(&ve->base.sched_engine->lock);
>>
>> -		old = fetch_and_zero(&ve->request);
>> +		old = exchange(&ve->request, NULL);
>>   		if (old) {
>>   			GEM_BUG_ON(!__i915_request_is_complete(old));
>>   			__i915_request_submit(old);
>> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> index 0c7fe360f87331..2eb0173c6e968c 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
>> @@ -684,7 +684,7 @@ static void fini_aliasing_ppgtt(struct i915_ggtt *ggtt)  {
>>   	struct i915_ppgtt *ppgtt;
>>
>> -	ppgtt = fetch_and_zero(&ggtt->alias);
>> +	ppgtt = exchange(&ggtt->alias, NULL);
>>   	if (!ppgtt)
>>   		return;
>>
>> @@ -1238,7 +1238,7 @@ bool i915_ggtt_resume_vm(struct
>> i915_address_space *vm)
>>   				   was_bound);
>>
>>   		if (obj) { /* only used during resume => exclusive access */
>> -			write_domain_objs |= fetch_and_zero(&obj-
>>> write_domain);
>> +			write_domain_objs |= exchange(&obj-
>>> write_domain, 0);
>>   			obj->read_domains |= I915_GEM_DOMAIN_GTT;
>>   		}
>>   	}
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c
>> b/drivers/gpu/drm/i915/gt/intel_gsc.c
>> index bcc3605158dbde..7226b42bb70b2a 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
>> @@ -70,7 +70,7 @@ gsc_ext_om_alloc(struct intel_gsc *gsc, struct
>> intel_gsc_intf *intf, size_t size
>>
>>   static void gsc_ext_om_destroy(struct intel_gsc_intf *intf)  {
>> -	struct drm_i915_gem_object *obj = fetch_and_zero(&intf->gem_obj);
>> +	struct drm_i915_gem_object *obj = exchange(&intf->gem_obj, NULL);
>>
>>   	if (!obj)
>>   		return;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index 4e7af9bc73ad05..a277bd47db813e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -757,7 +757,7 @@ int intel_gt_init(struct intel_gt *gt)
>>   	intel_uc_fini(&gt->uc);
>>   err_engines:
>>   	intel_engines_release(gt);
>> -	i915_vm_put(fetch_and_zero(&gt->vm));
>> +	i915_vm_put(exchange(&gt->vm, 0));
>>   err_pm:
>>   	intel_gt_pm_fini(gt);
>>   	intel_gt_fini_scratch(gt);
>> @@ -806,7 +806,7 @@ void intel_gt_driver_release(struct intel_gt *gt)  {
>>   	struct i915_address_space *vm;
>>
>> -	vm = fetch_and_zero(&gt->vm);
>> +	vm = exchange(&gt->vm, NULL);
>>   	if (vm) /* FIXME being called twice on error paths :( */
>>   		i915_vm_put(vm);
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> index 16db85fab0b19b..f066936994a9e2 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> @@ -123,7 +123,7 @@ static int __gt_unpark(struct intel_wakeref *wf)
>> static int __gt_park(struct intel_wakeref *wf)  {
>>   	struct intel_gt *gt = container_of(wf, typeof(*gt), wakeref);
>> -	intel_wakeref_t wakeref = fetch_and_zero(&gt->awake);
>> +	intel_wakeref_t wakeref = exchange(&gt->awake, 0);
>>   	struct drm_i915_private *i915 = gt->i915;
>>
>>   	GT_TRACE(gt, "\n");
>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> index 7771a19008c604..9a2bfb6d14196c 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> @@ -1144,7 +1144,7 @@ __lrc_alloc_state(struct intel_context *ce, struct
>> intel_engine_cs *engine)  static struct intel_timeline *  pinned_timeline(struct
>> intel_context *ce, struct intel_engine_cs *engine)  {
>> -	struct intel_timeline *tl = fetch_and_zero(&ce->timeline);
>> +	struct intel_timeline *tl = exchange(&ce->timeline, NULL);
>>
>>   	return intel_timeline_create_from_engine(engine,
>> page_unmask_bits(tl));  } @@ -1261,8 +1261,8 @@ void lrc_fini(struct
>> intel_context *ce)
>>   	if (!ce->state)
>>   		return;
>>
>> -	intel_ring_put(fetch_and_zero(&ce->ring));
>> -	i915_vma_put(fetch_and_zero(&ce->state));
>> +	intel_ring_put(exchange(&ce->ring, 0));
>> +	i915_vma_put(exchange(&ce->state, 0));
>>   }
>>
>>   void lrc_destroy(struct kref *kref)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c
>> b/drivers/gpu/drm/i915/gt/intel_migrate.c
>> index b405a04135ca21..2c076a51b66b30 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_migrate.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
>> @@ -1116,7 +1116,7 @@ void intel_migrate_fini(struct intel_migrate *m)  {
>>   	struct intel_context *ce;
>>
>> -	ce = fetch_and_zero(&m->context);
>> +	ce = exchange(&m->context, NULL);
>>   	if (!ce)
>>   		return;
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
>> b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> index 2ee4051e4d9613..2451ebddb0f982 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> @@ -702,7 +702,7 @@ void intel_rc6_fini(struct intel_rc6 *rc6)
>>
>>   	intel_rc6_disable(rc6);
>>
>> -	pctx = fetch_and_zero(&rc6->pctx);
>> +	pctx = exchange(&rc6->pctx, NULL);
>>   	if (pctx)
>>   		i915_gem_object_put(pctx);
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
>> b/drivers/gpu/drm/i915/gt/intel_rps.c
>> index 9ad3bc7201cbaa..a102d8768e1d7b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>> @@ -1831,7 +1831,7 @@ static void rps_work(struct work_struct *work)
>>   	u32 pm_iir = 0;
>>
>>   	spin_lock_irq(gt->irq_lock);
>> -	pm_iir = fetch_and_zero(&rps->pm_iir) & rps->pm_events;
>> +	pm_iir = exchange(&rps->pm_iir, 0) & rps->pm_events;
>>   	client_boost = atomic_read(&rps->num_waiters);
>>   	spin_unlock_irq(gt->irq_lock);
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c
>> b/drivers/gpu/drm/i915/gt/selftest_context.c
>> index 76fbae358072df..ca0a38de696eec 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
>> @@ -171,7 +171,7 @@ static int live_context_size(void *arg)
>>   		 * active state is sufficient, we are only checking that we
>>   		 * don't use more than we planned.
>>   		 */
>> -		saved = fetch_and_zero(&engine->default_state);
>> +		saved = exchange(&engine->default_state, NULL);
>>
>>   		/* Overlaps with the execlists redzone */
>>   		engine->context_size += I915_GTT_PAGE_SIZE; diff --git
>> a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
>> b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
>> index 87ceb0f374b673..9e901f1d5d76a9 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_ring_submission.c
>> @@ -269,7 +269,7 @@ static int live_ctx_switch_wa(void *arg)
>>   		if (IS_GRAPHICS_VER(gt->i915, 4, 5))
>>   			continue; /* MI_STORE_DWORD is privileged! */
>>
>> -		saved_wa = fetch_and_zero(&engine->wa_ctx.vma);
>> +		saved_wa = exchange(&engine->wa_ctx.vma, NULL);
>>
>>   		intel_engine_pm_get(engine);
>>   		err = __live_ctx_switch_wa(engine);
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> index 522d0190509ccc..d74b13b1b38a6e 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
>> @@ -892,7 +892,7 @@ static int create_watcher(struct hwsp_watcher *w,
>> static int check_watcher(struct hwsp_watcher *w, const char *name,
>>   			 bool (*op)(u32 hwsp, u32 seqno))
>>   {
>> -	struct i915_request *rq = fetch_and_zero(&w->rq);
>> +	struct i915_request *rq = exchange(&w->rq, NULL);
>>   	u32 offset, end;
>>   	int err;
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 4f4b519e12c1b7..0085b1727dd47a 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -166,7 +166,7 @@ static void __uc_capture_load_err_log(struct intel_uc
>> *uc)
>>
>>   static void __uc_free_load_err_log(struct intel_uc *uc)  {
>> -	struct drm_i915_gem_object *log = fetch_and_zero(&uc-
>>> load_err_log);
>> +	struct drm_i915_gem_object *log = exchange(&uc->load_err_log,
>> NULL);
>>
>>   	if (log)
>>   		i915_gem_object_put(log);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index 6c83a8b66c9e32..44ff6da26bd698 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -1055,7 +1055,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw
>> *uc_fw)
>>   	if (!intel_uc_fw_is_available(uc_fw))
>>   		return;
>>
>> -	i915_gem_object_put(fetch_and_zero(&uc_fw->obj));
>> +	i915_gem_object_put(exchange(&uc_fw->obj, 0));
> Should this be set to NULL instead?

Yes, apparently my cocci script was not able to parse type of uc_fw->obj.
Thx, will fix it in another iteration.

Regards
Andrzej



>
> Thanks,
> Tejas
>>   	intel_uc_fw_change_status(uc_fw,
>> INTEL_UC_FIRMWARE_SELECTED);  }
>> --
>> 2.34.1

[PATCH 4/5] drm/i915/gvt: kill fetch_and_zero usage

[Andrzej Hajda]

Better use recently introduced kernel core helper.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c     | 2 +-
 drivers/gpu/drm/i915/gvt/scheduler.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 077892a9aa8fdc..061302abb0a189 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1831,7 +1831,7 @@ static int init_service_thread(struct intel_gvt *gvt)
  */
 static void intel_gvt_clean_device(struct drm_i915_private *i915)
 {
-	struct intel_gvt *gvt = fetch_and_zero(&i915->gvt);
+	struct intel_gvt *gvt = exchange(&i915->gvt, NULL);
 
 	if (drm_WARN_ON(&i915->drm, !gvt))
 		return;
diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index 9cd8fcbf7cad16..6699135f366f3f 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -826,7 +826,7 @@ static int dispatch_workload(struct intel_vgpu_workload *workload)
 		/* We might still need to add request with
 		 * clean ctx to retire it properly..
 		 */
-		rq = fetch_and_zero(&workload->req);
+		rq = exchange(&workload->req, NULL);
 		i915_request_put(rq);
 	}
 
@@ -1103,7 +1103,7 @@ static void complete_current_workload(struct intel_gvt *gvt, int ring_id)
 				intel_vgpu_trigger_virtual_event(vgpu, event);
 		}
 
-		i915_request_put(fetch_and_zero(&workload->req));
+		i915_request_put(exchange(&workload->req, 0));
 	}
 
 	gvt_dbg_sched("ring id %d complete workload %p status %d\n",
-- 
2.34.1

[PATCH 5/5] drm/i915: kill fetch_and_zero

[Andrzej Hajda]

Better use recently introduced kernel core helper.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_pages.c             | 4 ++--
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c            | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c               | 6 +++---
 drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 4 ++--
 drivers/gpu/drm/i915/i915_hwmon.c                     | 2 +-
 drivers/gpu/drm/i915/i915_perf.c                      | 2 +-
 drivers/gpu/drm/i915/i915_query.c                     | 2 +-
 drivers/gpu/drm/i915/i915_request.c                   | 4 ++--
 drivers/gpu/drm/i915/i915_vma.c                       | 2 +-
 drivers/gpu/drm/i915/intel_memory_region.c            | 2 +-
 drivers/gpu/drm/i915/intel_uncore.c                   | 4 ++--
 drivers/gpu/drm/i915/intel_wakeref.c                  | 4 ++--
 drivers/gpu/drm/i915/pxp/intel_pxp.c                  | 2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c          | 4 ++--
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c              | 2 +-
 15 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 05a27723ebb8cb..5498bd00ffd5e1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -209,7 +209,7 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj)
 
 	assert_object_held_shared(obj);
 
-	pages = fetch_and_zero(&obj->mm.pages);
+	pages = exchange(&obj->mm.pages, NULL);
 	if (IS_ERR_OR_NULL(pages))
 		return pages;
 
@@ -515,7 +515,7 @@ void __i915_gem_object_release_map(struct drm_i915_gem_object *obj)
 	 * Furthermore, since this is an unsafe operation reserved only
 	 * for construction time manipulation, we ignore locking prudence.
 	 */
-	unmap_object(obj, page_mask_bits(fetch_and_zero(&obj->mm.mapping)));
+	unmap_object(obj, page_mask_bits(exchange(&obj->mm.mapping, 0)));
 
 	i915_gem_object_unpin_map(obj);
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index bc952107880734..6901b2529d1b29 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -652,7 +652,7 @@ static void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	struct drm_mm_node *stolen = fetch_and_zero(&obj->stolen);
+	struct drm_mm_node *stolen = exchange(&obj->stolen, NULL);
 
 	GEM_BUG_ON(!stolen);
 	i915_gem_stolen_remove_node(i915, stolen);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 5247d88b3c13e6..075112ebe30247 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -447,7 +447,7 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj)
 		 */
 		shmem_truncate_range(file_inode(i915_tt->filp),
 				     0, (loff_t)-1);
-		fput(fetch_and_zero(&i915_tt->filp));
+		fput(exchange(&i915_tt->filp, 0));
 	}
 
 	obj->write_domain = 0;
@@ -779,7 +779,7 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
 	int ret;
 
 	/* First try only the requested placement. No eviction. */
-	real_num_busy = fetch_and_zero(&placement->num_busy_placement);
+	real_num_busy = exchange(&placement->num_busy_placement, 0);
 	ret = ttm_bo_validate(bo, placement, &ctx);
 	if (ret) {
 		ret = i915_ttm_err_to_gem(ret);
@@ -905,7 +905,7 @@ static void i915_ttm_put_pages(struct drm_i915_gem_object *obj,
 	 */
 
 	if (obj->mm.rsgt)
-		i915_refct_sgt_put(fetch_and_zero(&obj->mm.rsgt));
+		i915_refct_sgt_put(exchange(&obj->mm.rsgt, 0));
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index ac02fb03659208..4d456f74d5d901 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -615,7 +615,7 @@ static void throttle_release(struct i915_request **q, int count)
 		if (IS_ERR_OR_NULL(q[i]))
 			continue;
 
-		i915_request_put(fetch_and_zero(&q[i]));
+		i915_request_put(exchange(&q[i], 0));
 	}
 }
 
@@ -1072,7 +1072,7 @@ __sseu_prepare(const char *name,
 err_fini:
 	igt_spinner_fini(*spin);
 err_free:
-	kfree(fetch_and_zero(spin));
+	kfree(exchange(spin, 0));
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index cca7a4350ec8fd..c9d02f1124a067 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -732,5 +732,5 @@ void i915_hwmon_register(struct drm_i915_private *i915)
 
 void i915_hwmon_unregister(struct drm_i915_private *i915)
 {
-	fetch_and_zero(&i915->hwmon);
+	exchange(&i915->hwmon, 0);
 }
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index d22f30dd4fba27..d8b412e75b6079 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1535,7 +1535,7 @@ static void oa_put_render_ctx_id(struct i915_perf_stream *stream)
 {
 	struct intel_context *ce;
 
-	ce = fetch_and_zero(&stream->pinned_ctx);
+	ce = exchange(&stream->pinned_ctx, NULL);
 	if (ce) {
 		ce->tag = 0; /* recomputed on next submission after parking */
 		intel_context_unpin(ce);
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 00871ef9979204..ace0f6f98a430b 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -404,7 +404,7 @@ static int query_perf_config_list(struct drm_i915_private *i915,
 		if (!ids)
 			return -ENOMEM;
 
-		alloc = fetch_and_zero(&n_configs);
+		alloc = exchange(&n_configs, 0);
 
 		ids[n_configs++] = 1ull; /* reserved for test_config */
 		rcu_read_lock();
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index f949a9495758a0..9173b6c33d03c4 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -117,7 +117,7 @@ static void i915_fence_release(struct dma_fence *fence)
 	GEM_BUG_ON(rq->guc_prio != GUC_PRIO_INIT &&
 		   rq->guc_prio != GUC_PRIO_FINI);
 
-	i915_request_free_capture_list(fetch_and_zero(&rq->capture_list));
+	i915_request_free_capture_list(exchange(&rq->capture_list, 0));
 	if (rq->batch_res) {
 		i915_vma_resource_put(rq->batch_res);
 		rq->batch_res = NULL;
@@ -1948,7 +1948,7 @@ static void request_wait_wake(struct dma_fence *fence, struct dma_fence_cb *cb)
 {
 	struct request_wait *wait = container_of(cb, typeof(*wait), cb);
 
-	wake_up_process(fetch_and_zero(&wait->tsk));
+	wake_up_process(exchange(&wait->tsk, 0));
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 34f0e6c923c26d..d3498314357073 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -635,7 +635,7 @@ void i915_vma_unpin_and_release(struct i915_vma **p_vma, unsigned int flags)
 	struct i915_vma *vma;
 	struct drm_i915_gem_object *obj;
 
-	vma = fetch_and_zero(p_vma);
+	vma = exchange(p_vma, NULL);
 	if (!vma)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index 9a4a7fb55582db..95d3ca1aa9a8f1 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -367,7 +367,7 @@ void intel_memory_regions_driver_release(struct drm_i915_private *i915)
 
 	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
 		struct intel_memory_region *region =
-			fetch_and_zero(&i915->mm.regions[i]);
+			exchange(&i915->mm.regions[i], NULL);
 
 		if (region)
 			intel_memory_region_destroy(region);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 614013745fcafe..c86abf1755039a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -627,7 +627,7 @@ void intel_uncore_resume_early(struct intel_uncore *uncore)
 	if (!intel_uncore_has_forcewake(uncore))
 		return;
 
-	restore_forcewake = fetch_and_zero(&uncore->fw_domains_saved);
+	restore_forcewake = exchange(&uncore->fw_domains_saved, 0);
 	forcewake_early_sanitize(uncore, restore_forcewake);
 
 	iosf_mbi_register_pmic_bus_access_notifier(&uncore->pmic_bus_access_nb);
@@ -2249,7 +2249,7 @@ static void fw_domain_fini(struct intel_uncore *uncore,
 
 	GEM_BUG_ON(domain_id >= FW_DOMAIN_ID_COUNT);
 
-	d = fetch_and_zero(&uncore->fw_domain[domain_id]);
+	d = exchange(&uncore->fw_domain[domain_id], NULL);
 	if (!d)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index dfd87d08221807..01ee7713e8171b 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -16,7 +16,7 @@ static void rpm_get(struct intel_wakeref *wf)
 
 static void rpm_put(struct intel_wakeref *wf)
 {
-	intel_wakeref_t wakeref = fetch_and_zero(&wf->wakeref);
+	intel_wakeref_t wakeref = exchange(&wf->wakeref, 0);
 
 	intel_runtime_pm_put(wf->rpm, wakeref);
 	INTEL_WAKEREF_BUG_ON(!wakeref);
@@ -134,7 +134,7 @@ static void wakeref_auto_timeout(struct timer_list *t)
 	if (!refcount_dec_and_lock_irqsave(&wf->count, &wf->lock, &flags))
 		return;
 
-	wakeref = fetch_and_zero(&wf->wakeref);
+	wakeref = exchange(&wf->wakeref, 0);
 	spin_unlock_irqrestore(&wf->lock, flags);
 
 	intel_runtime_pm_put(wf->rpm, wakeref);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 5efe61f6754601..d2c13bf7e9acf2 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -104,7 +104,7 @@ static int create_vcs_context(struct intel_pxp *pxp)
 static void destroy_vcs_context(struct intel_pxp *pxp)
 {
 	if (pxp->ce)
-		intel_engine_destroy_pinned_context(fetch_and_zero(&pxp->ce));
+		intel_engine_destroy_pinned_context(exchange(&pxp->ce, 0));
 }
 
 static void pxp_init_full(struct intel_pxp *pxp)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 85572360c71a99..89e37ebb93b0ab 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -132,7 +132,7 @@ static void pxp_terminate(struct intel_pxp *pxp)
 static void pxp_terminate_complete(struct intel_pxp *pxp)
 {
 	/* Re-create the arb session after teardown handle complete */
-	if (fetch_and_zero(&pxp->hw_state_invalidated))
+	if (exchange(&pxp->hw_state_invalidated, 0))
 		pxp_create_arb_session(pxp);
 
 	complete_all(&pxp->termination);
@@ -146,7 +146,7 @@ static void pxp_session_work(struct work_struct *work)
 	u32 events = 0;
 
 	spin_lock_irq(gt->irq_lock);
-	events = fetch_and_zero(&pxp->session_events);
+	events = exchange(&pxp->session_events, 0);
 	spin_unlock_irq(gt->irq_lock);
 
 	if (!events)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index b0c9170b139542..345656515b8f0e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -231,7 +231,7 @@ static int alloc_streaming_command(struct intel_pxp *pxp)
 
 static void free_streaming_command(struct intel_pxp *pxp)
 {
-	struct drm_i915_gem_object *obj = fetch_and_zero(&pxp->stream_cmd.obj);
+	struct drm_i915_gem_object *obj = exchange(&pxp->stream_cmd.obj, NULL);
 
 	if (!obj)
 		return;
-- 
2.34.1

Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Arnd Bergmann]

On Fri, Dec 9, 2022, at 16:48, Andrzej Hajda wrote:
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases. Since name xchg is already in use and __xchg is used
> in architecture code, proposition is to name the macro exchange.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>

While I generally don't like type invariant calling conventions
of xchg() and cmpxchg(), having a new function that has a similar
name without being able to tell which one is which from the
name seems more confusing.

Since __xchg() is only used on 11 architectures as an internal
name for the backing of arch_xchg() or arch_xchg_relaxed(),
maybe we can instead rename those to __arch_xchg() and use the
__xchg() name for the new non-atomic version?

> +/**
> + * exchange - set variable pointed by @ptr to @val, return old value
> + * @ptr: pointer to affected variable
> + * @val: value to be written
> + *
> + * This is non-atomic variant of xchg.
> + */
> +#define exchange(ptr, val) ({		\
> +	typeof(ptr) __ptr = ptr;	\
> +	typeof(*__ptr) __t = *__ptr;	\

I think you can better express this using __auto_type than typeof(),
it is now provided by all supported compilers now.

     Arnd

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andrzej Hajda]

On 09.12.2022 18:16, Arnd Bergmann wrote:
> On Fri, Dec 9, 2022, at 16:48, Andrzej Hajda wrote:
>> The pattern of setting variable with new value and returning old
>> one is very common in kernel. Usually atomicity of the operation
>> is not required, so xchg seems to be suboptimal and confusing in
>> such cases. Since name xchg is already in use and __xchg is used
>> in architecture code, proposition is to name the macro exchange.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> While I generally don't like type invariant calling conventions
> of xchg() and cmpxchg(), having a new function that has a similar
> name without being able to tell which one is which from the
> name seems more confusing.
> 
> Since __xchg() is only used on 11 architectures as an internal

Quite big number for 'only' :)

> name for the backing of arch_xchg() or arch_xchg_relaxed(),
> maybe we can instead rename those to __arch_xchg() and use the
> __xchg() name for the new non-atomic version?

I will try, but even compile test will be some challenge, need to find 
cross-compilers for these archs.

Btw exchange is not totally new name, for example C++ uses it [1].

[1]: https://en.cppreference.com/w/cpp/utility/exchange

Regards
Andrzej

> 
>> +/**
>> + * exchange - set variable pointed by @ptr to @val, return old value
>> + * @ptr: pointer to affected variable
>> + * @val: value to be written
>> + *
>> + * This is non-atomic variant of xchg.
>> + */
>> +#define exchange(ptr, val) ({		\
>> +	typeof(ptr) __ptr = ptr;	\
>> +	typeof(*__ptr) __t = *__ptr;	\
> 
> I think you can better express this using __auto_type than typeof(),
> it is now provided by all supported compilers now.
> 
>       Arnd

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Arnd Bergmann]

On Tue, Dec 13, 2022, at 10:28, Andrzej Hajda wrote:
> On 09.12.2022 18:16, Arnd Bergmann wrote:
>> name for the backing of arch_xchg() or arch_xchg_relaxed(),
>> maybe we can instead rename those to __arch_xchg() and use the
>> __xchg() name for the new non-atomic version?
>
> I will try, but even compile test will be some challenge, need to find 
> cross-compilers for these archs.

I maintain this set of cross compilers, let me know if you
have problems running them:

https://mirrors.edge.kernel.org/pub/tools/crosstool/

    Arnd

Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases. Since name xchg is already in use and __xchg is used
> in architecture code, proposition is to name the macro exchange.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> Hi,
> 
> I hope there will be place for such tiny helper in kernel.
> Quick cocci analyze shows there is probably few thousands places
> where it could be used, of course I do not intend to do it :).
> 
> I was not sure where to put this macro, I hope near swap definition
> is the most suitable place.

Ah, swap() in this context is not the same. minmax.h hosts it because
it's often related to the swap function in the sort-type algorithms.

> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> more confused than me, to who address this patch.

...

>  include/linux/minmax.h | 14 ++++++++++++++

Does it really suit this header? I would expect something else.
Maybe include/linux/non-atomic/xchg.h, dunno.

Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
and related headers? Maybe some ideas can be taken from there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Fri, Dec 09, 2022 at 08:56:28PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > I hope there will be place for such tiny helper in kernel.
> > Quick cocci analyze shows there is probably few thousands places
> > where it could be used, of course I do not intend to do it :).
> > 
> > I was not sure where to put this macro, I hope near swap definition
> > is the most suitable place.
> 
> Ah, swap() in this context is not the same. minmax.h hosts it because
> it's often related to the swap function in the sort-type algorithms.
> 
> > Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > more confused than me, to who address this patch.
> 
> ...
> 
> >  include/linux/minmax.h | 14 ++++++++++++++
> 
> Does it really suit this header? I would expect something else.

> Maybe include/linux/non-atomic/xchg.h, dunno.

It may become a candidate to host io-64 non-atomic versions and other
non-atomic generic headers...

> Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> and related headers? Maybe some ideas can be taken from there?

-- 
With Best Regards,
Andy Shevchenko

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andrzej Hajda]

On 09.12.2022 19:56, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:
>> The pattern of setting variable with new value and returning old
>> one is very common in kernel. Usually atomicity of the operation
>> is not required, so xchg seems to be suboptimal and confusing in
>> such cases. Since name xchg is already in use and __xchg is used
>> in architecture code, proposition is to name the macro exchange.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>> Hi,
>>
>> I hope there will be place for such tiny helper in kernel.
>> Quick cocci analyze shows there is probably few thousands places
>> where it could be used, of course I do not intend to do it :).
>>
>> I was not sure where to put this macro, I hope near swap definition
>> is the most suitable place.
> 
> Ah, swap() in this context is not the same. minmax.h hosts it because
> it's often related to the swap function in the sort-type algorithms. > >> Moreover sorry if to/cc is not correct - get_maintainers.pl was
>> more confused than me, to who address this patch.
> 
> ...
> 
>>   include/linux/minmax.h | 14 ++++++++++++++
> 
> Does it really suit this header? I would expect something else.
> Maybe include/linux/non-atomic/xchg.h, dunno.

non-atomic seems quite strange for me, I would assume everything not in 
atomic is non-atomic, unless explicitly specified.

> 
> Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> and related headers? Maybe some ideas can be taken from there?
> 

Grepping it didn't give any clue.

Looking at 'near' languages just to get an idea (they name the function 
differently):

C++ [1]: exchange and swap are in utility header
Rust[2]: replace and swap are in std::mem module

This is some argument to put them together.

[1]: https://en.cppreference.com/w/cpp/header/utility
[2]: https://doc.rust-lang.org/std/mem/index.html

Regards
Andrzej

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Andy Shevchenko]

On Tue, Dec 13, 2022 at 11:09:12AM +0100, Andrzej Hajda wrote:
> On 09.12.2022 19:56, Andy Shevchenko wrote:
> > On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > > I hope there will be place for such tiny helper in kernel.
> > > Quick cocci analyze shows there is probably few thousands places
> > > where it could be used, of course I do not intend to do it :).
> > > 
> > > I was not sure where to put this macro, I hope near swap definition
> > > is the most suitable place.
> > 
> > Ah, swap() in this context is not the same. minmax.h hosts it because
> > it's often related to the swap function in the sort-type algorithms.

> >> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > > more confused than me, to who address this patch.

...

> > >   include/linux/minmax.h | 14 ++++++++++++++
> > 
> > Does it really suit this header? I would expect something else.
> > Maybe include/linux/non-atomic/xchg.h, dunno.
> 
> non-atomic seems quite strange for me, I would assume everything not in
> atomic is non-atomic, unless explicitly specified.
> 
> > 
> > Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> > and related headers? Maybe some ideas can be taken from there?
> 
> Grepping it didn't give any clue.
> 
> Looking at 'near' languages just to get an idea (they name the function
> differently):
> 
> C++ [1]: exchange and swap are in utility header
> Rust[2]: replace and swap are in std::mem module
> 
> This is some argument to put them together.

Again, I left the above part on top of this message, the swap() in Linux kernel
is not related to __xchg() or similar. That said, minmax.h is not a good place
for the latter.

> [1]: https://en.cppreference.com/w/cpp/header/utility
> [2]: https://doc.rust-lang.org/std/mem/index.html

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[David Laight]

From: Andrzej Hajda <andrzej.hajda@intel.com>
> Sent: 09 December 2022 15:49
> 
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases. Since name xchg is already in use and __xchg is used
> in architecture code, proposition is to name the macro exchange.

Dunno, if it is non-atomic then two separate assignment statements
is decidedly more obvious and needs less brain cells to process.
Otherwise someone will assume 'something clever' is going on
and the operation is atomic.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Daniel Vetter]

On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> From: Andrzej Hajda <andrzej.hajda@intel.com>
> > Sent: 09 December 2022 15:49
> > 
> > The pattern of setting variable with new value and returning old
> > one is very common in kernel. Usually atomicity of the operation
> > is not required, so xchg seems to be suboptimal and confusing in
> > such cases. Since name xchg is already in use and __xchg is used
> > in architecture code, proposition is to name the macro exchange.
> 
> Dunno, if it is non-atomic then two separate assignment statements
> is decidedly more obvious and needs less brain cells to process.
> Otherwise someone will assume 'something clever' is going on
> and the operation is atomic.

Yes, this also my take. The i915 code that uses this to excess is decidely
unreadable imo, and the macro should simply be replaced by open-coded
versions.

Not moved into shared headers where even more people can play funny games
with it.

I think swap() is a standard idiom in C, this one here just isn't.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Jani Nikula]

On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
>> From: Andrzej Hajda <andrzej.hajda@intel.com>
>> > Sent: 09 December 2022 15:49
>> > 
>> > The pattern of setting variable with new value and returning old
>> > one is very common in kernel. Usually atomicity of the operation
>> > is not required, so xchg seems to be suboptimal and confusing in
>> > such cases. Since name xchg is already in use and __xchg is used
>> > in architecture code, proposition is to name the macro exchange.
>> 
>> Dunno, if it is non-atomic then two separate assignment statements
>> is decidedly more obvious and needs less brain cells to process.
>> Otherwise someone will assume 'something clever' is going on
>> and the operation is atomic.
>
> Yes, this also my take. The i915 code that uses this to excess is decidely
> unreadable imo, and the macro should simply be replaced by open-coded
> versions.
>
> Not moved into shared headers where even more people can play funny games
> with it.

My stand in i915 has been that the local fetch_and_zero() needs to
go. Either replaced by a common helper in core kernel headers, or open
coded, I personally don't care, but the local version can't stay.

My rationale has been that fetch_and_zero() looks atomic and looks like
it comes from shared headers, but it's neither. It's deceptive. It
started small and harmless, but things like this just proliferate and
get copy-pasted all over the place.

So here we are, with Andrzej looking to add the common helper. And the
same concerns crop up. What should it be called to make it clear that
it's not atomic? Is that possible?


BR,
Jani.



>
> I think swap() is a standard idiom in C, this one here just isn't.
> -Daniel

-- 
Jani Nikula, Intel Open Source Graphics Center

RE: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[David Laight]

From: Jani Nikula
> Sent: 05 January 2023 13:28
> 
> On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> >> From: Andrzej Hajda <andrzej.hajda@intel.com>
> >> > Sent: 09 December 2022 15:49
> >> >
> >> > The pattern of setting variable with new value and returning old
> >> > one is very common in kernel. Usually atomicity of the operation
> >> > is not required, so xchg seems to be suboptimal and confusing in
> >> > such cases. Since name xchg is already in use and __xchg is used
> >> > in architecture code, proposition is to name the macro exchange.
> >>
> >> Dunno, if it is non-atomic then two separate assignment statements
> >> is decidedly more obvious and needs less brain cells to process.
> >> Otherwise someone will assume 'something clever' is going on
> >> and the operation is atomic.
> >
> > Yes, this also my take. The i915 code that uses this to excess is decidely
> > unreadable imo, and the macro should simply be replaced by open-coded
> > versions.
> >
> > Not moved into shared headers where even more people can play funny games
> > with it.
> 
> My stand in i915 has been that the local fetch_and_zero() needs to
> go. Either replaced by a common helper in core kernel headers, or open
> coded, I personally don't care, but the local version can't stay.
> 
> My rationale has been that fetch_and_zero() looks atomic and looks like
> it comes from shared headers, but it's neither. It's deceptive. It
> started small and harmless, but things like this just proliferate and
> get copy-pasted all over the place.
> 
> So here we are, with Andrzej looking to add the common helper. And the
> same concerns crop up. What should it be called to make it clear that
> it's not atomic? Is that possible?

old_value = read_write(variable, new_value);

But two statements are much clearer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Daniel Vetter]

On Thu, Jan 05, 2023 at 01:34:33PM +0000, David Laight wrote:
> From: Jani Nikula
> > Sent: 05 January 2023 13:28
> > 
> > On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> > >> From: Andrzej Hajda <andrzej.hajda@intel.com>
> > >> > Sent: 09 December 2022 15:49
> > >> >
> > >> > The pattern of setting variable with new value and returning old
> > >> > one is very common in kernel. Usually atomicity of the operation
> > >> > is not required, so xchg seems to be suboptimal and confusing in
> > >> > such cases. Since name xchg is already in use and __xchg is used
> > >> > in architecture code, proposition is to name the macro exchange.
> > >>
> > >> Dunno, if it is non-atomic then two separate assignment statements
> > >> is decidedly more obvious and needs less brain cells to process.
> > >> Otherwise someone will assume 'something clever' is going on
> > >> and the operation is atomic.
> > >
> > > Yes, this also my take. The i915 code that uses this to excess is decidely
> > > unreadable imo, and the macro should simply be replaced by open-coded
> > > versions.
> > >
> > > Not moved into shared headers where even more people can play funny games
> > > with it.
> > 
> > My stand in i915 has been that the local fetch_and_zero() needs to
> > go. Either replaced by a common helper in core kernel headers, or open
> > coded, I personally don't care, but the local version can't stay.
> > 
> > My rationale has been that fetch_and_zero() looks atomic and looks like
> > it comes from shared headers, but it's neither. It's deceptive. It
> > started small and harmless, but things like this just proliferate and
> > get copy-pasted all over the place.

Yeah the entire "is it atomic or not" is the issue on top here.

> > So here we are, with Andrzej looking to add the common helper. And the
> > same concerns crop up. What should it be called to make it clear that
> > it's not atomic? Is that possible?
> 
> old_value = read_write(variable, new_value);
> 
> But two statements are much clearer.

Yeah this is my point for fetch_and_zero or any of the other proposals.
We're essentially replacing these two lines:

	var = some->pointer->chase;
	some->pointer->chase = NULL;

with a macro. C is verbose, and sometimes painfully so, if the pointer
chase is really to onerous then I think that should be refactored with a
meaningfully locally name variable, not fancy macros wrapped around to
golf a few characters away.

But what about swap() you ask? That one needs a temp variable, and it does
make sense to hide that in a ({}) block in a macro. But for the above two
lines I really don't see a point outside of obfuscated C contexts.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

RE: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[David Laight]

From: Daniel Vetter
> Sent: 05 January 2023 14:13
...
> > > So here we are, with Andrzej looking to add the common helper. And the
> > > same concerns crop up. What should it be called to make it clear that
> > > it's not atomic? Is that possible?
> >
> > old_value = read_write(variable, new_value);
> >
> > But two statements are much clearer.
> 
> Yeah this is my point for fetch_and_zero or any of the other proposals.
> We're essentially replacing these two lines:
> 
> 	var = some->pointer->chase;
> 	some->pointer->chase = NULL;
> 
> with a macro. C is verbose, and sometimes painfully so,

Try ADA or VHDL :-)

> if the pointer
> chase is really to onerous then I think that should be refactored with a
> meaningfully locally name variable, not fancy macros wrapped around to
> golf a few characters away.

Provided 'var' is a local the compiler is pretty likely to only do the
'pointer chase' once.
You can also do:
	var = NULL;
	swap(some->pointer->chase, var);
and get pretty much the same object code.

> But what about swap() you ask? That one needs a temp variable, and it does
> make sense to hide that in a ({}) block in a macro.

Sometimes, but not enough for the 'missed opportunity for swap()'
message. 

> But for the above two
> lines I really don't see a point outside of obfuscated C contexts.

Indeed.

Isn't the suggested __xchg() in one of the 'reserved for implementation'
namespaces - so shouldn't be a function that might be expected to be
actually used.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Daniel Vetter]

On Thu, Jan 05, 2023 at 02:41:43PM +0000, David Laight wrote:
> From: Daniel Vetter
> > Sent: 05 January 2023 14:13
> ...
> > > > So here we are, with Andrzej looking to add the common helper. And the
> > > > same concerns crop up. What should it be called to make it clear that
> > > > it's not atomic? Is that possible?
> > >
> > > old_value = read_write(variable, new_value);
> > >
> > > But two statements are much clearer.
> > 
> > Yeah this is my point for fetch_and_zero or any of the other proposals.
> > We're essentially replacing these two lines:
> > 
> > 	var = some->pointer->chase;
> > 	some->pointer->chase = NULL;
> > 
> > with a macro. C is verbose, and sometimes painfully so,
> 
> Try ADA or VHDL :-)
> 
> > if the pointer
> > chase is really to onerous then I think that should be refactored with a
> > meaningfully locally name variable, not fancy macros wrapped around to
> > golf a few characters away.
> 
> Provided 'var' is a local the compiler is pretty likely to only do the
> 'pointer chase' once.
> You can also do:
> 	var = NULL;
> 	swap(some->pointer->chase, var);
> and get pretty much the same object code.
> 
> > But what about swap() you ask? That one needs a temp variable, and it does
> > make sense to hide that in a ({}) block in a macro.
> 
> Sometimes, but not enough for the 'missed opportunity for swap()'
> message. 
> 
> > But for the above two
> > lines I really don't see a point outside of obfuscated C contexts.
> 
> Indeed.
> 
> Isn't the suggested __xchg() in one of the 'reserved for implementation'
> namespaces - so shouldn't be a function that might be expected to be
> actually used.

It's more fun, for the atomic functions which don't have the atomic_
prefix in their names, the __ prefixed versions provide the non-atomic
implementation.  This pattern was started with the long * bitops stuff for
managing really big bitmasks.

And I really don't think it's a great function name scheme that we should
proliferate.

The "reserved for implementation" only applies to the standard C library
in userspace, which the kernel doesn't use, so can fairly freely use that
namespace.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Mark Rutland]

On Thu, Jan 05, 2023 at 03:57:25PM +0100, Daniel Vetter wrote:
> It's more fun, for the atomic functions which don't have the atomic_
> prefix in their names, the __ prefixed versions provide the non-atomic
> implementation.  This pattern was started with the long * bitops stuff for
> managing really big bitmasks.
> 
> And I really don't think it's a great function name scheme that we should
> proliferate.

FWIW I agree it's not great, but we're stuck between a rock and a bikeshed
w.r.t. better naming -- it's quite hard to clean that up becuase the atomic_*()
namespace is reserved for atomic_t (and mirrors atomic64_*() and
atomic_long_*()).

We could consider renaming atomic_t to atomic32_t and atomic_*() to
atomic32_*(), which'd free up the atomic_*() namespace for more genral usage
(e.g. allowing us to have atomic_xchg() and xhcg(), with the latter not being
atomic).

Thanks,
Mark.

Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Tvrtko Ursulin]

On 05/01/2023 13:34, David Laight wrote:
> From: Jani Nikula
>> Sent: 05 January 2023 13:28
>>
>> On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
>>>> From: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>> Sent: 09 December 2022 15:49
>>>>>
>>>>> The pattern of setting variable with new value and returning old
>>>>> one is very common in kernel. Usually atomicity of the operation
>>>>> is not required, so xchg seems to be suboptimal and confusing in
>>>>> such cases. Since name xchg is already in use and __xchg is used
>>>>> in architecture code, proposition is to name the macro exchange.
>>>>
>>>> Dunno, if it is non-atomic then two separate assignment statements
>>>> is decidedly more obvious and needs less brain cells to process.
>>>> Otherwise someone will assume 'something clever' is going on
>>>> and the operation is atomic.
>>>
>>> Yes, this also my take. The i915 code that uses this to excess is decidely
>>> unreadable imo, and the macro should simply be replaced by open-coded
>>> versions.
>>>
>>> Not moved into shared headers where even more people can play funny games
>>> with it.
>>
>> My stand in i915 has been that the local fetch_and_zero() needs to
>> go. Either replaced by a common helper in core kernel headers, or open
>> coded, I personally don't care, but the local version can't stay.
>>
>> My rationale has been that fetch_and_zero() looks atomic and looks like
>> it comes from shared headers, but it's neither. It's deceptive. It
>> started small and harmless, but things like this just proliferate and
>> get copy-pasted all over the place.
>>
>> So here we are, with Andrzej looking to add the common helper. And the
>> same concerns crop up. What should it be called to make it clear that
>> it's not atomic? Is that possible?
> 
> old_value = read_write(variable, new_value);
> 
> But two statements are much clearer.

In a later thread there was more discussion on this and some new 
suggestions - exchange(), replace() or even take() sound fine to me. 
Last one is perhaps most specialized if it implies zeroing, which I at 
least assume it does.

All three are distant enough from atomic connotations of xchg. If that 
was a concern with __xchg, which I not sure it should be since there is 
"prior art" in the kernel for atomic vs non-atomic like set_bit and 
__set_bit.

My 2c, regardless of what name, that it is not something which is 
strictly needed, but a convenient syntactic sugar. (Exploded line counts 
with sometimes single use local variables are a bit meh.) And I am not 
really sure that open coding is more readable once the new pattern would 
be established. In short, if there can be swap there can be $insert_name 
too I guess.

Bonus points if needlessly atomic sites can be converted but identifying 
them is probably an exercise for a later phase.

Regards,

Tvrtko

P.S. FWIW my preference are either replace() or __xchg().