Re: [PATCHv2] nvme: ensure disabling pairs with unquiesce

[Christoph Hellwig <hch@lst.de>]

On Wed, Jul 12, 2023 at 08:40:04AM -0700, Keith Busch wrote:
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1298,9 +1298,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
>  	 */
>  	if (nvme_should_reset(dev, csts)) {
>  		nvme_warn_reset(dev, csts);
> -		nvme_dev_disable(dev, false);
> -		nvme_reset_ctrl(&dev->ctrl);
> -		return BLK_EH_DONE;
> +		goto disable;
>  	}
>  
>  	/*
> @@ -1351,10 +1349,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
>  			 "I/O %d QID %d timeout, reset controller\n",
>  			 req->tag, nvmeq->qid);
>  		nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> -		nvme_dev_disable(dev, false);
> -		nvme_reset_ctrl(&dev->ctrl);
> -
> -		return BLK_EH_DONE;
> +		goto disable;
>  	}
>  
>  	if (atomic_dec_return(&dev->ctrl.abort_limit) < 0) {
> @@ -1391,6 +1386,15 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req)
>  	 * as the device then is in a faulty state.
>  	 */
>  	return BLK_EH_RESET_TIMER;
> +
> +disable:
> +	if (!nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING))
> +		return BLK_EH_DONE;
> +
> +	nvme_dev_disable(dev, false);
> +	if (nvme_try_sched_reset(&dev->ctrl))
> +		nvme_unquiesce_io_queues(&dev->ctrl);
> +	return BLK_EH_DONE;

Just curious, do you remember why we do an explicitnvme_dev_disable
here instead of relying on the one at the beginning of
nvme_dev_disable?  This isn't new, and the patch should not be blocked
on it, but it looks very odd to me an we'll need to either document
it or kill it eventually..

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>