On 1/11/21 23:27, Christoph Hellwig wrote:
> The Command Set Identifier has no "NVM" in its name.
>
>
>> +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.
