Skip to contentSkip to conversation

nvme-core: small cleanups

PATCH New Chaitanya Kulkarni 6 participants 9 messages Nov 29, 2018 – Dec 5, 2018 NVMe

lore
Share & tools
lore

Chaitanya Kulkarni

author
#0 lore raw

PATCH 1/1 nvme-core: small cleanups

This is a small cleanup, patch which removes the extra lines, inconsistent initialization of the struct nvme_command variables, extra initialization. We don't change any functionality in this patch.

Signed-off-by Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/nvme/host/core.c +12 -12
index 81d4514b9d06..0e845b702fe1 100644
@@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(nvme_delete_ctrl);
 
 int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl)
 {
-	int ret = 0;
+	int ret;
 
 	/*
 	 * Keep a reference until the work is flushed since ->delete_ctrl

@@ -918,9 +918,10 @@ EXPORT_SYMBOL_GPL(nvme_stop_keep_alive);
 
 static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 {
-	struct nvme_command c = { };
+	struct nvme_command c;
 	int error;
 
+	memset(&c, 0, sizeof(c));
 	/* gcc-4.4.4 (at least) has issues with initializers and anon unions */
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = NVME_ID_CNS_CTRL;

@@ -939,12 +940,13 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 		struct nvme_ns_ids *ids)
 {
-	struct nvme_command c = { };
+	struct nvme_command c;
 	int status;
 	void *data;
 	int pos;
 	int len;
 
+	memset(&c, 0, sizeof(c));
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.nsid = cpu_to_le32(nsid);
 	c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;

@@ -1010,8 +1012,9 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
 
 static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
 {
-	struct nvme_command c = { };
+	struct nvme_command c;
 
+	memset(&c, 0, sizeof(c));
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.cns = NVME_ID_CNS_NS_ACTIVE_LIST;
 	c.identify.nsid = cpu_to_le32(nsid);

@@ -1023,9 +1026,10 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 		unsigned nsid)
 {
 	struct nvme_id_ns *id;
-	struct nvme_command c = { };
+	struct nvme_command c};
 	int error;
 
+	memset(&c, 0, sizeof(c));
 	/* gcc-4.4.4 (at least) has issues with initializers and anon unions */
 	c.identify.opcode = nvme_admin_identify;
 	c.identify.nsid = cpu_to_le32(nsid);

@@ -1565,10 +1569,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	}
 
 	id = nvme_identify_ns(ctrl, ns->head->ns_id);
-	if (!id)
-		return -ENODEV;
-
-	if (id->ncap == 0) {
+	if (!id || id->ncap == 0) {
 		ret = -ENODEV;
 		goto out;
 	}

@@ -2051,7 +2052,6 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
 	case PM_QOS_LATENCY_ANY:
 		latency = U64_MAX;
 		break;
-
 	default:
 		latency = val;
 	}

@@ -2338,9 +2338,10 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
 		void *log, size_t size, u64 offset)
 {
-	struct nvme_command c = { };
+	struct nvme_command c;
 	unsigned long dwlen = size / 4 - 1;
 
+	memset(&c, 0, sizeof(c));
 	c.get_log_page.opcode = nvme_admin_get_log_page;
 	c.get_log_page.nsid = cpu_to_le32(nsid);
 	c.get_log_page.lid = log_page;

@@ -3415,7 +3416,6 @@ static void nvme_async_event_work(struct work_struct *work)
 
 static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
 {
-
 	u32 csts;
 
 	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
Signature · 1 line
2.17.0
#1 lore raw

PATCH 1/1 nvme-core: small cleanups

Hi Chaitanya,

drivers/nvme/host/core.c static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl, · line 1031

Quoted 8 lines · Chaitanya Kulkarni
drivers/nvme/host/core.c +1 -1
@@ -1023,9 +1026,10 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 		unsigned nsid)
 {
 	struct nvme_id_ns *id;
-	struct nvme_command c = { };
+	struct nvme_command c};
 	int error;

This hunk looks wrong to me.

Signature · 6 lines
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
author
#2 lore raw

PATCH 1/1 nvme-core: small cleanups

From: Johannes Thumshirn <[email protected]>
Sent: Monday, December 3, 2018 2:13 AM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] nvme-core: small cleanups
?

Hi Chaitanya,

Quoted 8 lines · Chaitanya Kulkarni
(quoted diff) +1 -1
@@ -1023,9 +1026,10 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 ??????????????? unsigned nsid)
 ? {
 ??????? struct nvme_id_ns *id;
-???? struct nvme_command c = { };
+???? struct nvme_command c};
 ??????? int error;

?

[CK] Yes it is, let me resend.

This hunk looks wrong to me.

