[PATCH] RDMA/cma: Fix checkpatch error

[PATCH] RDMA/cma: Fix checkpatch error

[Max Hirsch]

When running checkpatch on cma.c the following error was found:

ERROR: do not use assignment in if condition
#413: FILE: drivers/infiniband/tmp.c:413:
+	if ((ret = (id_priv->state == comp)))

This patch moves the assignment of ret to the previous line. The if statement then checks the value of ret assigned on the previous line. The assigned value of ret is not changed. Testing involved recompiling and loading the kernel. After the changes checkpatch does not report this the error in cma.c.

Signed-off-by: Max Hirsch <max.hirsch@gmail.com>
---
 drivers/infiniband/core/cma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 25f2b70fd8ef..bdb7a8493517 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -410,7 +410,8 @@ static int cma_comp_exch(struct rdma_id_private *id_priv,
 	int ret;
 
 	spin_lock_irqsave(&id_priv->lock, flags);
-	if ((ret = (id_priv->state == comp)))
+	ret = (id_priv->state == comp);
+	if (ret)
 		id_priv->state = exch;
 	spin_unlock_irqrestore(&id_priv->lock, flags);
 	return ret;
-- 
2.17.1

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Jason Gunthorpe]

On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> When running checkpatch on cma.c the following error was found:

I think checkpatch will complain about your patch, did you run it?

> ERROR: do not use assignment in if condition
> #413: FILE: drivers/infiniband/tmp.c:413:
> +	if ((ret = (id_priv->state == comp)))
> 
> This patch moves the assignment of ret to the previous line. The if statement then checks the value of ret assigned on the previous line. The assigned value of ret is not changed. Testing involved recompiling and loading the kernel. After the changes checkpatch does not report this the error in cma.c.
> 
> Signed-off-by: Max Hirsch <max.hirsch@gmail.com>
>  drivers/infiniband/core/cma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 25f2b70fd8ef..bdb7a8493517 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -410,7 +410,8 @@ static int cma_comp_exch(struct rdma_id_private *id_priv,
>  	int ret;
>  
>  	spin_lock_irqsave(&id_priv->lock, flags);
> -	if ((ret = (id_priv->state == comp)))
> +	ret = (id_priv->state == comp);

Brackets are not needed

Ret and the return result should be changed to a bool

Jason

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Max Hirsch]

Thanks for the quick response. This is my first patch, so I want to
follow the correct protocol. I reran checkpatch after making the
changes and there were no errors or warnings in the region I changed.

...
WARNING: line over 80 characters
#308: FILE: drivers/infiniband/core/cma.c:308:
+ return cma_dev->default_roce_tos[port - rdma_start_port(cma_dev->device)];

<<<<<<<<<<<<<<<<<< HERE IS WHERE THE ERROR WAS

WARNING: line over 80 characters
#495: FILE: drivers/infiniband/core/cma.c:495:
+ struct cma_multicast *mc = container_of(kref, struct cma_multicast, mcref);
...

I was have read that it is beneficial to make the changes very small.
I wanted to target a specific checkpatch error. If you believe it
would be better I can make a patch cleaning up the entire function.

I can also make a second patch changing ret to a bool. I did not want
to do that as a part of this patch because: I wanted a VERY small
change (increase acceptance likelihood), and there is a specific
protocol for submitting multiple commits in a patch, i.e. ordering
them correctly. I did not want to introduce a potential error source
when submitting a patch i.e. I submitted 2 commits in the wrong order.

Since you have the comment to remove the brackets around the ret
assign how would I go about modifying this patch? Should I resubmit a
patch i.e. start a new email with with your proposed changes?


On Wed, Dec 11, 2019 at 11:26 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> > When running checkpatch on cma.c the following error was found:
>
> I think checkpatch will complain about your patch, did you run it?
>
> > ERROR: do not use assignment in if condition
> > #413: FILE: drivers/infiniband/tmp.c:413:
> > +     if ((ret = (id_priv->state == comp)))
> >
> > This patch moves the assignment of ret to the previous line. The if statement then checks the value of ret assigned on the previous line. The assigned value of ret is not changed. Testing involved recompiling and loading the kernel. After the changes checkpatch does not report this the error in cma.c.
> >
> > Signed-off-by: Max Hirsch <max.hirsch@gmail.com>
> >  drivers/infiniband/core/cma.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 25f2b70fd8ef..bdb7a8493517 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -410,7 +410,8 @@ static int cma_comp_exch(struct rdma_id_private *id_priv,
> >       int ret;
> >
> >       spin_lock_irqsave(&id_priv->lock, flags);
> > -     if ((ret = (id_priv->state == comp)))
> > +     ret = (id_priv->state == comp);
>
> Brackets are not needed
>
> Ret and the return result should be changed to a bool
>
> Jason

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Leon Romanovsky]

On Wed, Dec 11, 2019 at 12:26:54PM -0400, Jason Gunthorpe wrote:
> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> > When running checkpatch on cma.c the following error was found:
>
> I think checkpatch will complain about your patch, did you run it?

Jason, Doug

I would like to ask to refrain from accepting checkpatch.pl patches
which are not part of other large submission. Such standalone cleanups
do more harm than actual benefit from them for old and more or less
stable code (e.g. RDMA-CM).

Let's use the opportunity to clean the code, while we are fixing bugs
and/or submitting new features.

