[PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel]

Workarounds for gcc issues with initializers and anon unions was first
introduced in commit e44ac588cd61 ("drivers/block/nvme-core.c: fix build
with gcc-4.4.4").

The gcc bug in question has been fixed since gcc 4.6.0:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

The minimum gcc version for building the kernel has been 4.6.0 since
commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6"),
and has since been updated to gcc 4.8.0 in
commit 5429ef62bcf3 ("compiler/gcc: Raise minimum GCC version for
kernel builds to 4.8").

For that reason, it should now be safe to remove these workarounds
and make the code look like it did before
commit e44ac588cd61 ("drivers/block/nvme-core.c: fix build with gcc-4.4.4")
was introduced.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
If, for some reason, we want to allow builds with gcc < 4.6.0
even though the minimum gcc version is now 4.8.0,
there is another less intrusive workaround where you add an extra pair of
curly braces, see e.g. commit 6cc65be4f6f2 ("locking/qspinlock: Fix build
for anonymous union in older GCC compilers").

 drivers/nvme/host/core.c     | 59 ++++++++++++++++++------------------
 drivers/nvme/host/lightnvm.c | 32 +++++++++----------
 drivers/nvme/host/rdma.c     | 28 ++++++++---------
 3 files changed, 59 insertions(+), 60 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9491dbcfe81a..99059340d723 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1038,13 +1038,12 @@ static bool nvme_ctrl_limited_cns(struct nvme_ctrl *ctrl)
 
 static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 {
-	struct nvme_command c = { };
+	struct nvme_command c = {
+		.identify.opcode = nvme_admin_identify,
+		.identify.cns = NVME_ID_CNS_CTRL,
+	};
 	int error;
 
-	/* 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;
-
 	*id = kmalloc(sizeof(struct nvme_id_ctrl), GFP_KERNEL);
 	if (!*id)
 		return -ENOMEM;
@@ -1096,16 +1095,16 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
 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 = {
+		.identify.opcode = nvme_admin_identify,
+		.identify.nsid = cpu_to_le32(nsid),
+		.identify.cns = NVME_ID_CNS_NS_DESC_LIST,
+	};
 	int status;
 	void *data;
 	int pos;
 	int len;
 
-	c.identify.opcode = nvme_admin_identify;
-	c.identify.nsid = cpu_to_le32(nsid);
-	c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
-
 	data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -1143,11 +1142,12 @@ 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 = {
+		.identify.opcode = nvme_admin_identify,
+		.identify.cns = NVME_ID_CNS_NS_ACTIVE_LIST,
+		.identify.nsid = cpu_to_le32(nsid),
+	};
 
-	c.identify.opcode = nvme_admin_identify;
-	c.identify.cns = NVME_ID_CNS_NS_ACTIVE_LIST;
-	c.identify.nsid = cpu_to_le32(nsid);
 	return nvme_submit_sync_cmd(dev->admin_q, &c, ns_list,
 				    NVME_IDENTIFY_DATA_SIZE);
 }
@@ -1155,14 +1155,13 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
 static int nvme_identify_ns(struct nvme_ctrl *ctrl,
 		unsigned nsid, struct nvme_id_ns **id)
 {
-	struct nvme_command c = { };
+	struct nvme_command c = {
+		.identify.opcode = nvme_admin_identify,
+		.identify.nsid = cpu_to_le32(nsid),
+		.identify.cns = NVME_ID_CNS_NS,
+	};
 	int error;
 
-	/* 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);
-	c.identify.cns = NVME_ID_CNS_NS;
-
 	*id = kmalloc(sizeof(**id), GFP_KERNEL);
 	if (!*id)
 		return -ENOMEM;
@@ -2815,17 +2814,17 @@ 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 = { };
 	u32 dwlen = nvme_bytes_to_numd(size);
-
-	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;
-	c.get_log_page.lsp = lsp;
-	c.get_log_page.numdl = cpu_to_le16(dwlen & ((1 << 16) - 1));
-	c.get_log_page.numdu = cpu_to_le16(dwlen >> 16);
-	c.get_log_page.lpol = cpu_to_le32(lower_32_bits(offset));
-	c.get_log_page.lpou = cpu_to_le32(upper_32_bits(offset));
+	struct nvme_command c = {
+		.get_log_page.opcode = nvme_admin_get_log_page,
+		.get_log_page.nsid = cpu_to_le32(nsid),
+		.get_log_page.lid = log_page,
+		.get_log_page.lsp = lsp,
+		.get_log_page.numdl = cpu_to_le16(dwlen & ((1 << 16) - 1)),
+		.get_log_page.numdu = cpu_to_le16(dwlen >> 16),
+		.get_log_page.lpol = cpu_to_le32(lower_32_bits(offset)),
+		.get_log_page.lpou = cpu_to_le32(upper_32_bits(offset)),
+	};
 
 	return nvme_submit_sync_cmd(ctrl->admin_q, &c, log, size);
 }
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 69608755d415..7c44eca78f0d 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -432,12 +432,12 @@ static int nvme_nvm_identity(struct nvm_dev *nvmdev)
 {
 	struct nvme_ns *ns = nvmdev->q->queuedata;
 	struct nvme_nvm_id12 *id;
-	struct nvme_nvm_command c = {};
+	struct nvme_nvm_command c = {
+		.identity.opcode = nvme_nvm_admin_identity,
+		.identity.nsid = cpu_to_le32(ns->head->ns_id),
+	};
 	int ret;
 
-	c.identity.opcode = nvme_nvm_admin_identity;
-	c.identity.nsid = cpu_to_le32(ns->head->ns_id);
-
 	id = kmalloc(sizeof(struct nvme_nvm_id12), GFP_KERNEL);
 	if (!id)
 		return -ENOMEM;
@@ -479,16 +479,16 @@ static int nvme_nvm_get_bb_tbl(struct nvm_dev *nvmdev, struct ppa_addr ppa,
 	struct nvm_geo *geo = &nvmdev->geo;
 	struct nvme_ns *ns = q->queuedata;
 	struct nvme_ctrl *ctrl = ns->ctrl;
-	struct nvme_nvm_command c = {};
+	struct nvme_nvm_command c = {
+		.get_bb.opcode = nvme_nvm_admin_get_bb_tbl,
+		.get_bb.nsid = cpu_to_le32(ns->head->ns_id),
+		.get_bb.spba = cpu_to_le64(ppa.ppa),
+	};
 	struct nvme_nvm_bb_tbl *bb_tbl;
 	int nr_blks = geo->num_chk * geo->num_pln;
 	int tblsz = sizeof(struct nvme_nvm_bb_tbl) + nr_blks;
 	int ret = 0;
 
-	c.get_bb.opcode = nvme_nvm_admin_get_bb_tbl;
-	c.get_bb.nsid = cpu_to_le32(ns->head->ns_id);
-	c.get_bb.spba = cpu_to_le64(ppa.ppa);
-
 	bb_tbl = kzalloc(tblsz, GFP_KERNEL);
 	if (!bb_tbl)
 		return -ENOMEM;
@@ -532,15 +532,15 @@ static int nvme_nvm_set_bb_tbl(struct nvm_dev *nvmdev, struct ppa_addr *ppas,
 							int nr_ppas, int type)
 {
 	struct nvme_ns *ns = nvmdev->q->queuedata;
-	struct nvme_nvm_command c = {};
+	struct nvme_nvm_command c = {
+		.set_bb.opcode = nvme_nvm_admin_set_bb_tbl,
+		.set_bb.nsid = cpu_to_le32(ns->head->ns_id),
+		.set_bb.spba = cpu_to_le64(ppas->ppa),
+		.set_bb.nlb = cpu_to_le16(nr_ppas - 1),
+		.set_bb.value = type,
+	};
 	int ret = 0;
 
-	c.set_bb.opcode = nvme_nvm_admin_set_bb_tbl;
-	c.set_bb.nsid = cpu_to_le32(ns->head->ns_id);
-	c.set_bb.spba = cpu_to_le64(ppas->ppa);
-	c.set_bb.nlb = cpu_to_le16(nr_ppas - 1);
-	c.set_bb.value = type;
-
 	ret = nvme_submit_sync_cmd(ns->ctrl->admin_q, (struct nvme_command *)&c,
 								NULL, 0);
 	if (ret)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f8f856dc0c67..08c8728b3b29 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1804,22 +1804,22 @@ static int nvme_rdma_addr_resolved(struct nvme_rdma_queue *queue)
 static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 {
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
-	struct rdma_conn_param param = { };
-	struct nvme_rdma_cm_req priv = { };
+	struct rdma_conn_param param = {
+		.qp_num = queue->qp->qp_num,
+		.flow_control = 1,
+		.responder_resources = queue->device->dev->attrs.max_qp_rd_atom,
+		/* maximum retry count */
+		.retry_count = 7,
+		.rnr_retry_count = 7,
+		.private_data = &priv,
+		.private_data_len = sizeof(priv),
+	};
+	struct nvme_rdma_cm_req priv = {
+		.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
+		.qid = cpu_to_le16(nvme_rdma_queue_idx(queue)),
+	};
 	int ret;
 
-	param.qp_num = queue->qp->qp_num;
-	param.flow_control = 1;
-
-	param.responder_resources = queue->device->dev->attrs.max_qp_rd_atom;
-	/* maximum retry count */
-	param.retry_count = 7;
-	param.rnr_retry_count = 7;
-	param.private_data = &priv;
-	param.private_data_len = sizeof(priv);
-
-	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
-	priv.qid = cpu_to_le16(nvme_rdma_queue_idx(queue));
 	/*
 	 * set the admin queue depth to the minimum size
 	 * specified by the Fabrics standard.
-- 
2.26.2

[PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel]

Workarounds for gcc issues with initializers and anon unions was first
introduced in commit e44ac588cd61 ("drivers/block/nvme-core.c: fix build
with gcc-4.4.4").

The gcc bug in question has been fixed since gcc 4.6.0:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10676

The minimum gcc version for building the kernel has been 4.6.0 since
commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6"),
and has since been updated to gcc 4.8.0 in
commit 5429ef62bcf3 ("compiler/gcc: Raise minimum GCC version for
kernel builds to 4.8").

For that reason, it should now be safe to remove these workarounds
and make the code look like it did before
commit e44ac588cd61 ("drivers/block/nvme-core.c: fix build with gcc-4.4.4")
was introduced.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
If, for some reason, we want to allow builds with gcc < 4.6.0
even though the minimum gcc version is now 4.8.0,
there is another less intrusive workaround where you add an extra pair of
curly braces, see e.g. commit 6cc65be4f6f2 ("locking/qspinlock: Fix build
for anonymous union in older GCC compilers").

 drivers/nvme/target/rdma.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 6731e0349480..85c6ff0b0e44 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1535,19 +1535,20 @@ static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
 		struct nvmet_rdma_queue *queue,
 		struct rdma_conn_param *p)
 {
-	struct rdma_conn_param  param = { };
-	struct nvme_rdma_cm_rep priv = { };
+	struct rdma_conn_param  param = {
+		.rnr_retry_count = 7,
+		.flow_control = 1,
+		.initiator_depth = min_t(u8, p->initiator_depth,
+			queue->dev->device->attrs.max_qp_init_rd_atom),
+		.private_data = &priv,
+		.private_data_len = sizeof(priv),
+	};
+	struct nvme_rdma_cm_rep priv = {
+		.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
+		.crqsize = cpu_to_le16(queue->recv_queue_size),
+	};
 	int ret = -ENOMEM;
 
-	param.rnr_retry_count = 7;
-	param.flow_control = 1;
-	param.initiator_depth = min_t(u8, p->initiator_depth,
-		queue->dev->device->attrs.max_qp_init_rd_atom);
-	param.private_data = &priv;
-	param.private_data_len = sizeof(priv);
-	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
-	priv.crqsize = cpu_to_le16(queue->recv_queue_size);
-
 	ret = rdma_accept(cm_id, &param);
 	if (ret)
 		pr_err("rdma_accept failed (error code = %d)\n", ret);
-- 
2.26.2

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Chaitanya Kulkarni]

On 6/18/20 7:32 AM, Niklas Cassel wrote:
>   drivers/nvme/target/rdma.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 6731e0349480..85c6ff0b0e44 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1535,19 +1535,20 @@ static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
>   		struct nvmet_rdma_queue *queue,
>   		struct rdma_conn_param *p)
>   {
> -	struct rdma_conn_param  param = { };
> -	struct nvme_rdma_cm_rep priv = { };
> +	struct rdma_conn_param  param = {
> +		.rnr_retry_count = 7,
> +		.flow_control = 1,
> +		.initiator_depth = min_t(u8, p->initiator_depth,
> +			queue->dev->device->attrs.max_qp_init_rd_atom),
> +		.private_data = &priv,
> +		.private_data_len = sizeof(priv),
> +	};
> +	struct nvme_rdma_cm_rep priv = {
> +		.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
> +		.crqsize = cpu_to_le16(queue->recv_queue_size),
> +	};
>   	int ret = -ENOMEM;
>   
> -	param.rnr_retry_count = 7;
> -	param.flow_control = 1;
> -	param.initiator_depth = min_t(u8, p->initiator_depth,
> -		queue->dev->device->attrs.max_qp_init_rd_atom);
> -	param.private_data = &priv;
> -	param.private_data_len = sizeof(priv);
> -	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
> -	priv.crqsize = cpu_to_le16(queue->recv_queue_size);
> -
>   	ret = rdma_accept(cm_id, &param);
>   	if (ret)
>   		pr_err("rdma_accept failed (error code = %d)\n", ret);
> -- 2.26.2

What is the issue with existing code that we need this patch for ?

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel]

On Thu, Jun 18, 2020 at 03:23:21PM +0000, Chaitanya Kulkarni wrote:
> On 6/18/20 7:32 AM, Niklas Cassel wrote:
> >   drivers/nvme/target/rdma.c | 23 ++++++++++++-----------
> >   1 file changed, 12 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> > index 6731e0349480..85c6ff0b0e44 100644
> > --- a/drivers/nvme/target/rdma.c
> > +++ b/drivers/nvme/target/rdma.c
> > @@ -1535,19 +1535,20 @@ static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
> >   		struct nvmet_rdma_queue *queue,
> >   		struct rdma_conn_param *p)
> >   {
> > -	struct rdma_conn_param  param = { };
> > -	struct nvme_rdma_cm_rep priv = { };
> > +	struct rdma_conn_param  param = {
> > +		.rnr_retry_count = 7,
> > +		.flow_control = 1,
> > +		.initiator_depth = min_t(u8, p->initiator_depth,
> > +			queue->dev->device->attrs.max_qp_init_rd_atom),
> > +		.private_data = &priv,
> > +		.private_data_len = sizeof(priv),
> > +	};
> > +	struct nvme_rdma_cm_rep priv = {
> > +		.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
> > +		.crqsize = cpu_to_le16(queue->recv_queue_size),
> > +	};
> >   	int ret = -ENOMEM;
> >   
> > -	param.rnr_retry_count = 7;
> > -	param.flow_control = 1;
> > -	param.initiator_depth = min_t(u8, p->initiator_depth,
> > -		queue->dev->device->attrs.max_qp_init_rd_atom);
> > -	param.private_data = &priv;
> > -	param.private_data_len = sizeof(priv);
> > -	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
> > -	priv.crqsize = cpu_to_le16(queue->recv_queue_size);
> > -
> >   	ret = rdma_accept(cm_id, &param);
> >   	if (ret)
> >   		pr_err("rdma_accept failed (error code = %d)\n", ret);
> > -- 2.26.2
> 
> What is the issue with existing code that we need this patch for ?
> 

Hello Chaitanya,

This is just a cleanup patch, no functional change intended.

It simply performs the initialization at declaration time, which is how we
usually initialize a subset of all fields.

This is also how it was originally done, but this was changed to a
non-standard way in order to workaround a compiler bug.

Since the compiler bug is no longer relevant, we can go back to the
standard way of doing things.

Performing initialization in a uniform way makes it easier to read and
comprehend the code, especially for people unfamiliar with it, since
it follows the same pattern used in other places of the kernel.

Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
all fields will be zero initialized.. reading futher down in the function
you realize that this function actually does initialize the struct..
which causes a mental hiccup, so you need to do a mental pipeline flush
and go back and read the code from the beginning. This only happens with
patterns that deviate from the standard way of doing things.


Kind regards,
Niklas

Re: [PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[Daniel Wagner]

On Thu, Jun 18, 2020 at 04:32:40PM +0200, Niklas Cassel wrote:
> If, for some reason, we want to allow builds with gcc < 4.6.0
> even though the minimum gcc version is now 4.8.0,

Just one thing to watch out: the stable trees are still using
older version of gcc. Note sure how relevant this is though.

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Chaitanya Kulkarni]

I'm  not against the code cleanup and it always welcome.
Please also have a look at other comment.

>> What is the issue with existing code that we need this patch for ?
>>
> 
> Hello Chaitanya,
> 
> This is just a cleanup patch, no functional change intended.
> 
I can see that.
> It simply performs the initialization at declaration time, which is how we
> usually initialize a subset of all fields.
Absolutely not true in case nvme subsystem.
> 
> This is also how it was originally done, but this was changed to a
> non-standard way in order to workaround a compiler bug.
> 
and the existing code matches the pattern present in the repo no need to 
change if it is not causing any problem.
> Since the compiler bug is no longer relevant, we can go back to the
> standard way of doing things.
Is there any problem with the code now which mandates this change ? 
which I don't see any.
> 
> Performing initialization in a uniform way makes it easier to read and
> comprehend the code, especially for people unfamiliar with it, since
> it follows the same pattern used in other places of the kernel.
> 
This is absolutely not uniform way infact what you are proposing is true 
then we need to change all existing function which does not follow this 
pattern, have a look at the following list.

In NVMe subsystem case there are more than 10 functions which are on the 
similar line of this function and doesn't initialize the variable at the 
time declaration.

Please have a look the code :-
nvmf_reg_read32
nvmf_reg_read64
nvmf_reg_write32
nvmf_connect_admin_queue
nvmf_connect_io_queue
nvme_identify_ctrl
nvme_identify_ns_descs
nvme_identify_ns_list
nvme_identify_ns
nvme_features
nvme_get_log
nvme_toggle_streams
nvme_get_stream_params

Also here :-
nvme_user_cmd
nvme_user_cmd64

Last two are an exception of copy_from_user() call before initialization 
case, but we can do copy from user from caller and pass that as an 
argument if we really want to follow the declare-init pattern.

In several places we don't follow this pattern when function is compact 
and it looks ugly for larger structures such as this example. this is
exactly the reason {} used in nvme subsystem.

> Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
> all fields will be zero initialized.. reading futher down in the function

No this is standard style and used in nvme/host/core.c everywhere 
nothing wrong with this at all, please have a look at the author.

> you realize that this function actually does initialize the struct..
> which causes a mental hiccup, so you need to do a mental pipeline flush
> and go back and read the code from the beginning. This only happens with
> patterns that deviate from the standard way of doing things.

The function is small enough for such hiccup which follows the pattern 
which we have it in the tree there is nothing wrong.

> 
> Kind regards,
> Niklas
> 

Re: [PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel]

On Thu, Jun 18, 2020 at 07:11:24PM +0200, Daniel Wagner wrote:
> On Thu, Jun 18, 2020 at 04:32:40PM +0200, Niklas Cassel wrote:
> > If, for some reason, we want to allow builds with gcc < 4.6.0
> > even though the minimum gcc version is now 4.8.0,
> 
> Just one thing to watch out: the stable trees are still using
> older version of gcc. Note sure how relevant this is though.

While the AUTOSEL can be a bit annoying when autoselecting patches
to backport that you didn't intend, I think that it in most cases
backports fixes that has a Fixes-tag, which this doesn't.

Kind regards,
Niklas

Re: [PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7542 bytes --]

Hi Niklas,

I love your patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.8-rc1 next-20200618]
[cannot apply to hch-configfs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Niklas-Cassel/nvme-remove-workarounds-for-gcc-bug-wrt-unnamed-fields-in-initializers/20200618-223525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/m68k/include/asm/io_mm.h:25,
                    from arch/m68k/include/asm/io.h:8,
                    from include/linux/scatterlist.h:9,
                    from include/linux/dma-mapping.h:11,
                    from include/rdma/ib_verbs.h:44,
                    from include/rdma/mr_pool.h:8,
                    from drivers/nvme/host/rdma.c:10:
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsb':
   arch/m68k/include/asm/raw_io.h:83:7: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      83 |  ({u8 __w, __v = (b);  u32 _addr = ((u32) (addr)); \
         |       ^~~
   arch/m68k/include/asm/raw_io.h:430:3: note: in expansion of macro 'rom_out_8'
     430 |   rom_out_8(port, *buf++);
         |   ^~~~~~~~~
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw':
   arch/m68k/include/asm/raw_io.h:86:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      86 |  ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
         |        ^~~
   arch/m68k/include/asm/raw_io.h:448:3: note: in expansion of macro 'rom_out_be16'
     448 |   rom_out_be16(port, *buf++);
         |   ^~~~~~~~~~~~
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw_swapw':
   arch/m68k/include/asm/raw_io.h:90:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      90 |  ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
         |        ^~~
   arch/m68k/include/asm/raw_io.h:466:3: note: in expansion of macro 'rom_out_le16'
     466 |   rom_out_le16(port, *buf++);
         |   ^~~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/nvme/host/rdma.c:7:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from arch/m68k/include/asm/bug.h:32,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/m68k/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:51,
                    from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/nvme/host/rdma.c:7:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   arch/m68k/include/asm/page_mm.h:170:25: note: in expansion of macro 'virt_addr_valid'
     170 | #define pfn_valid(pfn)  virt_addr_valid(pfn_to_virt(pfn))
         |                         ^~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/nvme/host/rdma.c: In function 'nvme_rdma_route_resolved':
>> drivers/nvme/host/rdma.c:1814:20: error: 'priv' undeclared (first use in this function)
    1814 |   .private_data = &priv,
         |                    ^~~~
   drivers/nvme/host/rdma.c:1814:20: note: each undeclared identifier is reported only once for each function it appears in

vim +/priv +1814 drivers/nvme/host/rdma.c

  1803	
  1804	static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
  1805	{
  1806		struct nvme_rdma_ctrl *ctrl = queue->ctrl;
  1807		struct rdma_conn_param param = {
  1808			.qp_num = queue->qp->qp_num,
  1809			.flow_control = 1,
  1810			.responder_resources = queue->device->dev->attrs.max_qp_rd_atom,
  1811			/* maximum retry count */
  1812			.retry_count = 7,
  1813			.rnr_retry_count = 7,
> 1814			.private_data = &priv,
  1815			.private_data_len = sizeof(priv),
  1816		};
  1817		struct nvme_rdma_cm_req priv = {
  1818			.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
  1819			.qid = cpu_to_le16(nvme_rdma_queue_idx(queue)),
  1820		};
  1821		int ret;
  1822	
  1823		/*
  1824		 * set the admin queue depth to the minimum size
  1825		 * specified by the Fabrics standard.
  1826		 */
  1827		if (priv.qid == 0) {
  1828			priv.hrqsize = cpu_to_le16(NVME_AQ_DEPTH);
  1829			priv.hsqsize = cpu_to_le16(NVME_AQ_DEPTH - 1);
  1830		} else {
  1831			/*
  1832			 * current interpretation of the fabrics spec
  1833			 * is at minimum you make hrqsize sqsize+1, or a
  1834			 * 1's based representation of sqsize.
  1835			 */
  1836			priv.hrqsize = cpu_to_le16(queue->queue_size);
  1837			priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
  1838		}
  1839	
  1840		ret = rdma_connect(queue->cm_id, &param);
  1841		if (ret) {
  1842			dev_err(ctrl->ctrl.device,
  1843				"rdma_connect failed (%d).\n", ret);
  1844			goto out_destroy_queue_ib;
  1845		}
  1846	
  1847		return 0;
  1848	
  1849	out_destroy_queue_ib:
  1850		nvme_rdma_destroy_queue_ib(queue);
  1851		return ret;
  1852	}
  1853	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57206 bytes --]

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7171 bytes --]

