[patch] checkpatch: remove the ether_addr_copy warning

[patch] checkpatch: remove the ether_addr_copy warning

[Dan Carpenter]

Most people sending checkpatch.pl fixes don't know how to verify the
alignment.  This checkpatch warning just encourages newbies to try
introduce bugs.  Patch submitters tell us that they just sed the code
and it's the job for the maintainer to check that it's correct.

If you want to work on these then you can get the same information by
typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN`

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 52a223e..d540dd4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4698,16 +4698,6 @@ sub process {
 			}
 		}
 
-# Check for memcpy(foo, bar, ETH_ALEN) that could be ether_addr_copy(foo, bar)
-		if ($^V && $^V ge 5.10.0 &&
-		    $line =~ /^\+(?:.*?)\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s) {
-			if (WARN("PREFER_ETHER_ADDR_COPY",
-				 "Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)\n" . $herecurr) &&
-			    $fix) {
-				$fixed[$fixlinenr] =~ s/\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/ether_addr_copy($2, $7)/;
-			}
-		}
-
 # typecasts on min/max could be min_t/max_t
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Joe Perches]

On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> Most people sending checkpatch.pl fixes don't know how to verify the
> alignment.  This checkpatch warning just encourages newbies to try
> introduce bugs.  Patch submitters tell us that they just sed the code
> and it's the job for the maintainer to check that it's correct.

I haven't seen many instances of bad patch submittals
on netdev.  Is this mostly an issue for staging?

Maybe a downgrade to CHK requiring --strict is OK.

> If you want to work on these then you can get the same information by
> typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN`

That's not much of an argument for or against anything in
checkpatch as every operation done by it can be reproduced
by a series of grep operations.

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Julia Lawall]

On Fri, 3 Oct 2014, Joe Perches wrote:

> On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> > Most people sending checkpatch.pl fixes don't know how to verify the
> > alignment.  This checkpatch warning just encourages newbies to try
> > introduce bugs.  Patch submitters tell us that they just sed the code
> > and it's the job for the maintainer to check that it's correct.
>
> I haven't seen many instances of bad patch submittals
> on netdev.  Is this mostly an issue for staging?
>
> Maybe a downgrade to CHK requiring --strict is OK.
>
> > If you want to work on these then you can get the same information by
> > typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN`
>
> That's not much of an argument for or against anything in
> checkpatch as every operation done by it can be reproduced
> by a series of grep operations.

I think it is too bad to have a piece of knowledge that was apparent be
made more obscure.  Why not just change the checkpatch warning to make
more explicit that a lot of expertise is required to make the change?

julia

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Joe Perches]

On Fri, 2014-10-03 at 16:30 +0200, Julia Lawall wrote:
> On Fri, 3 Oct 2014, Joe Perches wrote:
> > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> > > Most people sending checkpatch.pl fixes don't know how to verify the
> > > alignment.  This checkpatch warning just encourages newbies to try
> > > introduce bugs.  Patch submitters tell us that they just sed the code
> > > and it's the job for the maintainer to check that it's correct.
> >
> > I haven't seen many instances of bad patch submittals
> > on netdev.  Is this mostly an issue for staging?
> >
> > Maybe a downgrade to CHK requiring --strict is OK.
[]
> I think it is too bad to have a piece of knowledge that was apparent be
> made more obscure.  Why not just change the checkpatch warning to make
> more explicit that a lot of expertise is required to make the change?

Any wordsmithing appreciated.  Maybe something like:

from: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)
to: Where both Ethernet addresses are guaranteed to be __aligned(2), prefer ether_addr_copy() over memcpy() 

That won't stop people from blindly following any message.

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Julia Lawall]

On Fri, 3 Oct 2014, Joe Perches wrote:

> On Fri, 2014-10-03 at 16:30 +0200, Julia Lawall wrote:
> > On Fri, 3 Oct 2014, Joe Perches wrote:
> > > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> > > > Most people sending checkpatch.pl fixes don't know how to verify the
> > > > alignment.  This checkpatch warning just encourages newbies to try
> > > > introduce bugs.  Patch submitters tell us that they just sed the code
> > > > and it's the job for the maintainer to check that it's correct.
> > >
> > > I haven't seen many instances of bad patch submittals
> > > on netdev.  Is this mostly an issue for staging?
> > >
> > > Maybe a downgrade to CHK requiring --strict is OK.
> []
> > I think it is too bad to have a piece of knowledge that was apparent be
> > made more obscure.  Why not just change the checkpatch warning to make
> > more explicit that a lot of expertise is required to make the change?
>
> Any wordsmithing appreciated.  Maybe something like:
>
> from: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)
> to: Where both Ethernet addresses are guaranteed to be __aligned(2), prefer ether_addr_copy() over memcpy()
>
> That won't stop people from blindly following any message.

Perhaps add: Note that checking whether the addresses are aligned may
require knowing properties of all possible architectures on which the code
may be executed.  Make this change only if you have this information.

julia

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Dan Carpenter]

On Fri, Oct 03, 2014 at 07:22:27AM -0700, Joe Perches wrote:
> On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> > Most people sending checkpatch.pl fixes don't know how to verify the
> > alignment.  This checkpatch warning just encourages newbies to try
> > introduce bugs.  Patch submitters tell us that they just sed the code
> > and it's the job for the maintainer to check that it's correct.
> 
> I haven't seen many instances of bad patch submittals
> on netdev.  Is this mostly an issue for staging?

I don't follow netdev so I can't say.

Most of the time data is aligned at a 4 byte mark so probably you are
just getting lucky.  I really doubt that netdev checkpatch newbies know
about alignment...

> 
> Maybe a downgrade to CHK requiring --strict is OK.

I would actually like to turn --strict by default in staging.

Checkpatch is a good concept, but it should only do safe things instead
of telling newbies to send buggy patches.

regards,
dan carpenter

Re: [patch] checkpatch: remove the ether_addr_copy warning

[Joe Perches]

On Fri, 2014-10-03 at 17:47 +0300, Dan Carpenter wrote:
> On Fri, Oct 03, 2014 at 07:22:27AM -0700, Joe Perches wrote:
> > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote:
> > > Most people sending checkpatch.pl fixes don't know how to verify the
> > > alignment.  This checkpatch warning just encourages newbies to try
> > > introduce bugs.  Patch submitters tell us that they just sed the code
> > > and it's the job for the maintainer to check that it's correct.

And that's where it's the maintainer's job to educate,
inform, reject 
 
> > I haven't seen many instances of bad patch submittals
> > on netdev.  Is this mostly an issue for staging?
> 
> I don't follow netdev so I can't say.
> 
> Most of the time data is aligned at a 4 byte mark so probably you are
> just getting lucky.

All typical Ethernet frames have 1 of the 2 addresses on an
even byte boundary but not 4 byte aligned.

Most all of the is_<foo>_ether_addr tests assume __aligned(2)

> I really doubt that netdev checkpatch newbies know
> about alignment...

I think that's a learning opportunity...

> > Maybe a downgrade to CHK requiring --strict is OK.
> 
> I would actually like to turn --strict by default in staging.

I recall a suggested patch for that.

> Checkpatch is a good concept, but it should only do safe things instead
> of telling newbies to send buggy patches.

checkpatch isn't just for the inexperienced.
It's an oversight tester and a style tool.

Degrading it doesn't make it better.