[PATCH 1/3] IB/isert: remove unneeded new lines

[PATCH 1/3] IB/isert: remove unneeded new lines

[Max Gurtovoy]

The Linux convention is to have only 1 new line between functions.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 2ba27221ea85..5958929b7dec 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -71,7 +71,6 @@ static int isert_sg_tablesize_set(const char *val, const struct kernel_param *kp
 	return param_set_int(val, kp);
 }
 
-
 static inline bool
 isert_prot_cmd(struct isert_conn *conn, struct se_cmd *cmd)
 {
@@ -79,7 +78,6 @@ isert_prot_cmd(struct isert_conn *conn, struct se_cmd *cmd)
 		cmd->prot_op != TARGET_PROT_NORMAL);
 }
 
-
 static void
 isert_qp_event_callback(struct ib_event *e, void *context)
 {
-- 
2.25.4

[PATCH 2/3] IB/isert: remove unneeded semicolon

[Max Gurtovoy]

No need to add semicolon after closing bracket.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 5958929b7dec..96514f675427 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -1991,7 +1991,7 @@ isert_set_dif_domain(struct se_cmd *se_cmd, struct ib_sig_domain *domain)
 	if (se_cmd->prot_type == TARGET_DIF_TYPE1_PROT ||
 	    se_cmd->prot_type == TARGET_DIF_TYPE2_PROT)
 		domain->sig.dif.ref_remap = true;
-};
+}
 
 static int
 isert_set_sig_attrs(struct se_cmd *se_cmd, struct ib_sig_attrs *sig_attrs)
-- 
2.25.4

Re: [PATCH 2/3] IB/isert: remove unneeded semicolon

[Leon Romanovsky]

On Sun, Jan 10, 2021 at 11:19:02AM +0000, Max Gurtovoy wrote:
> No need to add semicolon after closing bracket.
>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  drivers/infiniband/ulp/isert/ib_isert.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 5958929b7dec..96514f675427 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -1991,7 +1991,7 @@ isert_set_dif_domain(struct se_cmd *se_cmd, struct ib_sig_domain *domain)
>  	if (se_cmd->prot_type == TARGET_DIF_TYPE1_PROT ||
>  	    se_cmd->prot_type == TARGET_DIF_TYPE2_PROT)
>  		domain->sig.dif.ref_remap = true;
> -};
> +}

The same goes for iser_set_dif_domain() and iser_cleanup_handler().

Thanks

>
>  static int
>  isert_set_sig_attrs(struct se_cmd *se_cmd, struct ib_sig_attrs *sig_attrs)
> --
> 2.25.4
>

Re: [PATCH 2/3] IB/isert: remove unneeded semicolon

[Max Gurtovoy]

On 1/10/2021 1:36 PM, Leon Romanovsky wrote:
> On Sun, Jan 10, 2021 at 11:19:02AM +0000, Max Gurtovoy wrote:
>> No need to add semicolon after closing bracket.
>>
>> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   drivers/infiniband/ulp/isert/ib_isert.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
>> index 5958929b7dec..96514f675427 100644
>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
>> @@ -1991,7 +1991,7 @@ isert_set_dif_domain(struct se_cmd *se_cmd, struct ib_sig_domain *domain)
>>   	if (se_cmd->prot_type == TARGET_DIF_TYPE1_PROT ||
>>   	    se_cmd->prot_type == TARGET_DIF_TYPE2_PROT)
>>   		domain->sig.dif.ref_remap = true;
>> -};
>> +}
> The same goes for iser_set_dif_domain() and iser_cleanup_handler().

Good catch, I'll send iSER fixes in different series.

This series is for minor fixes for iSERT.

>
> Thanks
>
>>   static int
>>   isert_set_sig_attrs(struct se_cmd *se_cmd, struct ib_sig_attrs *sig_attrs)
>> --
>> 2.25.4
>>

Re: [PATCH 2/3] IB/isert: remove unneeded semicolon

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 3/3] IB/isert: simplify signature cap check

[Max Gurtovoy]

Use if/else clause instead of "condition ? val1 : val2" to make the code
cleaner and simpler.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 96514f675427..7305ed8976c2 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -230,8 +230,10 @@ isert_create_device_ib_res(struct isert_device *device)
 	}
 
 	/* Check signature cap */
-	device->pi_capable = ib_dev->attrs.device_cap_flags &
-			     IB_DEVICE_INTEGRITY_HANDOVER ? true : false;
+	if (ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER)
+		device->pi_capable = true;
+	else
+		device->pi_capable = false;
 
 	return 0;
 }
