Skip to contentSkip to conversation

two minor nvme endianess patches

PATCH New Christoph Hellwig 3 participants 7 messages Aug 17, 2015 – Aug 19, 2015

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore
author maintainer
#1 lore raw

PATCH 1/2 nvme: remove spurious use of *_to_cpup helpers

Switch to the normal endianess helpers that take an integer instead of the pointer to it.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/block/nvme-core.c | 28 ++++++++++++++--------------
 drivers/block/nvme-scsi.c |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)
drivers/block/nvme-core.c +14 -14
index 666e994..8a27893 100644
@@ -281,13 +281,13 @@ static void special_completion(struct nvme_queue *nvmeq, void *ctx,
 	if (ctx == CMD_CTX_COMPLETED) {
 		dev_warn(nvmeq->q_dmadev,
 				"completed id %d twice on queue %d\n",
-				cqe->command_id, le16_to_cpup(&cqe->sq_id));
+				cqe->command_id, le16_to_cpu(cqe->sq_id));
 		return;
 	}
 	if (ctx == CMD_CTX_INVALID) {
 		dev_warn(nvmeq->q_dmadev,
 				"invalid id %d completed on queue %d\n",
-				cqe->command_id, le16_to_cpup(&cqe->sq_id));
+				cqe->command_id, le16_to_cpu(cqe->sq_id));
 		return;
 	}
 	dev_warn(nvmeq->q_dmadev, "Unknown special completion %p\n", ctx);

@@ -308,8 +308,8 @@ static void *cancel_cmd_info(struct nvme_cmd_info *cmd, nvme_completion_fn *fn)
 static void async_req_completion(struct nvme_queue *nvmeq, void *ctx,
 						struct nvme_completion *cqe)
 {
-	u32 result = le32_to_cpup(&cqe->result);
-	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	u32 result = le32_to_cpu(cqe->result);
+	u16 status = le16_to_cpu(cqe->status) >> 1;
 
 	if (status == NVME_SC_SUCCESS || status == NVME_SC_ABORT_REQ)
 		++nvmeq->dev->event_limit;

@@ -330,8 +330,8 @@ static void abort_completion(struct nvme_queue *nvmeq, void *ctx,
 {
 	struct request *req = ctx;
 
-	u16 status = le16_to_cpup(&cqe->status) >> 1;
-	u32 result = le32_to_cpup(&cqe->result);
+	u16 status = le16_to_cpu(cqe->status) >> 1;
+	u32 result = le32_to_cpu(cqe->result);
 
 	blk_mq_free_request(req);
 

@@ -343,8 +343,8 @@ static void async_completion(struct nvme_queue *nvmeq, void *ctx,
 						struct nvme_completion *cqe)
 {
 	struct async_cmd_info *cmdinfo = ctx;
-	cmdinfo->result = le32_to_cpup(&cqe->result);
-	cmdinfo->status = le16_to_cpup(&cqe->status) >> 1;
+	cmdinfo->result = le32_to_cpu(cqe->result);
+	cmdinfo->status = le16_to_cpu(cqe->status) >> 1;
 	queue_kthread_work(cmdinfo->worker, &cmdinfo->work);
 	blk_mq_free_request(cmdinfo->req);
 }

@@ -607,7 +607,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	struct request *req = iod_get_private(iod);
 	struct nvme_cmd_info *cmd_rq = blk_mq_rq_to_pdu(req);
 
-	u16 status = le16_to_cpup(&cqe->status) >> 1;
+	u16 status = le16_to_cpu(cqe->status) >> 1;
 
 	if (unlikely(status)) {
 		if (!(status & NVME_SC_DNR || blk_noretry_request(req))

@@ -632,7 +632,7 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
 	} else
 		req->errors = 0;
 	if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
-		u32 result = le32_to_cpup(&cqe->result);
+		u32 result = le32_to_cpu(cqe->result);
 		req->special = (void *)(uintptr_t)result;
 	}
 

@@ -2031,7 +2031,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	if (ns->ms && !blk_get_integrity(disk))
 		set_capacity(disk, 0);
 	else
-		set_capacity(disk, le64_to_cpup(&id->nsze) << (ns->lba_shift - 9));
+		set_capacity(disk, le64_to_cpu(id->nsze) << (ns->lba_shift - 9));
 
 	if (dev->oncs & NVME_CTRL_ONCS_DSM)
 		nvme_config_discard(ns);

@@ -2430,7 +2430,7 @@ static void nvme_dev_scan(struct work_struct *work)
 		return;
 	if (nvme_identify_ctrl(dev, &ctrl))
 		return;
-	nvme_scan_namespaces(dev, le32_to_cpup(&ctrl->nn));
+	nvme_scan_namespaces(dev, le32_to_cpu(ctrl->nn));
 	kfree(ctrl);
 }
 

@@ -2454,8 +2454,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
 		return -EIO;
 	}
 
-	nn = le32_to_cpup(&ctrl->nn);
-	dev->oncs = le16_to_cpup(&ctrl->oncs);
+	nn = le32_to_cpu(ctrl->nn);
+	dev->oncs = le16_to_cpu(ctrl->oncs);
 	dev->abort_limit = ctrl->acl + 1;
 	dev->vwc = ctrl->vwc;
 	memcpy(dev->serial, ctrl->sn, sizeof(ctrl->sn));
drivers/block/nvme-scsi.c +1 -1
index e5a63f0..800427b 100644
@@ -1178,7 +1178,7 @@ static void nvme_trans_fill_read_cap(u8 *response, struct nvme_id_ns *id_ns,
 
 	flbas = (id_ns->flbas) & 0x0F;
 	lba_length = (1 << (id_ns->lbaf[flbas].ds));
-	rlba = le64_to_cpup(&id_ns->nsze) - 1;
+	rlba = le64_to_cpu(id_ns->nsze) - 1;
 	(id_ns->dps) ? (prot_en = 0x01) : (prot_en = 0);
 
 	if (!cdb16) {
Signature · 1 line
1.9.1
#2 lore raw

PATCH 1/2 nvme: remove spurious use of *_to_cpup helpers

Quoted 2 lines · Christoph Hellwig

Switch to the normal endianess helpers that take an integer instead of the pointer to it.

Why? Some CPUs have a 'load-reversed-endian' instruction, which can be used in the _to_cpup() cases, but not in the _to_cpu() cases.

Quoted 2 lines
(quoted diff) +1 -1
-				cqe->command_id, le16_to_cpup(&cqe->sq_id));
+				cqe->command_id, le16_to_cpu(cqe->sq_id));
author maintainer
#3 lore raw

PATCH 1/2 nvme: remove spurious use of *_to_cpup helpers

Quoted 6 lines · Matthew Wilcox
Quoted 2 lines · Christoph Hellwig

Switch to the normal endianess helpers that take an integer instead of the pointer to it.

Why? Some CPUs have a 'load-reversed-endian' instruction, which can be used in the _to_cpup() cases, but not in the _to_cpu() cases.

They can and will be used in both cases.

Powerpc examples listings below, as you can see they produce identical code.

------------------ le32_to_cpu ------------------
test.o: file format elf32-powerpc

Disassembly of section .text:

00000000 <foor>:
};

