[PATCH 2/2] nvme: fix atomic write boundary validation

[Yi Zhang]

On Wed, Jun 11, 2025 at 1:56 PM Christoph Hellwig <hch at lst.de> wrote:
>
> Don't mix the namespace and controller values, and validate the
> per-controller limit when probing the controller.  This avoid spurious
> failures for controllers with namespaces that have different namespaces
> with different logical block sizes, or report the per-namespace values
> only for some namespaces.
>
> It also fixes a missing queue_limits_cancel_update in an error path by
> removing that error path.
>
> Fixes: 8695f060a029 ("nvme: all namespaces in a subsystem must adhere to a common atomic write size")
> Reported-by: Yi Zhang <yi.zhang at redhat.com>

Thanks for the fix:
Tested-by: Yi Zhang <yi.zhang at redhat.com>


> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/nvme/host/core.c | 34 ++++++++++++----------------------
>  drivers/nvme/host/nvme.h |  3 +--
>  2 files changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 520fb5f1e214..3da5ac71a9b0 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2041,17 +2041,7 @@ static u32 nvme_configure_atomic_write(struct nvme_ns *ns,
>                  * no clear language in the specification prohibiting different
>                  * values for different controllers in the subsystem.
>                  */
> -               atomic_bs = (1 + ns->ctrl->awupf) * bs;
> -       }
> -
> -       if (!ns->ctrl->subsys->atomic_bs) {
> -               ns->ctrl->subsys->atomic_bs = atomic_bs;
> -       } else if (ns->ctrl->subsys->atomic_bs != atomic_bs) {
> -               dev_err_ratelimited(ns->ctrl->device,
> -                       "%s: Inconsistent Atomic Write Size, Namespace will not be added: Subsystem=%d bytes, Controller/Namespace=%d bytes\n",
> -                       ns->disk ? ns->disk->disk_name : "?",
> -                       ns->ctrl->subsys->atomic_bs,
> -                       atomic_bs);
> +               atomic_bs = (1 + ns->ctrl->subsys->awupf) * bs;
>         }
>
>         lim->atomic_write_hw_max = atomic_bs;
> @@ -2386,16 +2376,6 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>         if (!nvme_update_disk_info(ns, id, &lim))
>                 capacity = 0;
>
> -       /*
> -        * Validate the max atomic write size fits within the subsystem's
> -        * atomic write capabilities.
> -        */
> -       if (lim.atomic_write_hw_max > ns->ctrl->subsys->atomic_bs) {
> -               blk_mq_unfreeze_queue(ns->disk->queue, memflags);
> -               ret = -ENXIO;
> -               goto out;
> -       }
> -
>         nvme_config_discard(ns, &lim);
>         if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
>             ns->head->ids.csi == NVME_CSI_ZNS)
> @@ -3555,7 +3535,18 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>                 ret = nvme_init_effects(ctrl, id);
>                 if (ret)
>                         goto out_free;
> +
> +               ctrl->subsys->awupf = le16_to_cpu(id->awupf);
>         }
> +
> +       if (le16_to_cpu(id->awupf) != ctrl->subsys->awupf) {
> +               dev_err_ratelimited(ctrl->device,
> +                       "inconsistent AWUPF, controller not added (%u/%u).\n",
> +                       le16_to_cpu(id->awupf), ctrl->subsys->awupf);
> +               ret = -EINVAL;
> +               goto out_free;
> +       }
> +
>         memcpy(ctrl->subsys->firmware_rev, id->fr,
>                sizeof(ctrl->subsys->firmware_rev));
>
> @@ -3651,7 +3642,6 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>                 dev_pm_qos_expose_latency_tolerance(ctrl->device);
>         else if (!ctrl->apst_enabled && prev_apst_enabled)
>                 dev_pm_qos_hide_latency_tolerance(ctrl->device);
> -       ctrl->awupf = le16_to_cpu(id->awupf);
>  out_free:
>         kfree(id);
>         return ret;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a468cdc5b5cb..7df2ea21851f 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -410,7 +410,6 @@ struct nvme_ctrl {
>
>         enum nvme_ctrl_type cntrltype;
>         enum nvme_dctype dctype;
> -       u16 awupf; /* 0's based value. */
>  };
>
>  static inline enum nvme_ctrl_state nvme_ctrl_state(struct nvme_ctrl *ctrl)
> @@ -443,11 +442,11 @@ struct nvme_subsystem {
>         u8                      cmic;
>         enum nvme_subsys_type   subtype;
>         u16                     vendor_id;
> +       u16                     awupf; /* 0's based value. */
>         struct ida              ns_ida;
>  #ifdef CONFIG_NVME_MULTIPATH
>         enum nvme_iopolicy      iopolicy;
>  #endif
> -       u32                     atomic_bs;
>  };
>
>  /*
> --
> 2.47.2
>
>


--
Best Regards,
  Yi Zhang