Re: [PATCH v2 3/8] nvme: add nvme opcodes, structures and helper functions

[Jens Axboe <axboe@kernel.dk>]

On 5/27/22 8:54 AM, Jens Axboe wrote:
> On 5/27/22 8:45 AM, Vincent Fu wrote:
>>> -----Original Message-----
>>> From: Ankit Kumar [mailto:ankit.kumar@samsung.com]
>>
>>> +
>>> +	if (io_u->ddir == DDIR_READ)
>>> +		cmd->opcode = nvme_cmd_read;
>>> +	if (io_u->ddir == DDIR_WRITE)
>>> +		cmd->opcode = nvme_cmd_write;
>>
>> Consider changing this to a switch statement and adding a default:
>> case in case someone tries to send an unsupported command.
>>
>> Since this is in the  fast path a switch statement would also reduce
>> the number of times ddir is checked.
> 
> A switch or if/else won't make any difference there, the compiler should
> generate the same code. But I do agree that it's nicer to use a switch
> so that unhandled cases are done properly.

How I'd do this:

make the prep handler return an error, and then do:

if (io_u->ddir == DDIR_READ)
	cmd->opcode = nvme_cmd_read;
else if (io_u->ddir == DDIR_WRITE)
	cmd->opcode = nvme_cmd_write;
else
	return appropriate error;

after you do the command memset, before all the other logic. Now make
sure that whoever calls the prep handler will check for an error and
error the io_u in that case.

-- 
Jens Axboe