diff mbox series

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

Message ID 20201029093306.1063879-26-philmd@redhat.com
State New
Headers show
Series block/nvme: Fix Aarch64 or big-endian hosts | expand

Commit Message

Philippe Mathieu-Daudé Oct. 29, 2020, 9:33 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi Oct. 30, 2020, 2 p.m. UTC | #1 
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
Philippe Mathieu-Daudé Oct. 30, 2020, 2:53 p.m. UTC | #2 
On 10/30/20 3:00 PM, Stefan Hajnoczi wrote:
> 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.

OK. (This patch is for 6.0 anyway, I included because it was
following the previous patch in its previous version).

> 
> Thanks,
> Stefan
>
diff mbox series

Patch

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);