Advice on first patch
I'm hopefully going to be submitting the attached patch to the mainline kernel tree, and as it's my first patch, I figured it would be wise to run it past KN first in case I'd done something monumentally stupid! I found it through a checkpatch run, from the excellent talk by GregKH at FOSDEM a year or so back. Any input would be greatly appreciated! Matthew Walster
From: Matthew Walster <matthew@walster.org> Remove an old commented out piece of code. Remove an if(true) statement. Remove a set of braces that weren't strictly necessary. All found by running checkpatch.pl against the code. Signed-off-by: Matthew Walster <matthew@walster.org> --- net/ethernet/eth.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 4efad53..a9f8531 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -178,11 +178,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev) * seems to set IFF_PROMISC. */ - else if (1 /*dev->flags&IFF_PROMISC */ ) { - if (unlikely(!ether_addr_equal_64bits(eth->h_dest, - dev->dev_addr))) - skb->pkt_type = PACKET_OTHERHOST; - } + else if (unlikely(!ether_addr_equal_64bits(eth->h_dest, dev->dev_addr))) + skb->pkt_type = PACKET_OTHERHOST; /* * Some variants of DSA tagging don't have an ethertype field -- 1.7.10.4
On Tue, Oct 2, 2012 at 6:56 AM, <matthew@walster.org> wrote:
From: Matthew Walster <matthew@walster.org>
Remove an old commented out piece of code. Remove an if(true) statement. Remove a set of braces that weren't strictly necessary.
All found by running checkpatch.pl against the code.
Signed-off-by: Matthew Walster <matthew@walster.org> --- net/ethernet/eth.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 4efad53..a9f8531 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -178,11 +178,8 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev) * seems to set IFF_PROMISC. */
- else if (1 /*dev->flags&IFF_PROMISC */ ) { - if (unlikely(!ether_addr_equal_64bits(eth->h_dest, - dev->dev_addr))) - skb->pkt_type = PACKET_OTHERHOST; - } + else if (unlikely(!ether_addr_equal_64bits(eth->h_dest, dev->dev_addr))) + skb->pkt_type = PACKET_OTHERHOST;
/* * Some variants of DSA tagging don't have an ethertype field -- 1.7.10.4
Looks good to me. -- Peace and Blessings, -Scott.
On 2 October 2012 17:16, Scott Lovenberg <scott.lovenberg@gmail.com> wrote:
Looks good to me.
Maintainer didn't think so :( On 2 October 2012 19:46, David Miller <davem@davemloft.net> wrote:
That comment and that unconditional if() are documentation.
Don't be an automaton and blindly make changes based upon checkpatch.pl output.
Perhaps I'll just clean up some of drivers/staging while I learn the process before I dive in to "net" again. Matthew Walster
On Wed, Oct 3, 2012 at 5:13 AM, Matthew Walster <matthew@walster.org> wrote:
On 2 October 2012 17:16, Scott Lovenberg <scott.lovenberg@gmail.com> wrote:
Looks good to me.
Maintainer didn't think so :(
On 2 October 2012 19:46, David Miller <davem@davemloft.net> wrote:
That comment and that unconditional if() are documentation.
Don't be an automaton and blindly make changes based upon checkpatch.pl output.
Perhaps I'll just clean up some of drivers/staging while I learn the process before I dive in to "net" again.
Matthew Walster
Sorry, man. I'm not going to mix it up with Dave Miller, but really if he wanted that to stay in there a comment suggesting so would have been nice. -- Peace and Blessings, -Scott.
On 3 October 2012 14:12, Scott Lovenberg <scott.lovenberg@gmail.com> wrote:
Sorry, man. I'm not going to mix it up with Dave Miller, but really if he wanted that to stay in there a comment suggesting so would have been nice.
I have a thick skin, don't worry! I figured I was being helpful, clearly I caught him either at a bad time or he's just had a rash of semi-ignorant checkpatch-monkeys submitting and he's fed up with us! M
Matthew Walster <matthew@walster.org> writes:
On 3 October 2012 14:12, Scott Lovenberg <scott.lovenberg@gmail.com> wrote:
Sorry, man. I'm not going to mix it up with Dave Miller, but really if he wanted that to stay in there a comment suggesting so would have been nice.
I have a thick skin, don't worry! I figured I was being helpful, clearly I caught him either at a bad time or he's just had a rash of semi-ignorant checkpatch-monkeys submitting and he's fed up with us!
Please note that despite the direct tone, the reply you got a) was prompt, b) explained why the patch was rejected, c) contained additional advice on how to proceed You will learn to appreciate these things after a while. The "net" subsystem is one of the best places to send a patch, just because of the three points above. I am not saying that there aren't other subsystems which are equally well maintained. There sure are. But the "net" subsystem is definitely among the best, and that makes it easy to work with for us new patch submitters. The absolute worst thing that could have happened would have been no answer at all, right? Bjørn
matthew@walster.org writes:
I'm hopefully going to be submitting the attached patch to the mainline kernel tree, and as it's my first patch, I figured it would be wise to run it past KN first in case I'd done something monumentally stupid! I found it through a checkpatch run, from the excellent talk by GregKH at FOSDEM a year or so back.
Any input would be greatly appreciated!
Looks fine to me, but you might want to remove/update the related comment as well: /* * This ALLMULTI check should be redundant by 1.4 * so don't forget to remove it. * * Seems, you forgot to remove it. All silly devices * seems to set IFF_PROMISC. */ else if (1 /*dev->flags&IFF_PROMISC */ ) { if (unlikely(!ether_addr_equal_64bits(eth->h_dest, dev->dev_addr))) skb->pkt_type = PACKET_OTHERHOST; } Looks like someone was expecting a 1.4 at the time. We know better now :-) Bjørn
On 2 October 2012 12:29, Bjørn Mork <bjorn@mork.no> wrote:
matthew@walster.org writes:
I'm hopefully going to be submitting the attached patch to the mainline kernel tree, and as it's my first patch, I figured it would be wise to run it past KN first in case I'd done something monumentally stupid! I found it through a checkpatch run, from the excellent talk by GregKH at FOSDEM a year or so back.
Any input would be greatly appreciated!
Looks fine to me, but you might want to remove/update the related comment as well:
/* * This ALLMULTI check should be redundant by 1.4 * so don't forget to remove it. * * Seems, you forgot to remove it. All silly devices * seems to set IFF_PROMISC. */
else if (1 /*dev->flags&IFF_PROMISC */ ) { if (unlikely(!ether_addr_equal_64bits(eth->h_dest, dev->dev_addr))) skb->pkt_type = PACKET_OTHERHOST; }
Looks like someone was expecting a 1.4 at the time. We know better now :-)
Thanks, marvellous ;) Matthew Walster
...but davem wanted to keep it as documentation. Who could have imagined that? That's how things work. Just find something else to fix. Sooner or later you will get a patch accepted :-) But you might consider taking his (hidden) advise and look for something more challenging than checkpatch output. There are plenty of bugs to fix and drivers to write. You might already own some hardware which isn't fully supported? Try to find out why, and see if you can fix it. Not everything is fixable this way, as lack of documentation often is the problem. But there are more than enough things that are. Bjørn
Matthew, The file drivers/net/wan/cosa.c has many lines like: return -1; and err = -1; goto err_out; Returning -1 is not the default way of returning error codes. Take a look at http://lxr.free-electrons.com/source/include/asm-generic/errno-base.h for a small list of error codes. There is also http://lxr.free-electrons.com/source/include/asm-generic/errno.h. If you decide to propose the use of error codes instead of -1, make sure to update include section if needed. You can see how other functions use return codes... Peter On Tue, Oct 2, 2012 at 12:56 PM, <matthew@walster.org> wrote:
I'm hopefully going to be submitting the attached patch to the mainline kernel tree, and as it's my first patch, I figured it would be wise to run it past KN first in case I'd done something monumentally stupid! I found it through a checkpatch run, from the excellent talk by GregKH at FOSDEM a year or so back.
Any input would be greatly appreciated!
Matthew Walster
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-- Peter
participants (5)
-
Bjørn Mork -
Matthew Walster -
matthew＠walster.org
-
Peter Senna Tschudin -
Scott Lovenberg