All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Max Reitz <mreitz@redhat.com>, Keith Busch <kbusch@kernel.org>,
	Eric Auger <eric.auger@redhat.com>,
	Klaus Jensen <its@irrelevant.dk>
Subject: Re: [PATCH-for-6.0 v2 25/25] block/nvme: Simplify Completion Queue Command Identifier field use
Date: Fri, 30 Oct 2020 15:53:54 +0100	[thread overview]
Message-ID: <f46d3622-7fc4-7d98-c6c4-6a70d7fdb79a@redhat.com> (raw)
In-Reply-To: <20201030140002.GB330921@stefanha-x1.localdomain>

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
> 



  reply	other threads:[~2020-10-30 15:12 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29  9:32 [PATCH-for-5.2 v2 00/25] block/nvme: Fix Aarch64 or big-endian hosts Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 01/25] MAINTAINERS: Cover 'block/nvme.h' file Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 02/25] block/nvme: Use hex format to display offset in trace events Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 03/25] block/nvme: Report warning with warn_report() Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 04/25] block/nvme: Trace controller capabilities Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 05/25] block/nvme: Trace nvme_poll_queue() per queue Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 06/25] block/nvme: Improve nvme_free_req_queue_wait() trace information Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 07/25] block/nvme: Trace queue pair creation/deletion Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 08/25] block/nvme: Move definitions before structure declarations Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 09/25] block/nvme: Use unsigned integer for queue counter/size Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 10/25] block/nvme: Make nvme_identify() return boolean indicating error Philippe Mathieu-Daudé
2020-10-30 14:03   ` Stefan Hajnoczi
2020-10-29  9:32 ` [PATCH-for-5.2 v2 11/25] block/nvme: Make nvme_init_queue() " Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 12/25] block/nvme: Introduce Completion Queue definitions Philippe Mathieu-Daudé
2020-10-30 14:03   ` Stefan Hajnoczi
2020-10-30 14:52     ` Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 13/25] block/nvme: Use definitions instead of magic values in add_io_queue() Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 14/25] block/nvme: Correctly initialize Admin Queue Attributes Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 15/25] block/nvme: Simplify ADMIN queue access Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 16/25] block/nvme: Simplify nvme_cmd_sync() Philippe Mathieu-Daudé
2020-10-30 14:25   ` Stefan Hajnoczi
2020-10-29  9:32 ` [PATCH-for-5.2 v2 17/25] block/nvme: Set request_alignment at initialization Philippe Mathieu-Daudé
2020-10-29  9:32 ` [PATCH-for-5.2 v2 18/25] block/nvme: Correct minimum device page size Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 19/25] block/nvme: Change size and alignment of IDENTIFY response buffer Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 20/25] block/nvme: Change size and alignment of queue Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 21/25] block/nvme: Change size and alignment of prp_list_pages Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 22/25] block/nvme: Align iov's va and size on host page size Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 23/25] block/nvme: Fix use of write-only doorbells page on Aarch64 arch Philippe Mathieu-Daudé
2020-10-29  9:33 ` [PATCH-for-5.2 v2 24/25] block/nvme: Fix nvme_submit_command() on big-endian host Philippe Mathieu-Daudé
2020-10-30 13:57   ` Stefan Hajnoczi
2020-10-29  9:33 ` [PATCH-for-6.0 v2 25/25] block/nvme: Simplify Completion Queue Command Identifier field use Philippe Mathieu-Daudé
2020-10-30 14:00   ` Stefan Hajnoczi
2020-10-30 14:53     ` Philippe Mathieu-Daudé [this message]
2020-11-03 17:14 ` [PATCH-for-5.2 v2 00/25] block/nvme: Fix Aarch64 or big-endian hosts Stefan Hajnoczi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f46d3622-7fc4-7d98-c6c4-6a70d7fdb79a@redhat.com \
    --to=philmd@redhat.com \
    --cc=eric.auger@redhat.com \
    --cc=fam@euphon.net \
    --cc=its@irrelevant.dk \
    --cc=kbusch@kernel.org \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.