Re: [PATCH] cpuhp: make target_store() a nop when target == state

[Phil Auld <pauld@redhat.com>]

On Wed, May 25, 2022 at 10:48:31AM +0100 Valentin Schneider wrote:
> On 24/05/22 12:39, Phil Auld wrote:

> >
> >> >         if (st->state < target)
> >> >                 ret = cpu_up(dev->id, target);
> >> >         else
> >> >                 ret = cpu_down(dev->id, target);
> >
> > is not correct. If we catch the == case earlier then this makes
> > sense as is.
> >
> > I suppose "if (st->state <= target)" would work too since __cpu_up()
> > already checks. Catching this sooner seems better to me though.
> >
> 
> Yeah it would be neater to not even enter cpu_{up, down}(), but my paranoia
> makes me think we need the comparison to happen with at least the
> cpu_add_remove_lock held to make sure st->state isn't moving under our
> feet, otherwise we may still end up with target == state in _cpu_down() and
> hit the bug you're describing.
> 

This is what I was originally doing before I tried to "optimize" it:

         if (st->state < target)
                 ret = cpu_up(dev->id, target);
         else if (st->state > target)
                 ret = cpu_down(dev->id, target);

This does the check under the lock and just falls through if state==target.
I think I'll go back to that version.

I also noticed while testing that the boot cpu does not get its target set.
It's got state 233 but target 0.  So reading that out and writing it back
on offlines cpu0.   I'll try to find where that is not getting set.  


Thanks,

Phil

> >>
> >> >    ret = lock_device_hotplug_sysfs();
> >> >    if (ret)
> >> >            return ret;
> >> > --
> >> > 2.18.0
> >>
> >
> >
> > Cheers,
> > Phil
> >
> > --
> 

--