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

[Valentin Schneider <vschneid@redhat.com>]

On 24/05/22 12:39, Phil Auld wrote:
> On Tue, May 24, 2022 at 04:11:51PM +0100 Valentin Schneider wrote:
>
>>
>> _cpu_up() has:
>>
>>      /*
>>       * The caller of cpu_up() might have raced with another
>>       * caller. Nothing to do.
>>       */
>>      if (st->state >= target)
>>              goto out;
>>
>> Looks like we want an equivalent in _cpu_down(), what do you think?
>
> Maybe. I still think that
>
>> >         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.

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