| Message ID | 20201029093306.1063879-26-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Fix Aarch64 or big-endian hosts | expand |
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
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);
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(-)