Hi Niklas,

I love your patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.8-rc1 next-20200618]
[cannot apply to hch-configfs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Niklas-Cassel/nvme-remove-workarounds-for-gcc-bug-wrt-unnamed-fields-in-initializers/20200618-223525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/m68k/include/asm/io_mm.h:25,
                    from arch/m68k/include/asm/io.h:8,
                    from include/linux/scatterlist.h:9,
                    from include/linux/dma-mapping.h:11,
                    from include/linux/skbuff.h:31,
                    from include/net/net_namespace.h:39,
                    from include/linux/inet.h:42,
                    from drivers/nvme/target/rdma.c:17:
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsb':
   arch/m68k/include/asm/raw_io.h:83:7: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      83 |  ({u8 __w, __v = (b);  u32 _addr = ((u32) (addr)); \
         |       ^~~
   arch/m68k/include/asm/raw_io.h:430:3: note: in expansion of macro 'rom_out_8'
     430 |   rom_out_8(port, *buf++);
         |   ^~~~~~~~~
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw':
   arch/m68k/include/asm/raw_io.h:86:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      86 |  ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
         |        ^~~
   arch/m68k/include/asm/raw_io.h:448:3: note: in expansion of macro 'rom_out_be16'
     448 |   rom_out_be16(port, *buf++);
         |   ^~~~~~~~~~~~
   arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw_swapw':
   arch/m68k/include/asm/raw_io.h:90:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
      90 |  ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
         |        ^~~
   arch/m68k/include/asm/raw_io.h:466:3: note: in expansion of macro 'rom_out_le16'
     466 |   rom_out_le16(port, *buf++);
         |   ^~~~~~~~~~~~
   In file included from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:7,
                    from drivers/nvme/target/rdma.c:7:
   include/linux/scatterlist.h: In function 'sg_set_buf':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
      78 | # define unlikely(x) __builtin_expect(!!(x), 0)
         |                                          ^
   include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |  ^~~~~~
   include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
     143 |  BUG_ON(!virt_addr_valid(buf));
         |          ^~~~~~~~~~~~~~~
   In file included from arch/m68k/include/asm/bug.h:32,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/m68k/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from arch/m68k/include/asm/irqflags.h:6,
                    from include/linux/irqflags.h:16,
                    from arch/m68k/include/asm/atomic.h:6,
                    from include/linux/atomic.h:7,
                    from drivers/nvme/target/rdma.c:7:
   include/linux/dma-mapping.h: In function 'dma_map_resource':
   arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
     169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
         |                                                 ^~
   include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
     144 |  int __ret_warn_once = !!(condition);   \
         |                           ^~~~~~~~~
   arch/m68k/include/asm/page_mm.h:170:25: note: in expansion of macro 'virt_addr_valid'
     170 | #define pfn_valid(pfn)  virt_addr_valid(pfn_to_virt(pfn))
         |                         ^~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
     352 |  if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
         |                   ^~~~~~~~~
   drivers/nvme/target/rdma.c: In function 'nvmet_rdma_cm_accept':
>> drivers/nvme/target/rdma.c:1543:20: error: 'priv' undeclared (first use in this function)
    1543 |   .private_data = &priv,
         |                    ^~~~
   drivers/nvme/target/rdma.c:1543:20: note: each undeclared identifier is reported only once for each function it appears in
   drivers/nvme/target/rdma.c:1546:26: warning: unused variable 'priv' [-Wunused-variable]
    1546 |  struct nvme_rdma_cm_rep priv = {
         |                          ^~~~

vim +/priv +1543 drivers/nvme/target/rdma.c

  1533	
  1534	static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
  1535			struct nvmet_rdma_queue *queue,
  1536			struct rdma_conn_param *p)
  1537	{
  1538		struct rdma_conn_param  param = {
  1539			.rnr_retry_count = 7,
  1540			.flow_control = 1,
  1541			.initiator_depth = min_t(u8, p->initiator_depth,
  1542				queue->dev->device->attrs.max_qp_init_rd_atom),
> 1543			.private_data = &priv,
  1544			.private_data_len = sizeof(priv),
  1545		};
  1546		struct nvme_rdma_cm_rep priv = {
  1547			.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
  1548			.crqsize = cpu_to_le16(queue->recv_queue_size),
  1549		};
  1550		int ret = -ENOMEM;
  1551	
  1552		ret = rdma_accept(cm_id, &param);
  1553		if (ret)
  1554			pr_err("rdma_accept failed (error code = %d)\n", ret);
  1555	
  1556		return ret;
  1557	}
  1558	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57206 bytes --]

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[Niklas Cassel]

On Thu, Jun 18, 2020 at 05:29:00PM +0000, Chaitanya Kulkarni wrote:
> I'm  not against the code cleanup and it always welcome.
> Please also have a look at other comment.
> 
> >> What is the issue with existing code that we need this patch for ?
> >>
> > 
> > Hello Chaitanya,
> > 
> > This is just a cleanup patch, no functional change intended.
> > 
> I can see that.
> > It simply performs the initialization at declaration time, which is how we
> > usually initialize a subset of all fields.
> Absolutely not true in case nvme subsystem.
> > 
> > This is also how it was originally done, but this was changed to a
> > non-standard way in order to workaround a compiler bug.
> > 
> and the existing code matches the pattern present in the repo no need to 
> change if it is not causing any problem.
> > Since the compiler bug is no longer relevant, we can go back to the
> > standard way of doing things.
> Is there any problem with the code now which mandates this change ? 
> which I don't see any.
> > 
> > Performing initialization in a uniform way makes it easier to read and
> > comprehend the code, especially for people unfamiliar with it, since
> > it follows the same pattern used in other places of the kernel.
> > 
> This is absolutely not uniform way infact what you are proposing is true 
> then we need to change all existing function which does not follow this 
> pattern, have a look at the following list.
> 
> In NVMe subsystem case there are more than 10 functions which are on the 
> similar line of this function and doesn't initialize the variable at the 
> time declaration.
> 
> Please have a look the code :-

> nvmf_reg_read32
> nvmf_reg_read64
> nvmf_reg_write32
> nvmf_connect_admin_queue
> nvmf_connect_io_queue
> nvme_features
> nvme_toggle_streams
> nvme_get_stream_params
> nvme_user_cmd
> nvme_user_cmd64

These do not use an initializer at all, these use memset.
So at declaration time, these are not initialized at all.

Basically like:

int a;
a = 2;

I don't have any problem with the memset code pattern per se,
it is very common. Although it is weird that the nvme code
sometimes declares a "struct nvme_command c" on the stack,
and then memsets it, and sometimes uses an initializer.

Perhaps we should create a patch to unify this.
IMHO, using an initializer is more clear.
memset has to be used if the function needs to reset an
existing struct, but in none of the functions above are
we given a nvme_command that we need to reset. In each case
we declare a new nvme_command on the stack (so an initializer
makes more sense).

> nvme_identify_ctrl
> nvme_identify_ns_descs
> nvme_identify_ns_list
> nvme_identify_ns
> nvme_get_log

These used an initializer, and then also later did explict assignments,
e.g.:
struct nvme_command c = { 0 };
...
c.identify.cns = NVME_ID_CNS_CTRL;

which is basically the same as doing:

int a = 0;
a = 2;


However, I have fixed these functions in patch 1/2 in this series,
so that the values are set in the initializer, and then there is
not need for any further assignments.

Basically:
int a = 2;

> 
> Last two are an exception of copy_from_user() call before initialization 
> case, but we can do copy from user from caller and pass that as an 
> argument if we really want to follow the declare-init pattern.
> 
> In several places we don't follow this pattern when function is compact 
> and it looks ugly for larger structures such as this example. this is
> exactly the reason {} used in nvme subsystem.

If this pattern of using an initializer and then doing an assignment
is the desired pattern, then you should at least remove the
/* gcc-4.4.4 (at least) has issues with initializers and anon unions */
comments from drivers/nvme/host/core.c, because then that comment is
not true, because then you want this design pattern because you like it,
not because of a gcc bug.

> 
> > Just reading e.g. struct rdma_conn_param  param = { }; one assumes that
> > all fields will be zero initialized.. reading futher down in the function
> 
> No this is standard style and used in nvme/host/core.c everywhere 
> nothing wrong with this at all, please have a look at the author.

I think that the standard style in the nvme code is to assign the
values inside the initializer when assigning values to a struct
(except when using memset), see e.g.:

nvmet_tcp_try_recv_ddgst
nvmet_tcp_try_recv_pdu
nvmet_try_send_ddgst
nvmet_rdma_init_srq
nvmet_fc_add_port
nvme_fc_parse_traddr
nvmet_copy_ns_identifier
nvme_tcp_try_send_ddgst
nvme_rdma_inv_rkey
nvme_setup_irqs
nvme_fc_create_ctrl
nvme_fc_parse_traddr
nvme_fc_signal_discovery_scan
nvme_aen_uevent

And then there are the many many
static const struct's that are assigned values using initializers.
(Which I didn't count.)


I made a small mistake in v1, so I'll send out a v2,
feel free to continue the discussion there :)


Kind regards,
Niklas

Re: [PATCH 1/2] nvme: remove workarounds for gcc bug wrt unnamed fields in initializers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]

Hi Niklas,

I love your patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Niklas-Cassel/nvme-remove-workarounds-for-gcc-bug-wrt-unnamed-fields-in-initializers/20200618-223525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/nvme/host/rdma.c:1814:20: error: use of undeclared identifier 'priv'
                   .private_data = &priv,
                                    ^
   drivers/nvme/host/rdma.c:1815:30: error: use of undeclared identifier 'priv'
                   .private_data_len = sizeof(priv),
                                              ^
   2 errors generated.

vim +/priv +1814 drivers/nvme/host/rdma.c

  1803	
  1804	static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
  1805	{
  1806		struct nvme_rdma_ctrl *ctrl = queue->ctrl;
  1807		struct rdma_conn_param param = {
  1808			.qp_num = queue->qp->qp_num,
  1809			.flow_control = 1,
  1810			.responder_resources = queue->device->dev->attrs.max_qp_rd_atom,
  1811			/* maximum retry count */
  1812			.retry_count = 7,
  1813			.rnr_retry_count = 7,
> 1814			.private_data = &priv,
  1815			.private_data_len = sizeof(priv),
  1816		};
  1817		struct nvme_rdma_cm_req priv = {
  1818			.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
  1819			.qid = cpu_to_le16(nvme_rdma_queue_idx(queue)),
  1820		};
  1821		int ret;
  1822	
  1823		/*
  1824		 * set the admin queue depth to the minimum size
  1825		 * specified by the Fabrics standard.
  1826		 */
  1827		if (priv.qid == 0) {
  1828			priv.hrqsize = cpu_to_le16(NVME_AQ_DEPTH);
  1829			priv.hsqsize = cpu_to_le16(NVME_AQ_DEPTH - 1);
  1830		} else {
  1831			/*
  1832			 * current interpretation of the fabrics spec
  1833			 * is at minimum you make hrqsize sqsize+1, or a
  1834			 * 1's based representation of sqsize.
  1835			 */
  1836			priv.hrqsize = cpu_to_le16(queue->queue_size);
  1837			priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
  1838		}
  1839	
  1840		ret = rdma_connect(queue->cm_id, &param);
  1841		if (ret) {
  1842			dev_err(ctrl->ctrl.device,
  1843				"rdma_connect failed (%d).\n", ret);
  1844			goto out_destroy_queue_ib;
  1845		}
  1846	
  1847		return 0;
  1848	
  1849	out_destroy_queue_ib:
  1850		nvme_rdma_destroy_queue_ib(queue);
  1851		return ret;
  1852	}
  1853	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75296 bytes --]

Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed fields in initializers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

