Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: zyBnpfmEbOaEJ5Fddq16jsRK5qrz2Vjw3COJHzsJED5qp/4o/9FPyNpJYy/Jo59qHXo21FhICR
 oxR/DhCh4vkZ/3zJiOVimobY0R5kviw2uV99WU8KRQzejSlLLDJXKmhrENmGjceGoNiURX3paE
 CuKuSus5c7m2ECxGNNmOypvzE+tmRpQg7f5m5rIXqB4mnb8G0/YaP2jvx+/tKdVsAutqyNLDis
 JWypwNNYMLC/l6dfVDKhv+eNW/MuYiASxwqpcFIhNtLHrFrSgT6udTrZLyHFeYA5nTE262B85A
 Isg=
From:   Niklas Cassel <Niklas.Cassel@wdc.com>
To:     Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
CC:     Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed
 fields in initializers
Thread-Topic: [PATCH 2/2] nvmet: remove workarounds for gcc bug wrt unnamed
 fields in initializers
Thread-Index: AQHWRX1ZbrDHfOtockWlJJLIW+DNgajejEuA
Date:   Thu, 18 Jun 2020 16:15:12 +0000
Message-ID: <20200618161509.GA1059668@localhost.localdomain>
References: <20200618143241.1056800-1-niklas.cassel@wdc.com>
 <20200618143241.1056800-2-niklas.cassel@wdc.com>
 <BYAPR04MB49657026BADC613CA83CB896869B0@BYAPR04MB4965.namprd04.prod.outlook.com>
In-Reply-To: <BYAPR04MB49657026BADC613CA83CB896869B0@BYAPR04MB4965.namprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D101818765E6DA4EBB16014F1B03607C@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b289574f-8558-4e43-dec1-08d813a2c7bd
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jun 2020 16:15:12.8910
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8YeCgsDZUJgTecSjR9SLjMy4aM2bG5cF5T5KUITCv0hCj6S9+E9sxH7oT6/9nKaBO3TLe4sTMrpiR8T/tw9YIg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5941
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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(-)
> >=20
> > 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 =3D { };
> > -	struct nvme_rdma_cm_rep priv =3D { };
> > +	struct rdma_conn_param  param =3D {
> > +		.rnr_retry_count =3D 7,
> > +		.flow_control =3D 1,
> > +		.initiator_depth =3D min_t(u8, p->initiator_depth,
> > +			queue->dev->device->attrs.max_qp_init_rd_atom),
> > +		.private_data =3D &priv,
> > +		.private_data_len =3D sizeof(priv),
> > +	};
> > +	struct nvme_rdma_cm_rep priv =3D {
> > +		.recfmt =3D cpu_to_le16(NVME_RDMA_CM_FMT_1_0),
> > +		.crqsize =3D cpu_to_le16(queue->recv_queue_size),
> > +	};
> >   	int ret =3D -ENOMEM;
> >  =20
> > -	param.rnr_retry_count =3D 7;
> > -	param.flow_control =3D 1;
> > -	param.initiator_depth =3D min_t(u8, p->initiator_depth,
> > -		queue->dev->device->attrs.max_qp_init_rd_atom);
> > -	param.private_data =3D &priv;
> > -	param.private_data_len =3D sizeof(priv);
> > -	priv.recfmt =3D cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
> > -	priv.crqsize =3D cpu_to_le16(queue->recv_queue_size);
> > -
> >   	ret =3D rdma_accept(cm_id, &param);
> >   	if (ret)
> >   		pr_err("rdma_accept failed (error code =3D %d)\n", ret);
> > -- 2.26.2
>=20
> What is the issue with existing code that we need this patch for ?
>=20

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 =3D { }; 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=