* Advice on first patch
@ 2012-10-02 10:56 matthew at walster.org
2012-10-02 10:56 ` [PATCH] net: ethernet: clean out braces / old code (found via checkpatch) matthew at walster.org
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: matthew at walster.org @ 2012-10-02 10:56 UTC (permalink / raw)
To: kernelnewbies
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-02 10:56 Advice on first patch matthew at walster.org
@ 2012-10-02 10:56 ` matthew at walster.org
2012-10-02 16:16 ` Scott Lovenberg
2012-10-02 11:29 ` Advice on first patch Bjørn Mork
2012-10-03 13:40 ` Peter Senna Tschudin
2 siblings, 1 reply; 11+ messages in thread
From: matthew at walster.org @ 2012-10-02 10:56 UTC (permalink / raw)
To: kernelnewbies
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-02 10:56 ` [PATCH] net: ethernet: clean out braces / old code (found via checkpatch) matthew at walster.org
@ 2012-10-02 16:16 ` Scott Lovenberg
2012-10-03 9:13 ` Matthew Walster
0 siblings, 1 reply; 11+ messages in thread
From: Scott Lovenberg @ 2012-10-02 16:16 UTC (permalink / raw)
To: kernelnewbies
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-02 16:16 ` Scott Lovenberg
@ 2012-10-03 9:13 ` Matthew Walster
2012-10-03 13:12 ` Scott Lovenberg
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Walster @ 2012-10-03 9:13 UTC (permalink / raw)
To: kernelnewbies
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20121003/8cc33af7/attachment.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-03 9:13 ` Matthew Walster
@ 2012-10-03 13:12 ` Scott Lovenberg
2012-10-04 16:13 ` Matthew Walster
0 siblings, 1 reply; 11+ messages in thread
From: Scott Lovenberg @ 2012-10-03 13:12 UTC (permalink / raw)
To: kernelnewbies
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-03 13:12 ` Scott Lovenberg
@ 2012-10-04 16:13 ` Matthew Walster
2012-10-04 18:54 ` Bjørn Mork
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Walster @ 2012-10-04 16:13 UTC (permalink / raw)
To: kernelnewbies
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20121004/64041dca/attachment.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] net: ethernet: clean out braces / old code (found via checkpatch)
2012-10-04 16:13 ` Matthew Walster
@ 2012-10-04 18:54 ` Bjørn Mork
0 siblings, 0 replies; 11+ messages in thread
From: Bjørn Mork @ 2012-10-04 18:54 UTC (permalink / raw)
To: kernelnewbies
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Advice on first patch
2012-10-02 10:56 Advice on first patch matthew at walster.org
2012-10-02 10:56 ` [PATCH] net: ethernet: clean out braces / old code (found via checkpatch) matthew at walster.org
@ 2012-10-02 11:29 ` Bjørn Mork
2012-10-02 11:34 ` Matthew Walster
2012-10-03 13:40 ` Peter Senna Tschudin
2 siblings, 1 reply; 11+ messages in thread
From: Bjørn Mork @ 2012-10-02 11:29 UTC (permalink / raw)
To: kernelnewbies
matthew at 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Advice on first patch
2012-10-02 11:29 ` Advice on first patch Bjørn Mork
@ 2012-10-02 11:34 ` Matthew Walster
2012-10-03 9:25 ` Bjørn Mork
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Walster @ 2012-10-02 11:34 UTC (permalink / raw)
To: kernelnewbies
On 2 October 2012 12:29, Bj?rn Mork <bjorn@mork.no> wrote:
> matthew at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20121002/882cb4da/attachment-0001.html
^ permalink raw reply [flat|nested] 11+ messages in thread* Advice on first patch
2012-10-02 11:34 ` Matthew Walster
@ 2012-10-03 9:25 ` Bjørn Mork
0 siblings, 0 replies; 11+ messages in thread
From: Bjørn Mork @ 2012-10-03 9:25 UTC (permalink / raw)
To: kernelnewbies
...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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Advice on first patch
2012-10-02 10:56 Advice on first patch matthew at walster.org
2012-10-02 10:56 ` [PATCH] net: ethernet: clean out braces / old code (found via checkpatch) matthew at walster.org
2012-10-02 11:29 ` Advice on first patch Bjørn Mork
@ 2012-10-03 13:40 ` Peter Senna Tschudin
2 siblings, 0 replies; 11+ messages in thread
From: Peter Senna Tschudin @ 2012-10-03 13:40 UTC (permalink / raw)
To: kernelnewbies
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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
--
Peter
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-10-04 18:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-02 10:56 Advice on first patch matthew at walster.org
2012-10-02 10:56 ` [PATCH] net: ethernet: clean out braces / old code (found via checkpatch) matthew at walster.org
2012-10-02 16:16 ` Scott Lovenberg
2012-10-03 9:13 ` Matthew Walster
2012-10-03 13:12 ` Scott Lovenberg
2012-10-04 16:13 ` Matthew Walster
2012-10-04 18:54 ` Bjørn Mork
2012-10-02 11:29 ` Advice on first patch Bjørn Mork
2012-10-02 11:34 ` Matthew Walster
2012-10-03 9:25 ` Bjørn Mork
2012-10-03 13:40 ` Peter Senna Tschudin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).