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

[Andrzej Hajda <andrzej.hajda@intel.com>]

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.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.
> 
> +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.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);
>>
>