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

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >   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);