Signature · 6 lines
Johannes Thumshirn??????????????????????????? SUSE Labs Filesystems
jthumshirn at suse.de??????????????????????????????? +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
#3 lore raw

PATCH 1/1 nvme-core: small cleanups

In reply to Chaitanya Kulkarni

drivers/nvme/host/core.c EXPORT_SYMBOL_GPL(nvme_stop_keep_alive); · line 924

Quoted 7 lines · Chaitanya Kulkarni

static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) {

drivers/nvme/host/core.c +2 -1
-	struct nvme_command c = { };
+	struct nvme_command c;
 	int error;
 
+	memset(&c, 0, sizeof(c));

Please drop this and all similar changes. I think the preferred style in the Linux kernel for structure initialization is to use "= { }" instead of memset().

Thanks,

Bart.

maintainer
#4 lore raw

PATCH 1/1 nvme-core: small cleanups

Quoted 11 lines

drivers/nvme/host/core.c EXPORT_SYMBOL_GPL(nvme_stop_keep_alive); · line 924

Quoted 8 lines
  static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
  {
drivers/nvme/host/core.c +2 -1
-	struct nvme_command c = { };
+	struct nvme_command c;
  	int error;
 
+	memset(&c, 0, sizeof(c));

Please drop this and all similar changes. I think the preferred style in the Linux kernel for structure initialization is to use "= { }" instead of memset().

Agree with Bart.

#5 lore raw

PATCH 1/1 nvme-core: small cleanups

In reply to Bart Van Assche
Quoted 11 lines · Bart Van Assche

drivers/nvme/host/core.c EXPORT_SYMBOL_GPL(nvme_stop_keep_alive); · line 924

Quoted 7 lines · Chaitanya Kulkarni

static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) {

drivers/nvme/host/core.c +2 -1
-	struct nvme_command c = { };
+	struct nvme_command c;
 	int error;
 
+	memset(&c, 0, sizeof(c));

Please drop this and all similar changes. I think the preferred style in the Linux kernel for structure initialization is to use "= { }" instead of memset().

I defintively prefer it. But I don't think changing things either way is really worth the effort anyway unless you change the surrounding code to start with.

#6 lore raw

PATCH 1/1 nvme-core: small cleanups

Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Bart Van Assche
Quoted 6 lines · Chaitanya Kulkarni

static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id) {

drivers/nvme/host/core.c +1 -1
-	struct nvme_command c = { };
+	struct nvme_command c;
 	int error;
Quoted 8 lines
Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	memset(&c, 0, sizeof(c));

Please drop this and all similar changes. I think the preferred style in the Linux kernel for structure initialization is to use "= { }" instead of memset().

I defintively prefer it. But I don't think changing things either way is really worth the effort anyway unless you change the surrounding code to start with.

The empty braces initialization is a gcc extension. For standard C portability, you may use "= { 0 }" ... just in case anyone cares. :)

#7 lore raw

PATCH 1/1 nvme-core: small cleanups

Quoted 14 lines · Keith Busch
Quoted 13 lines · Christoph Hellwig
Quoted 11 lines · Bart Van Assche

drivers/nvme/host/core.c EXPORT_SYMBOL_GPL(nvme_stop_keep_alive); · line 924

Quoted 7 lines · Chaitanya Kulkarni
  static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
  {
drivers/nvme/host/core.c +2 -1
-	struct nvme_command c = { };
+	struct nvme_command c;
  	int error;
 
+	memset(&c, 0, sizeof(c));

Please drop this and all similar changes. I think the preferred style in the Linux kernel for structure initialization is to use "= { }" instead of memset().

Quoted 6 lines
Quoted 3 lines

I defintively prefer it. But I don't think changing things either way is really worth the effort anyway unless you change the surrounding code to start with.

The empty braces initialization is a gcc extension. For standard C portability, you may use "= { 0 }" ... just in case anyone cares. :)

Let's change the C standard instead. Several other gcc extensions have already been incorporated in recent versions of the C standard.

Bart.

#8 lore raw

PATCH 1/1 nvme-core: small cleanups

In reply to Chaitanya Kulkarni

drivers/nvme/host/core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1572

Quoted 10 lines
drivers/nvme/host/core.c +1 -4
 	c.identify.nsid = cpu_to_le32(nsid);

@@ -1565,10 +1569,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	}
 
 	id = nvme_identify_ns(ctrl, ns->head->ns_id);
-	if (!id)
-		return -ENODEV;
-
-	if (id->ncap == 0) {
+	if (!id || id->ncap == 0) {

This now moves to kfree id which wasn't even allocated.

I think we can just drop the rest of the patch as well, it isn't really worth the churn.