Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Sagi Grimberg <sagi@grimberg.me>]

>>> So if we change this we break interaction with all old Linux (and maybe
>>> other?) targets.
>>
>> Well yes.. Do you think that we should do a retry in case it fails for
>> backward compatibility?
> 
> I think we have to.

The annoying thing here is that this issue will manifest in the host
waiting on nvme_wait_ready for a long 7.5 seconds to understand that
its maybe incompatible and re-attempt (nvme_enable_ctrl writes to cap
and then polls CSTS)...

What I have now looks like:
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 02579f4f776c..ee65c89a991c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2457,8 +2457,14 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
  }
  EXPORT_SYMBOL_GPL(nvme_disable_ctrl);

+static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
+{
+       return ctrl->opts && ctrl->opts->discovery_nqn;
+}
+
  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
  {
+       bool compat_disc_ctrl_config = false;
         unsigned dev_page_min;
         int ret;

@@ -2482,13 +2488,33 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
                 ctrl->ctrl_config = NVME_CC_CSS_NVM;
         ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << 
NVME_CC_MPS_SHIFT;
         ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
-       ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
+again:
+       if (!nvme_discovery_ctrl(ctrl) || compat_disc_ctrl_config)
+               ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
         ctrl->ctrl_config |= NVME_CC_ENABLE;

         ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
         if (ret)
                 return ret;
-       return nvme_wait_ready(ctrl, ctrl->cap, true);
+
+       ret = nvme_wait_ready(ctrl, ctrl->cap, true);
+       if (ret) {
+               if (!compat_disc_ctrl_config) {
+                       pr_err("retrying...\n");
+                       /*
+                        * Backward compatibility work-around: some targets
+                        * (i.e. older Linux targets) may incorrectly verify
+                        * iosqes,iocqes are non-zero for discovery
+                        * controllers. So in order not to break them, we
+                        * attempt once again with the incorrect settings.
+                        */
+                       if (nvme_disable_ctrl(ctrl))
+                               return ret;
+                       compat_disc_ctrl_config = true;
+                       goto again;
+               }
+       }
+       return ret;
  }
  EXPORT_SYMBOL_GPL(nvme_enable_ctrl);

@@ -2902,11 +2928,6 @@ static const struct attribute_group 
*nvme_subsys_attrs_groups[] = {
         NULL,
  };

-static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
-{
-       return ctrl->opts && ctrl->opts->discovery_nqn;
-}
-
  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
                 struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
  {
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme