Re: [PATCH V10 2/8] nvmet: add NVM Command Set Identifier support

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

On 3/9/21 03:37, Christoph Hellwig wrote:
>> +static void nvmet_set_csi_nvm_effects(struct nvme_effects_log *log)
>>  {
>>  	log->acs[nvme_admin_get_log_page]	= cpu_to_le32(1 << 0);
>>  	log->acs[nvme_admin_identify]		= cpu_to_le32(1 << 0);
>>  	log->acs[nvme_admin_abort_cmd]		= cpu_to_le32(1 << 0);
>> @@ -184,8 +177,45 @@ static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
>>  	log->iocs[nvme_cmd_flush]		= cpu_to_le32(1 << 0);
>>  	log->iocs[nvme_cmd_dsm]			= cpu_to_le32(1 << 0);
>>  	log->iocs[nvme_cmd_write_zeroes]	= cpu_to_le32(1 << 0);
>> +}
>>  
>> +static u16 nvmet_set_csi_zns_effects(struct nvme_effects_log *log)
>> +{
>> +	if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +		return NVME_SC_INVALID_IO_CMD_SET;
> I'd move this check into the caller.

Okay.

>> +	switch (req->cmd->get_log_page.csi) {
>> +	case NVME_CSI_NVM:
>> +		nvmet_set_csi_nvm_effects(log);
>> +		break;
>> +	case NVME_CSI_ZNS:
>> +		status = nvmet_set_csi_zns_effects(log);
>> +		break;
> ZNS also needs a call to nvmet_set_csi_nvm_effects as it also supports
> the the NVM commands.  But more importantly the ZNS case does not
> belong into this patch at all, but into one that actually adds ZNS
> support.  CSI support is just infrastructure not tied to a new command
> set.  Same for all the other places referencing ZNS in this patch.
>

Okay, I'll move all the ZNS related code to the ZNS backend patch.

>> +	default:
>> +		status = NVME_SC_INVALID_LOG_PAGE;
>> +		break;
>> +	}
>> +
>> +	if (status == NVME_SC_SUCCESS)
>> +		status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
> Also I'd use a goto for the failure case instead of the check here.
> the 

Okay.

>> +	switch (req->ns->csi) {
>> +	case NVME_CSI_NVM:
>> +		status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI, o);
>> +		break;
>> +	case NVME_CSI_ZNS:
>> +		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +			return NVME_SC_INVALID_IO_CMD_SET;
>> +
>> +		status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI, o);
>> +		break;
>> +	default:
>> +		status = NVME_SC_INVALID_IO_CMD_SET;
>> +	}
>> +
>> +	return status;
> Just return directly from inside the switch statement to simplify this
> a bit.
>

Make sense.


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