RDMA/cma: Fix checkpatch error

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.

---
 drivers/infiniband/core/cma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 25f2b70fd8ef..bdb7a8493517 100644
@@ -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

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

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?

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

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

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

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

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.

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

Thanks