[PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Keith Busch]

On Fri, Apr 11, 2025 at 09:32:53AM +0900, Damien Le Moal wrote:
> On 4/10/25 17:34, Keith Busch wrote:
> > On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
> >> @@ -1895,6 +1912,11 @@ static void nvmet_pci_epf_disable_ctrl(struct nvmet_pci_epf_ctrl *ctrl)
> >>  	nvmet_pci_epf_delete_cq(ctrl->tctrl, 0);
> >>  
> >>  	ctrl->csts &= ~NVME_CSTS_RDY;
> >> +	if (shutdown) {
> >> +		ctrl->csts |= NVME_CSTS_SHST_CMPLT;
> >> +		ctrl->cc &= ~NVME_CC_ENABLE;
> >> +		nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CC, ctrl->cc);
> >> +	}
> >>  }
> > 
> > I think this is probably okay, but I don't know if it's necessary to be
> > messing with CC.EN that the host didn't request. The qemu emulated nvme
> > doesn't do this, at least. But it looks like that would all work out in
> > the end anyway, so again, I think it's fine.
> 
> Maybe qemu clears/resets all the registers of the BAR when it does a shutdown ?
> I did not check the code. But here, if we do not clear EN, *and* the host does
> not first disable the controller before trying to re-enable it (like the linux
> host pci driver does), after the shutdown, we fail to detect that the host set
> EN again since we can only detect that by polling and comparing to the previous
> value.

The spec has some text explaining the host needs to handle a shutdown
controller with CC.EN set in section 3.6.1:

  To start executing commands on the controller after that controller
  reports controller shutdown processing complete (...) utilizing the
  CC.EN bit:

    o if the CC.EN bit is set to `1´, then a Controller Level Reset is
      required to clear the CC.EN bit to `0´ on that controller and the
      CC.EN bit is subsequently required to be set to `1´ as part of the
      initialization sequence 


So I guess some hosts just aren't handling this? No biggie, linux will
react accordingly either way.

I didn't find anything that requires a shutdown controller retain CC.EN,
so clearing it on the condition is probably fine. I never really fully
understood why a disabled vs shutdown-ready controller needed such
distinctions anyway.