On Thu, 08 Dec 2022, Jani Nikula <jani.nik...@linux.intel.com> 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.
PS. The origin is commit 78ef2d9abad6 ("drm/i915: Add fetch_and_zero()
macro") which presents the idea of making it a pattern that can be
extended for atomic use, but six years and counting, that never
happened.
>
>
> 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);
--
Jani Nikula, Intel Open Source Graphics Center
