On 1/11/21 23:27, Christoph Hellwig wrote:

The Command Set Identifier has no "NVM" in its name.

quoted

+static inline bool nvmet_cc_css_check(u8 cc_css)
+{
+	switch (cc_css <<= NVME_CC_CSS_SHIFT) {
+	case NVME_CC_CSS_NVM:
+		return true;
+	default:
+		return false;
+	}
+}

This hunk looks misplaced, it isn't very useful on its own, but
should go together with the multiple command set support.

We advertise the support for command sets supported in
nvmet_init_cap() -> ctrl->cap = (1ULL << 37). This results in
nvme_enable_ctrl() setting the ctrl->ctrl_config -> NVME_CC_CSS_NVM.
In current code in nvmet_start_ctrl() ->  nvmet_cc_css(ctrl->cc) != 0
checks if value is not = 0 but doesn't use the macro used by the host.
Above function does that also makes it helper that we use in the next
patch where cc_css value is != 0 but NVME_CC_CSS_CSI with
ctrl->cap set to 1ULL << 43.

With code flow in [1] above function is needed to make sure css value
matches the value set by the host using the same macro in
nvme_enable_ctrl() NVME_CC_CSS_NVM. Otherwise patch looks incomplete
and adding check for the CSS NVM with CSS_CSI looks mixing up things
to me.

Are you okay with that ?

[1]
nvme_enable_ctrl()
 ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config)
  nvmf_reg_write32()
   nvmet_parse_fabrics_cmd()
    nvmet_execute_prop_set()
     nvmet_update_ctrl()
      new cc != old cc == true -> nvmet_start_ctrl()
       nvmet_cc_css_check(ctrl->css)
        Check if host has set the for controller config NVME_CC_CSS_NVM
        as we are supporting default CSS_NVM which ctrl needs to set
        irrespective of other CC_CSS values.