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 <[email protected]> >> Signed-off-by: Philippe Mathieu-Daudé <[email protected]> >> --- >> 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 >
