Skip to contentSkip to conversation

checkpatch: remove the ether_addr_copy warning

PATCH In review Dan Carpenter 3 participants 7 messages Oct 3, 2014

Subsystems (4)
Networking Network drivers Kernel janitors Checkpatch
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 checkpatch: remove the ether_addr_copy warning

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 <[email protected]>
scripts/checkpatch.pl +0 -10
index 52a223e..d540dd4 100755
@@ -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 &&
maintainer
#1 lore raw
Quoted 4 lines · 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.

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.

Quoted 2 lines

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.

#2 lore raw
Quoted 9 lines · Joe Perches
Quoted 4 lines · 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.

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

Quoted 8 lines

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

Quoted 2 lines

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

maintainer
#3 lore raw
Quoted 11 lines · Julia Lawall
Quoted 10 lines · Joe Perches
Quoted 4 lines · 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.

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.

[]

Quoted 3 lines

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.

#4 lore raw
Quoted 17 lines · Joe Perches
Quoted 11 lines · Julia Lawall
Quoted 10 lines · Joe Perches
Quoted 4 lines · 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.

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.

[]

Quoted 3 lines

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?

Quoted 6 lines

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

author
#5 lore raw
In reply to Joe Perches
Quoted 8 lines · Joe Perches
Quoted 4 lines · 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.

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...

Quoted 2 lines

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

maintainer
#6 lore raw
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Joe Perches
Quoted 4 lines · 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.

And that's where it's the maintainer's job to educate, inform, reject

Quoted 7 lines
Quoted 2 lines

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)

Quoted 2 lines

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

I think that's a learning opportunity...

Quoted 3 lines
Quoted 1 line

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.

Quoted 2 lines

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.