Re: [PATCH 4/5] nvme: Add a new exported function nvme_ctrl_shutdown_start()

[Christoph Hellwig <hch@lst.de>]

Normally we'd just say add a new helper instead of exported function.

On Wed, Feb 07, 2024 at 01:40:43PM -0800, Jeremy Allison wrote:
> Sets the shutdown bit but doesn't wait for ready.
> Use from nvme_disable_ctrl(). Export nvme_wait_ready()
> so we can call it from drivers/nvme/host/pci.c.

The nvme_wait_ready export seems unrelated to the nvme_disable_ctrl
changes.  But looking at the next patch, it seems like a helper that
wraps nvme_wait_ready with the shutdown-specific flags would seem
useful over open coding them anyway.

>  
>  int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
>  {
>  	int ret;
>  
> -	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
> -	if (shutdown)
> -		ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
> -	else
> +	if (shutdown) {
> +		ret = nvme_ctrl_shutdown_start(ctrl);
> +	} else {
> +		ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
>  		ctrl->ctrl_config &= ~NVME_CC_ENABLE;
> +		ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);

Please avoid the overly long line here.

Looking at the result after the series, nvme_disable_ctrl now has two
entirely separate code flows for the shutdown vs !shutdown case.  To me
this suggested we should just split it into a nvme_disable_ctrl that does
the non-shutdown disable, and a nvme_shutdown_ctl that does
nvme_ctrl_shutdown_start + the shutdown version of nvme_wait_ready and
let the callers do the 'if (shutdown)' for the cases where it is needed.