19.07.2023 10:36, Klaus Jensen wrote:
pu(req->cmd.dptr.prp2);
+ uint32_t v;
if (sq) {
+ v = cpu_to_le32(sq->tail);
- pci_dma_write(pci, sq->db_addr, &sq->tail, sizeof(sq->tail));
+ pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
This and similar cases hurts my eyes.
Why we pass address of v here, but use sizeof(sq->tail) ?
Yes, I know both in theory should be of the same size, but heck,
this is puzzling at best, and confusing in a regular case.
Dunno how it slipped in the review, it instantly catched my eye
in a row of applied patches..
Also, why v is computed a few lines before it is used, with
some expressions between the assignment and usage?
How about the following patch:
From: Michael Tokarev <m...@tls.msk.ru>
Date: Wed, 19 Jul 2023 23:10:53 +0300
Subject: [PATCH trivial] hw/nvme: fix sizeof() misuse and move endianness
conversions
closer to users
Signed-off-by: Michael Tokarev <m...@tls.msk.ru>
Fixes: ea3c76f1494d0
---
hw/nvme/ctrl.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dadc2dc7da..e33b28cf66 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6820,6 +6820,4 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
if (sq) {
- v = cpu_to_le32(sq->tail);
-
/*
* CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
@@ -6829,5 +6827,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
sq->db_addr = dbs_addr + (i << 3);
sq->ei_addr = eis_addr + (i << 3);
- pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+ v = cpu_to_le32(sq->tail);
+ pci_dma_write(pci, sq->db_addr, &v, sizeof(v));
if (n->params.ioeventfd && sq->sqid != 0) {
@@ -6839,10 +6838,9 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
if (cq) {
- v = cpu_to_le32(cq->head);
-
/* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
- pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+ v = cpu_to_le32(cq->head);
+ pci_dma_write(pci, cq->db_addr, &v, sizeof(v));
if (n->params.ioeventfd && cq->cqid != 0) {
@@ -7661,5 +7659,5 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
if (!qid && n->dbbuf_enabled) {
v = cpu_to_le32(cq->head);
- pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+ pci_dma_write(pci, cq->db_addr, &v, sizeof(v));
}
if (start_sqs) {
@@ -7721,6 +7719,4 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
sq->tail = new_tail;
if (!qid && n->dbbuf_enabled) {
- v = cpu_to_le32(sq->tail);
-
/*
* The spec states "the host shall also update the controller's
@@ -7736,5 +7732,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
* so we can't trust reading it for an appropriate sq tail.
*/
- pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+ v = cpu_to_le32(sq->tail);
+ pci_dma_write(pci, sq->db_addr, &v, sizeof(v));
}
