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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@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.


BR,
Jani.


>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@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);

-- 
Jani Nikula, Intel Open Source Graphics Center