Advice on first patch

Advice on first patch

[matthew at walster.org]

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

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[matthew at walster.org]

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

Advice on first patch

[Bjørn Mork]

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

Advice on first patch

[Matthew Walster]

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 

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[Scott Lovenberg]

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.

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[Matthew Walster]

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 

Advice on first patch

[Bjørn Mork]

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

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[Scott Lovenberg]

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.

Advice on first patch

[Peter Senna Tschudin]

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

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[Matthew Walster]

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 

[PATCH] net: ethernet: clean out braces / old code (found via checkpatch)

[Bjørn Mork]

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