-- 
2.25.4

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Sagi Grimberg]

> Use if/else clause instead of "condition ? val1 : val2" to make the code
> cleaner and simpler.

Not sure what is cleaner and simpler for a simple condition, but I don't
mind either way...

Acked-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Leon Romanovsky]

On Wed, Jan 13, 2021 at 04:08:29PM -0800, Sagi Grimberg wrote:
>
> > Use if/else clause instead of "condition ? val1 : val2" to make the code
> > cleaner and simpler.
>
> Not sure what is cleaner and simpler for a simple condition, but I don't
> mind either way...

Agree, probably even more cleaner variant will be:
 device->pi_capable = !!(ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER);

Thanks

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Jason Gunthorpe]

On Thu, Jan 14, 2021 at 09:29:38AM +0200, Leon Romanovsky wrote:
> On Wed, Jan 13, 2021 at 04:08:29PM -0800, Sagi Grimberg wrote:
> >
> > > Use if/else clause instead of "condition ? val1 : val2" to make the code
> > > cleaner and simpler.
> >
> > Not sure what is cleaner and simpler for a simple condition, but I don't
> > mind either way...
> 
> Agree, probably even more cleaner variant will be:
>  device->pi_capable = !!(ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER);

Gah, !! is rarely a sign of something good..

Jason

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Max Gurtovoy]

On 1/14/2021 2:49 PM, Jason Gunthorpe wrote:
> On Thu, Jan 14, 2021 at 09:29:38AM +0200, Leon Romanovsky wrote:
>> On Wed, Jan 13, 2021 at 04:08:29PM -0800, Sagi Grimberg wrote:
>>>> Use if/else clause instead of "condition ? val1 : val2" to make the code
>>>> cleaner and simpler.
>>> Not sure what is cleaner and simpler for a simple condition, but I don't
>>> mind either way...
>> Agree, probably even more cleaner variant will be:
>>   device->pi_capable = !!(ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER);
> Gah, !! is rarely a sign of something good..

can we take the series as-is ?

We have enough Acks and eyes on these minor cleanups.


>
> Jason

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Leon Romanovsky]

On Thu, Jan 14, 2021 at 02:54:29PM +0200, Max Gurtovoy wrote:
>
> On 1/14/2021 2:49 PM, Jason Gunthorpe wrote:
> > On Thu, Jan 14, 2021 at 09:29:38AM +0200, Leon Romanovsky wrote:
> > > On Wed, Jan 13, 2021 at 04:08:29PM -0800, Sagi Grimberg wrote:
> > > > > Use if/else clause instead of "condition ? val1 : val2" to make the code
> > > > > cleaner and simpler.
> > > > Not sure what is cleaner and simpler for a simple condition, but I don't
> > > > mind either way...
> > > Agree, probably even more cleaner variant will be:
> > >   device->pi_capable = !!(ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER);
> > Gah, !! is rarely a sign of something good..
>
> can we take the series as-is ?

Of course :)

Thanks

Re: [PATCH 3/3] IB/isert: simplify signature cap check

[Leon Romanovsky]

On Thu, Jan 14, 2021 at 08:49:39AM -0400, Jason Gunthorpe wrote:
> On Thu, Jan 14, 2021 at 09:29:38AM +0200, Leon Romanovsky wrote:
> > On Wed, Jan 13, 2021 at 04:08:29PM -0800, Sagi Grimberg wrote:
> > >
> > > > Use if/else clause instead of "condition ? val1 : val2" to make the code
> > > > cleaner and simpler.
> > >
> > > Not sure what is cleaner and simpler for a simple condition, but I don't
> > > mind either way...
> >
> > Agree, probably even more cleaner variant will be:
> >  device->pi_capable = !!(ib_dev->attrs.device_cap_flags & IB_DEVICE_INTEGRITY_HANDOVER);
>
> Gah, !! is rarely a sign of something good..

You see, three of us see same code differently.

>
> Jason

Re: [PATCH 1/3] IB/isert: remove unneeded new lines

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/3] IB/isert: remove unneeded new lines

[Jason Gunthorpe]

On Sun, Jan 10, 2021 at 11:19:01AM +0000, Max Gurtovoy wrote:
> The Linux convention is to have only 1 new line between functions.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/infiniband/ulp/isert/ib_isert.c | 2 --
>  1 file changed, 2 deletions(-)

Series applied to for-next, thanks

Jason