Re: [PATCH-for-6.0 v2 25/25] block/nvme: Simplify Completion Queue Command Identifier field use

[Stefan Hajnoczi]

On Thu, Oct 29, 2020 at 10:33:06AM +0100, Philippe Mathieu-Daudé wrote:
> The "Completion Queue Entry: DW 2" describes it as:
> 
>   This identifier is assigned by host software when
>   the command is submitted to the Submission
> 
> As the is just an opaque cookie, it is pointless to byte-swap it.
> 
> Suggested-by: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index a06a188d530..e7723c42a6d 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -344,7 +344,7 @@ static inline int nvme_translate_error(const NvmeCqe *c)
>          trace_nvme_error(le32_to_cpu(c->result),
>                           le16_to_cpu(c->sq_head),
>                           le16_to_cpu(c->sq_id),
> -                         le16_to_cpu(c->cid),
> +                         c->cid,
>                           le16_to_cpu(status));
>      }
>      switch (status) {
> @@ -401,7 +401,7 @@ static bool nvme_process_completion(NVMeQueuePair *q)
>          if (!q->cq.head) {
>              q->cq_phase = !q->cq_phase;
>          }
> -        cid = le16_to_cpu(c->cid);
> +        cid = c->cid;
>          if (cid == 0 || cid > NVME_QUEUE_SIZE) {
>              warn_report("NVMe: Unexpected CID in completion queue: 
> %"PRIu32", "
>                          "queue size: %u", cid, NVME_QUEUE_SIZE);
> @@ -469,7 +469,7 @@ static void nvme_submit_command(NVMeQueuePair *q, 
> NVMeRequest *req,
>      assert(!req->cb);
>      req->cb = cb;
>      req->opaque = opaque;
> -    cmd->cid = cpu_to_le16(req->cid);
> +    cmd->cid = req->cid;
>  
>      trace_nvme_submit_command(q->s, q->index, req->cid);
>      nvme_trace_command(cmd);

Eliminating the byteswap is safe but this patch makes the code
confusing, as I mentioned previously.

Please use a comment or macro to mark this field native endian. It's not
obvious to the reader that we can skip the byteswap here.

Otherwise it will confuse readers into adding the byteswap back, not
using byteswapping in other places where it is needed, etc.

Thanks,
Stefan

signature.asc
Description: PGP signature