First kernel patch - checkpatch for at86rf230.c, false-positives?

First kernel patch - checkpatch for at86rf230.c, false-positives?

[Christoffer Holmstedt]

Hi
I've access to a few raspberry pi:s with the openlabs extension board
and I'm currently configuring my development environment for kernel
development to help out with the wpan subsystem.

I just ran checkpatch.pl over at86rf230.c and got 109 errors most of
them concerning macros not enclosed in parentheses. Are these
false-positives or should I add parantheses?

Regards
--
Christoffer Holmstedt

Re: First kernel patch - checkpatch for at86rf230.c, false-positives?

[Alexander Aring]

On Thu, Apr 23, 2015 at 11:48:07AM +0200, Christoffer Holmstedt wrote:
> Hi
> I've access to a few raspberry pi:s with the openlabs extension board
> and I'm currently configuring my development environment for kernel
> development to help out with the wpan subsystem.
> 
> I just ran checkpatch.pl over at86rf230.c and got 109 errors most of
> them concerning macros not enclosed in parentheses. Are these
> false-positives or should I add parantheses?
> 

definitely false positiv!

what they do there is parameters for functions in a define.

Like:

#define STATIC_VALUES_PARAMETERS 1, 2, 3

void function(void *p, int first, int second, int third, void *foobar)
{
	....
}

and call later;

function(bar, STATIC_VALUES_PARAMETERS, foo);

compiler will replace it with:

function(bar, 1, 2, 3, foo);

and it's wrong to use:

function(bar, (1, 2, 3), foo);

it's not usually kernel programming and historical copy&pasted from
contiki code [0] or maybe contiki copy&pasted it from linux, I don't
know I don't programmed it.


checkpatch warnings about this because it's complicated to use logical
operations define inside another logical operations define. like:
BAR_MASK(FOO_MASK(bar)) - without brackets it's hard to understand
what's going on there.
But this isn't the case here.


btw:

There exists an code styling issue which is not shown by checkpatch.
It's the tab after every define, I have patches for this I will send
them later, it's included for the RFC to add phy capabilities dump.

- Alex

[0] https://github.com/contiki-os/contiki/blob/master/cpu/avr/radio/rf230/at86rf230_registermap.h

Re: First kernel patch - checkpatch for at86rf230.c, false-positives?

[Christoffer Holmstedt]

2015-04-23 12:09 GMT+02:00 Alexander Aring <alex.aring@gmail.com>:
> On Thu, Apr 23, 2015 at 11:48:07AM +0200, Christoffer Holmstedt wrote:
>> Hi
>> I've access to a few raspberry pi:s with the openlabs extension board
>> and I'm currently configuring my development environment for kernel
>> development to help out with the wpan subsystem.
>>
>> I just ran checkpatch.pl over at86rf230.c and got 109 errors most of
>> them concerning macros not enclosed in parentheses. Are these
>> false-positives or should I add parantheses?
>>
>
> definitely false positiv!
>
> what they do there is parameters for functions in a define.
>
> Like:
>
> #define STATIC_VALUES_PARAMETERS 1, 2, 3
>
> void function(void *p, int first, int second, int third, void *foobar)
> {
>         ....
> }
>
> and call later;
>
> function(bar, STATIC_VALUES_PARAMETERS, foo);
>
> compiler will replace it with:
>
> function(bar, 1, 2, 3, foo);
>
> and it's wrong to use:
>
> function(bar, (1, 2, 3), foo);
>
> it's not usually kernel programming and historical copy&pasted from
> contiki code [0] or maybe contiki copy&pasted it from linux, I don't
> know I don't programmed it.
>
>
> checkpatch warnings about this because it's complicated to use logical
> operations define inside another logical operations define. like:
> BAR_MASK(FOO_MASK(bar)) - without brackets it's hard to understand
> what's going on there.
> But this isn't the case here.
>
>
> btw:
>
> There exists an code styling issue which is not shown by checkpatch.
> It's the tab after every define, I have patches for this I will send
> them later, it's included for the RFC to add phy capabilities dump.
>
> - Alex
>
> [0] https://github.com/contiki-os/contiki/blob/master/cpu/avr/radio/rf230/at86rf230_registermap.h

Thank you Alexander for your fast and thorough response, I expected
something like that as other defines did have parantheses.

--
Christoffer Holmstedt

Re: First kernel patch - checkpatch for at86rf230.c, false-positives?

[Alexander Aring]

On Thu, Apr 23, 2015 at 12:25:34PM +0200, Christoffer Holmstedt wrote:
...
> 
> Thank you Alexander for your fast and thorough response, I expected
> something like that as other defines did have parantheses.
> 

if you like to send a first patch, maybe to "breaking the ice". Then I
see several cleanups there, like I used some magic values for masking
the trac status or trx state e.g. "trx_state = buf[1] & 0x1f;". You can
introduce some macro for that and replacing it where it used.

Or simple sending some patches for whatever you want, I/community will review
them.

With "breaking the ice", I mean some "testing linux send patch
environment" and getting familiar with sending patches. When it is your
first time.

- Alex

Re: First kernel patch - checkpatch for at86rf230.c, false-positives?

[Christoffer Holmstedt]

Yea, "breaking the ice" is what I need. So something like
"TRX_STATE_MASK (0x1f)"?
--
Christoffer Holmstedt


2015-04-23 13:15 GMT+02:00 Alexander Aring <alex.aring@gmail.com>:
> On Thu, Apr 23, 2015 at 12:25:34PM +0200, Christoffer Holmstedt wrote:
> ...
>>
>> Thank you Alexander for your fast and thorough response, I expected
>> something like that as other defines did have parantheses.
>>
>
> if you like to send a first patch, maybe to "breaking the ice". Then I
> see several cleanups there, like I used some magic values for masking
> the trac status or trx state e.g. "trx_state = buf[1] & 0x1f;". You can
> introduce some macro for that and replacing it where it used.
>
> Or simple sending some patches for whatever you want, I/community will review
> them.
>
> With "breaking the ice", I mean some "testing linux send patch
> environment" and getting familiar with sending patches. When it is your
> first time.
>
> - Alex

Re: First kernel patch - checkpatch for at86rf230.c, false-positives?

[Alexander Aring]

On Thu, Apr 23, 2015 at 01:48:05PM +0200, Christoffer Holmstedt wrote:
> Yea, "breaking the ice" is what I need. So something like
> "TRX_STATE_MASK (0x1f)"?

yes or cleanup the style issue "tabs after define" in the SR/RG defines.
It's simple a search & replace in your editor with s/define\t/define /
in that range (that's how I fix it). I will remove it then in my branch.

... or running aspell on that file, I am sure I did several typos there.
That's what others linux developers also do, running some automatic code
checkers (also aspell), but there exist more outside cppcheck, rats,
spare etc. (Normally the maintainer should run this before each patch
and I try to do that).

- Alex