Thanks

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Gal Pressman]

On 12/12/2019 10:49, Leon Romanovsky wrote:
> On Wed, Dec 11, 2019 at 12:26:54PM -0400, Jason Gunthorpe wrote:
>> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
>>> When running checkpatch on cma.c the following error was found:
>>
>> I think checkpatch will complain about your patch, did you run it?
> 
> Jason, Doug
> 
> I would like to ask to refrain from accepting checkpatch.pl patches
> which are not part of other large submission. Such standalone cleanups
> do more harm than actual benefit from them for old and more or less
> stable code (e.g. RDMA-CM).

Sounds like a great approach to prevent new developers from contributing code.
You have to start somewhere and checkpatch patches are a good entry point for
such developers, discouraging them will only hurt us in the long term.

Linus had an interesting post on the subject:
https://lkml.org/lkml/2004/12/20/255

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Max Hirsch]

I am happy to make a larger/functional change. From what I read,
desired patch scope is proportional to linux community involvement but
if that not how you guys do the infiniband driver that fine. Whats a
feature you guys want but no one is working on yet, or rather where is
such a list kept?

On Thu, Dec 12, 2019 at 7:10 AM Gal Pressman <galpress@amazon.com> wrote:
>
> On 12/12/2019 10:49, Leon Romanovsky wrote:
> > On Wed, Dec 11, 2019 at 12:26:54PM -0400, Jason Gunthorpe wrote:
> >> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> >>> When running checkpatch on cma.c the following error was found:
> >>
> >> I think checkpatch will complain about your patch, did you run it?
> >
> > Jason, Doug
> >
> > I would like to ask to refrain from accepting checkpatch.pl patches
> > which are not part of other large submission. Such standalone cleanups
> > do more harm than actual benefit from them for old and more or less
> > stable code (e.g. RDMA-CM).
>
> Sounds like a great approach to prevent new developers from contributing code.
> You have to start somewhere and checkpatch patches are a good entry point for
> such developers, discouraging them will only hurt us in the long term.
>
> Linus had an interesting post on the subject:
> https://lkml.org/lkml/2004/12/20/255

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Leon Romanovsky]

On Thu, Dec 12, 2019 at 02:10:12PM +0200, Gal Pressman wrote:
> On 12/12/2019 10:49, Leon Romanovsky wrote:
> > On Wed, Dec 11, 2019 at 12:26:54PM -0400, Jason Gunthorpe wrote:
> >> On Wed, Dec 11, 2019 at 11:16:26AM +0000, Max Hirsch wrote:
> >>> When running checkpatch on cma.c the following error was found:
> >>
> >> I think checkpatch will complain about your patch, did you run it?
> >
> > Jason, Doug
> >
> > I would like to ask to refrain from accepting checkpatch.pl patches
> > which are not part of other large submission. Such standalone cleanups
> > do more harm than actual benefit from them for old and more or less
> > stable code (e.g. RDMA-CM).
>
> Sounds like a great approach to prevent new developers from contributing code.
> You have to start somewhere and checkpatch patches are a good entry point for
> such developers, discouraging them will only hurt us in the long term.

We have staging tree where new developer can train their checkpatch patches.

What about fixing smatch and sparse errors? It doesn't require HW for
that and much better entry point for the new developers.

>
> Linus had an interesting post on the subject:
> https://lkml.org/lkml/2004/12/20/255

We are in 2019 and our opinions, Linus's back 15 years ago and mine can be different.

Thanks

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Leon Romanovsky]

On Thu, Dec 12, 2019 at 07:28:19AM -0500, Max Hirsch wrote:
> I am happy to make a larger/functional change. From what I read,
> desired patch scope is proportional to linux community involvement but
> if that not how you guys do the infiniband driver that fine. Whats a
> feature you guys want but no one is working on yet, or rather where is
> such a list kept?

I'm assuming that you don't have RDMA HW, so let's start from
compilation only task.

You can start from fixing smatch and sparse compilation warnings.

Smatch:
make -j 32 CHECK=smatch -p=kernel drivers/infiniband
Sparse:
make -j 32 CHECK=sparse C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' -p=kernel drivers/infiniband

And we will be thrilled to see your patches merged.

Thanks

Re: [PATCH] RDMA/cma: Fix checkpatch error

[Jason Gunthorpe]

On Wed, Dec 11, 2019 at 08:33:10PM -0500, Max Hirsch wrote:
> Thanks for the quick response. This is my first patch, so I want to
> follow the correct protocol. I reran checkpatch after making the
> changes and there were no errors or warnings in the region I changed.

You are supposed to run the patch itself through checkpatch:

$ git format-patch HEAD^!
0001-RDMA-cma-Fix-checkpatch-error.patch
$ scripts/checkpatch.pl 0001-RDMA-cma-Fix-checkpatch-error.patch
WARNING: A patch subject line should describe the change not the tool that found it
#4: 
Subject: [PATCH] RDMA/cma: Fix checkpatch error

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#12: 
This patch moves the assignment of ret to the previous line. The if statement then checks the value of ret assigned on the previous line. The assigned value of ret is not changed. Testing involved recompiling and loading the kernel. After the changes checkpatch does not report this the error in cma.c.

total: 0 errors, 2 warnings, 9 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-RDMA-cma-Fix-checkpatch-error.patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Jason