[PATCH] nvme-core: small cleanups

[PATCH] nvme-core: small cleanups

[Chaitanya Kulkarni]

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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 81d4514b9d06..0e845b702fe1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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))
-- 
2.17.0

[PATCH] nvme-core: small cleanups

[Johannes Thumshirn]

Hi Chaitanya,

On 29/11/2018 18:18, Chaitanya Kulkarni wrote:
> @@ -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.

-- 
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

[PATCH] nvme-core: small cleanups

[Chaitanya Kulkarni]

From: Johannes Thumshirn <jthumshirn@suse.de>
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,

On 29/11/2018 18:18, Chaitanya Kulkarni wrote:
> @@ -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.

-- 
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
    

[PATCH] nvme-core: small cleanups

[Bart Van Assche]

On Thu, 2018-11-29@09:18 -0800, Chaitanya Kulkarni wrote:
>  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));

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.

[PATCH] nvme-core: small cleanups

[Sagi Grimberg]

>>   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));
> 
> 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.

[PATCH] nvme-core: small cleanups

[Christoph Hellwig]

On Mon, Dec 03, 2018@02:42:56PM -0800, Bart Van Assche wrote:
> On Thu, 2018-11-29@09:18 -0800, Chaitanya Kulkarni wrote:
> >  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));
> 
> 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.

[PATCH] nvme-core: small cleanups

[Keith Busch]

On Tue, Dec 04, 2018@02:26:02PM -0800, Christoph Hellwig wrote:
> On Mon, Dec 03, 2018@02:42:56PM -0800, Bart Van Assche wrote:
> > On Thu, 2018-11-29@09:18 -0800, Chaitanya Kulkarni wrote:
> > >  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));
> > 
> > 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. :)

[PATCH] nvme-core: small cleanups

[Bart Van Assche]

On 12/4/18 2:37 PM, Keith Busch wrote:
> On Tue, Dec 04, 2018@02:26:02PM -0800, Christoph Hellwig wrote:
>> On Mon, Dec 03, 2018@02:42:56PM -0800, Bart Van Assche wrote:
>>> On Thu, 2018-11-29@09:18 -0800, Chaitanya Kulkarni wrote:
>>>>   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));
>>>
>>> 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. :)

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

Bart.

[PATCH] nvme-core: small cleanups

[Christoph Hellwig]

>  	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.