int foor(struct foo *foo)

{
	return le32_to_cpu(foo->foo);
}

0: 7c 60 1c 2c lwbrx r3,0,r3
4: 4e 80 00 20 blr

------------------ le32_to_cpup ------------------
test.o: file format elf32-powerpc

Disassembly of section .text:

00000000 <foor>:
};

int foor(struct foo *foo)

{
	return le32_to_cpup(&foo->foo);
}

0: 7c 60 1c 2c lwbrx r3,0,r3
4: 4e 80 00 20 blr

#4 lore raw

PATCH 1/2 nvme: remove spurious use of *_to_cpup helpers

In reply to Matthew Wilcox
Quoted 9 lines · Matthew Wilcox
Quoted 2 lines · Christoph Hellwig

Switch to the normal endianess helpers that take an integer instead of the pointer to it.

Why? Some CPUs have a 'load-reversed-endian' instruction, which can be used in the _to_cpup() cases, but not in the _to_cpu() cases.

Review

Quoted 2 lines
(quoted diff) +1 -1
-				cqe->command_id, le16_to_cpup(&cqe->sq_id));
+				cqe->command_id, le16_to_cpu(cqe->sq_id));

All those nvme equipped sparc and s390's? :-)

Joke aside, do we really have that 'load-reversed-endian' addition to the endianness conversion API just because of two rather esoteric platforms? Seems silly.

Signature · 1 line
Jens Axboe
author maintainer
#5 lore raw

PATCH 1/2 nvme: remove spurious use of *_to_cpup helpers

Quoted 1 line · Jens Axboe

All those nvme equipped sparc and s390's? :-)

I'm pretty sure there are some power systems that have them.

Quoted 3 lines

Joke aside, do we really have that 'load-reversed-endian' addition to the endianness conversion API just because of two rather esoteric platforms? Seems silly.

As mentioned before BE platforms don't need it, compilers have handled it fine every since the invention of peephole optimization in the 60s.

The cpup helpers are more useful if you iterate over an array of descriptors:

fs/nfs/blocklayout/dev.c:       b->type = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:               b->simple.nr_sigs = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:                       b->simple.sigs[i].sig_len = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:               b->slice.volume = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:               b->concat.volumes_count = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:                       b->concat.volumes[i] = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:               b->stripe.volumes_count = be32_to_cpup(p++);
fs/nfs/blocklayout/dev.c:                       b->stripe.volumes[i] = be32_to_cpup(p++);

XDR decoding is full of them for example.

author maintainer
#6 lore raw

PATCH 2/2 nvme.h: add missing endianess annotations

In reply to Christoph Hellwig

Doesn't matter for the current NVMe driver, but we might as well get it in order for future software controller implementations.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 include/uapi/linux/nvme.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/uapi/linux/nvme.h +2 -2
index 732b32e..d1b644b 100644
@@ -50,8 +50,8 @@ struct nvme_id_ctrl {
 	__u8			ieee[3];
 	__u8			mic;
 	__u8			mdts;
-	__u16			cntlid;
-	__u32			ver;
+	__le16			cntlid;
+	__le32			ver;
 	__u8			rsvd84[172];
 	__le16			oacs;
 	__u8			acl;
Signature · 1 line
1.9.1