Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: use fetch_and_zero if applicable

[Andrzej Hajda]

On 08.12.2022 14:36, Vivi, Rodrigo wrote:
On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote:
On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.ha...@intel.com> wrote:
Simplify the code.

Personally, I absolutely hate fetch_and_zero().

I understand the point, but there are two main traps:

First, the name implies atomicity, which there is none at all.

Second, the name implies it's part of a kernel core header, which it
isn't, and this just amplifies the first point.

It's surprising and misleading, and those are not things I like about
interfaces in the kernel.

I would not like to see this proliferate. If fetch_and_zero() was
atomic
*and* part of a core kernel header, it would be a different matter.
But
I don't think that's going to happen, exactly because it won't be
atomic
and the name implies it is.

+1 here.

Please let's go the other way around and try to kill macros like this.

we either kill or we ensure this gets accepted in the core kernel
libraries.


There is about 80 uses of the macro in i915. So I guessed this is 
accepted solution in i915 :) Moreover it looked to me as a nice
shortcut.

If not, I can replace it with xchg(ptr, 0), besides tiny overkill, 
assuming atomicity is not required here, it should work.

I can also expand it :) - quite big patch, but cocci should do the work.

Anyway I think it would be good to take some decision here, to avoid 
further confusions.

Regards
Andrzej




BR,
Jani.



Signed-off-by: Andrzej Hajda <andrzej.ha...@intel.com>
---
  drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++--------
  1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
b/drivers/gpu/drm/i915/display/intel_hotplug.c
index 907ab7526cb478..2972d7533da44e 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
@@ -304,10 +304,8 @@ static void i915_digport_work_func(struct
work_struct *work)
         u32 old_bits = 0;
  
         spin_lock_irq(&dev_priv->irq_lock);
-       long_port_mask = dev_priv->display.hotplug.long_port_mask;
-       dev_priv->display.hotplug.long_port_mask = 0;
-       short_port_mask = dev_priv-
display.hotplug.short_port_mask;
-       dev_priv->display.hotplug.short_port_mask = 0;
+       long_port_mask = fetch_and_zero(&dev_priv-
display.hotplug.long_port_mask);
+       short_port_mask = fetch_and_zero(&dev_priv-
display.hotplug.short_port_mask);
         spin_unlock_irq(&dev_priv->irq_lock);
  
         for_each_intel_encoder(&dev_priv->drm, encoder) {
@@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct
work_struct *work)
  
         spin_lock_irq(&dev_priv->irq_lock);
  
-       hpd_event_bits = dev_priv->display.hotplug.event_bits;
-       dev_priv->display.hotplug.event_bits = 0;
-       hpd_retry_bits = dev_priv->display.hotplug.retry_bits;
-       dev_priv->display.hotplug.retry_bits = 0;
+       hpd_event_bits = fetch_and_zero(&dev_priv-
display.hotplug.event_bits);
+       hpd_retry_bits = fetch_and_zero(&dev_priv-
display.hotplug.retry_bits);
  
         /* Enable polling for connectors which had HPD IRQ storms
*/
         intel_hpd_irq_storm_switch_to_polling(dev_priv);