Re: [PATCH v3 1/1] nvme-pci : Fix EEH failure on ppc after subsystem reset

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/10/24 18:02, Maurizio Lombardi wrote:
> út 4. 6. 2024 v 11:16 odesílatel Nilay Shroff <nilay@linux.ibm.com> napsal:
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index f5d150c62955..afb8419566a9 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -562,6 +562,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>>                 switch (old_state) {
>>                 case NVME_CTRL_NEW:
>>                 case NVME_CTRL_LIVE:
>> +               case NVME_CTRL_CONNECTING:
>>                         changed = true;
>>                         fallthrough;
>>                 default:
> 
> Side note:
> 
> I believe this could fix a race condition in the nvme-tcp driver.
> Some time ago we received a kernel dump where the controller was
> flagged as NVME_CTRL_LIVE but the sockets were marked as TCP_CLOSED,
> the kernel was hanging into io_work.
> It might be possible that the socket's nvme_tcp_error_recovery
> callback was executed while the ctrl was still marked as
> NVME_CTRL_CONNECTING, nvme_change_ctrl_state() doesn't allow a
> reconnecting controller to switch to the resetting state and therefore
> nvme_tcp_error_recovery() simply returns without triggering an error
> recovery.
> 
> nvme_tcp_setup_ctrl() then moves the ctrl to the LIVE state, but with
> closed tcp sockets.
> 
> I guess that this change would fix the possible race condition, even
> if I am not completely sure what could happen in case of a race
> between nvme_tcp_setup_ctrl() and the error recovery code.
> 
I think for your case you shall consider using nvme_wait_reset() before 
queuing error recovery work. For instance, it could be done something 
like below  assuming that all callers of nvme_tcp_error_recovery() are
allowed to sleep (not fully tested) :

static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
{
	if (!nvme_wait_reset(ctrl))
		return;

	dev_warn(ctrl->device, "starting error recovery\n");
	queue_work(nvme_reset_wq, &to_tcp_ctrl(ctrl)->err_work);
}

The nvme_wait_reset() shall wait until the nvme_tcp_setup_ctrl() finish 
its work and then the tcp error recovery work would be initiated. 

Thanks,
--Nilay