Hi Niklas,

I love your patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Niklas-Cassel/nvme-remove-workarounds-for-gcc-bug-wrt-unnamed-fields-in-initializers/20200618-223525
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/nvme/target/rdma.c:1543:20: error: use of undeclared identifier 'priv'
                   .private_data = &priv,
                                    ^
   drivers/nvme/target/rdma.c:1544:30: error: use of undeclared identifier 'priv'
                   .private_data_len = sizeof(priv),
                                              ^
   2 errors generated.

vim +/priv +1543 drivers/nvme/target/rdma.c

  1533	
  1534	static int nvmet_rdma_cm_accept(struct rdma_cm_id *cm_id,
  1535			struct nvmet_rdma_queue *queue,
  1536			struct rdma_conn_param *p)
  1537	{
  1538		struct rdma_conn_param  param = {
  1539			.rnr_retry_count = 7,
  1540			.flow_control = 1,
  1541			.initiator_depth = min_t(u8, p->initiator_depth,
  1542				queue->dev->device->attrs.max_qp_init_rd_atom),
> 1543			.private_data = &priv,
  1544			.private_data_len = sizeof(priv),
  1545		};
  1546		struct nvme_rdma_cm_rep priv = {
  1547			.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
  1548			.crqsize = cpu_to_le16(queue->recv_queue_size),
  1549		};
  1550		int ret = -ENOMEM;
  1551	
  1552		ret = rdma_accept(cm_id, &param);
  1553		if (ret)
  1554			pr_err("rdma_accept failed (error code = %d)\n", ret);
  1555	
  1556		return ret;
  1557	}
  1558	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75296 bytes --]