[PATCH 1/2] nvme: only allow entering LIVE from CONNECTING state

[Guenter Roeck]

On 4/29/25 23:43, Daniel Wagner wrote:
> On Tue, Apr 29, 2025 at 11:42:25AM -0700, Keith Busch wrote:
>> On Tue, Apr 29, 2025 at 11:23:25AM -0700, Guenter Roeck wrote:
>>> On 4/29/25 11:13, Keith Busch wrote:
>>>> On Mon, Apr 28, 2025 at 03:21:18PM +0200, Hannes Reinecke wrote:
>>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>>> index b502ac07483b..d3c4eacf607f 100644
>>>>>> --- a/drivers/nvme/host/core.c
>>>>>> +++ b/drivers/nvme/host/core.c
>>>>>> @@ -4493,7 +4493,8 @@ static void nvme_fw_act_work(struct work_struct *work)
>>>>>>                    msleep(100);
>>>>>>            }
>>>>>>
>>>>>> -       if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE))
>>>>>> +       if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING) ||
>>>>>> +           !nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE))
>>>>>>                    return;
>>>>>>
>>>>>>            nvme_unquiesce_io_queues(ctrl);
>>>>>
>>>>> I would rather have a separate state for firmware activation.
>>>>> (Ab-)using the 'RESETTING' state here has direct implications
>>>>> with the error handler, as for the error handler 'RESETTING'
>>>>> means that the error handler has been scheduled.
>>>>> Which is not true for firmware activation.
>>>>
>>>> But the point of having firmware activation set the state to RESETTING
>>>> was to fence off error handling from trying to schedule a real reset.
>>>> The fw activation work schedules its own recovery if it times out, but
>>>> we don't want any other recovery action or user requested resets to
>>>> proceed while an activation is still pending.
>>>
>>> Not only that; there are various checks against NVME_CTRL_RESETTING
>>> sprinkled through the code. What is the impact of introducing a new state
>>> without handling all those checks ?
>>
>> Good point, bad things will happen if these checks are not updated to
>> know about the new state. For example, nvme-pci will attempt aborting IO
>> or disabling the controller on a timeout instead of restarting the timer
>> as desired.
>>
>> Can we just revert the commit that prevented the RESETTING -> LIVE
>> transtion for now?
> 
> Unfortunately, it will break the FC error handling again(*). The
> simplest fix is right above, add the transition from RESETTING to
> CONNECTING and then to LIVE, IMO.
> 
> (*) ee59e3820ca9 ("nvme-fc: do not ignore connectivity loss during connecting")
>      f13409bb3f91 ("nvme-fc: rely on state transitions to handle connectivity loss")

I don't know if having nvme-fc _depend_ on state transition restrictions is good
or bad, and I don't know anything about the NVME state machine.

However, given that there _is_ a real regression in the code right now, I agree
that the solution (or call it workaround) suggested by Daniel (or something
similar) should be the way to go, and it would be easy to backport. After all,
firmware update is now broken in all stable release branches, and that is not
a good situation to be in.

Introducing a new state for firmware updates should be done carefully and not be
part of a regression fix which needs to be backported to all stable release
branches.

Thanks,
Guenter