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 <[email protected]>
---
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
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 <[email protected]>
> 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
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 <[email protected]> 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 <[email protected]>
> > 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
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
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
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 <[email protected]> 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
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
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