[PATCH] nvme: use ctrl state accessor

[Chaitanya Kulkarni]

On 1/24/24 09:28, Keith Busch wrote:
> From: Keith Busch <kbusch at kernel.org>
>
> The ctrl->state value is updated in another thread using WRITE_ONCE, so
> ensure all the readers use the appropriate accessor.
>
> Signed-off-by: Keith Busch <kbusch at kernel.org>
> ---
>   drivers/nvme/host/auth.c      |  2 +-
>   drivers/nvme/host/fabrics.h   |  8 +++++---
>   drivers/nvme/host/multipath.c | 15 ++++++++-------
>   drivers/nvme/host/sysfs.c     |  6 +++---
>   4 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 72c0525c75f50..2a12ee878783f 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -897,7 +897,7 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
>   	 * If the ctrl is no connected, bail as reconnect will handle
>   	 * authentication.
>   	 */
> -	if (ctrl->state != NVME_CTRL_LIVE)
> +	if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE)
>   		return;
>   
>   	/* Authenticate admin queue first */
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index fbaee5a7be196..06cc54851b1be 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -185,9 +185,11 @@ static inline bool
>   nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
>   			struct nvmf_ctrl_options *opts)
>   {
> -	if (ctrl->state == NVME_CTRL_DELETING ||
> -	    ctrl->state == NVME_CTRL_DELETING_NOIO ||
> -	    ctrl->state == NVME_CTRL_DEAD ||
> +	enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
> +
> +	if (state == NVME_CTRL_DELETING ||
> +	    state == NVME_CTRL_DELETING_NOIO ||
> +	    state == NVME_CTRL_DEAD ||
>   	    strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
>   	    strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
>   	    !uuid_equal(&opts->host->id, &ctrl->opts->host->id))

since original code is accessing ctrl->state each time instead of
separate variable, why not reading from it from nvme_ctrl_state() that
will have READ_ONCE() for each call ?

How about something like this following on the top of yours ?
it also removes the need for a variable on the stack :-

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 06cc54851b1b..78b4cef0cff2 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -185,11 +185,9 @@ static inline bool
  nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
                         struct nvmf_ctrl_options *opts)
  {
-       enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
-
-       if (state == NVME_CTRL_DELETING ||
-           state == NVME_CTRL_DELETING_NOIO ||
-           state == NVME_CTRL_DEAD ||
+       if (nvme_ctrl_state(ctrl) == NVME_CTRL_DELETING ||
+           nvme_ctrl_state(ctrl) == NVME_CTRL_DELETING_NOIO ||
+           nvme_ctrl_state(ctrl) == NVME_CTRL_DEAD ||
             strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
             strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
             !uuid_equal(&opts->host->id, &ctrl->opts->host->id))

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 2dd4137a08b28..5bc72653013c5 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -156,7 +156,7 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
>   		if (!ns->head->disk)
>   			continue;
>   		kblockd_schedule_work(&ns->head->requeue_work);
> -		if (ctrl->state == NVME_CTRL_LIVE)
> +		if (nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE)
>   			disk_uevent(ns->head->disk, KOBJ_CHANGE);
>   	}
>   	up_read(&ctrl->namespaces_rwsem);
> @@ -223,13 +223,14 @@ void nvme_mpath_revalidate_paths(struct nvme_ns *ns)
>   
>   static bool nvme_path_is_disabled(struct nvme_ns *ns)
>   {
> +	enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
> +

nvme_ctrl_state(ns->ctrl) is needed to compile the code, unless I'm 
totally wrong
here ...

-ck