Messages in this thread | | | Date | Wed, 25 May 2022 09:31:33 -0400 | From | Phil Auld <> | Subject | Re: [PATCH] cpuhp: make target_store() a nop when target == state |
| |
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 > > > > -- >
--
| |