i915 vs checkpatch

i915 vs checkpatch

[Arkadiusz Hiler]

Hey all,

Since not so long ago our CI is running and reporting sparse and
checkpatch. Sparse is doing just fine but I had to disable checkpatch
for the time being - too much "false" positives causing people to
complain. It's simply confusing to see one thing in the code, and
fitting your change in only to get a report that it's wrong.

We are explicitly going against couple of the recommendations it tries
to enforce, e.g. not using BIT macro, splitting quoted strings:
https://lists.freedesktop.org/archives/intel-gfx/2018-February/156613.html

IMHO we should make a couple of decisions here:
 1. Do we really want to use the checkpatch / have CI reports?
 2. Which of the checkpatch checks we want to disabled for i915?
 3. How strongly do we want to enforce the rest?
 4. Do we want to change what's already in the tree, for compliance?

Recent-ish drm-tip, vanilla checkpatch on i915 reports:
total: 399 errors, 3573 warnings, 209374 lines checked

-- 
Cheers,
Arek


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> Since not so long ago our CI is running and reporting sparse and
> checkpatch. Sparse is doing just fine but I had to disable checkpatch
> for the time being - too much "false" positives causing people to
> complain. It's simply confusing to see one thing in the code, and
> fitting your change in only to get a report that it's wrong.
>
> We are explicitly going against couple of the recommendations it tries
> to enforce, e.g. not using BIT macro, splitting quoted strings:
> https://lists.freedesktop.org/archives/intel-gfx/2018-February/156613.html
>
> IMHO we should make a couple of decisions here:
>  1. Do we really want to use the checkpatch / have CI reports?

I think yes, for the benefit of both patch authors and reviewers. For
the most part, we do want to encourage uniform style.

>  2. Which of the checkpatch checks we want to disabled for i915?

One low hanging fruit is to ignore the CHECK messages, or drop the
--strict option to checkpatch.pl in CI, although I think some of them
are nice.

>  3. How strongly do we want to enforce the rest?

That's a tough one. With code movement, you want the code to remain the
same instead of changing at the same time. And some of the warnings are
subjective. For example, I'd prefer to stick with the 80 column rule but
only when it makes sense. ;)

Another example, I would like to move towards kernel types over uint8_t
and friends. However, when you have code surrounded by uint8_t and
friends, it's often better to stick with the style around you.

>  4. Do we want to change what's already in the tree, for compliance?

No. I don't think we should encourage mindless checkpatch fixes.

Does checkpatch support disabling checks or do you have to filter them
out from the output?

BR,
Jani.


>
> Recent-ish drm-tip, vanilla checkpatch on i915 reports:
> total: 399 errors, 3573 warnings, 209374 lines checked

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 12:43:22PM +0200, Jani Nikula wrote:
> On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> > Since not so long ago our CI is running and reporting sparse and
> > checkpatch. Sparse is doing just fine but I had to disable checkpatch
> > for the time being - too much "false" positives causing people to
> > complain. It's simply confusing to see one thing in the code, and
> > fitting your change in only to get a report that it's wrong.
> >
> > We are explicitly going against couple of the recommendations it tries
> > to enforce, e.g. not using BIT macro, splitting quoted strings:
> > https://lists.freedesktop.org/archives/intel-gfx/2018-February/156613.html
> >
> > IMHO we should make a couple of decisions here:
> >  1. Do we really want to use the checkpatch / have CI reports?
> 
> I think yes, for the benefit of both patch authors and reviewers. For
> the most part, we do want to encourage uniform style.
> 
> >  2. Which of the checkpatch checks we want to disabled for i915?
> 
> One low hanging fruit is to ignore the CHECK messages, or drop the
> --strict option to checkpatch.pl in CI, although I think some of them
> are nice.

IMO the strict vs. not split is totally arbitrary. Some really obviously
correct warning are only enabled with strict, whereas even w/o strict
you get some warnings that are just plain silly. Thus I think strict
does more good than harm.

> 
> >  3. How strongly do we want to enforce the rest?
> 
> That's a tough one. With code movement, you want the code to remain the
> same instead of changing at the same time. And some of the warnings are
> subjective. For example, I'd prefer to stick with the 80 column rule but
> only when it makes sense. ;)
> 
> Another example, I would like to move towards kernel types over uint8_t
> and friends. However, when you have code surrounded by uint8_t and
> friends, it's often better to stick with the style around you.
> 
> >  4. Do we want to change what's already in the tree, for compliance?
> 
> No. I don't think we should encourage mindless checkpatch fixes.
> 
> Does checkpatch support disabling checks or do you have to filter them
> out from the output?
> 
> BR,
> Jani.
> 
> 
> >
> > Recent-ish drm-tip, vanilla checkpatch on i915 reports:
> > total: 399 errors, 3573 warnings, 209374 lines checked
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dim-tools mailing list
> dim-tools@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

I went through the recent checkpatch reports, and here's my take.

On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
>  2. Which of the checkpatch checks we want to disabled for i915?

I'd like to have these silenced:

CHECK: No space is necessary after a cast
WARNING: line over 80 characters
WARNING: quoted string split across lines

I'd prefer we conform to the last two too, but there's just too much
noise and too many cases where we explicitly should ignore them.

For the time being, I think we may have to silence these ones too, but
I'd like us to discuss enforcing them:

CHECK: Prefer kernel type 'u16' over 'uint16_t'
CHECK: Prefer kernel type 'u32' over 'uint32_t'
CHECK: Prefer kernel type 'u64' over 'uint64_t'
CHECK: Prefer kernel type 'u8' over 'uint8_t'
CHECK: Prefer using the BIT macro

The BIT macros is one that I'd consider accepting a one-time conversion
of i915_reg.h and after that use it exclusively. But up to debate.

>  3. How strongly do we want to enforce the rest?

It depends on the case. Some of the warnings are notes to check, will be
emitted even for correct code, and can't be automatically enforced.

Of the recently reported ones, I'd like to enforce:

CHECK: Alignment should match open parenthesis
CHECK: Blank lines aren't necessary after an open brace '{'
CHECK: Lines should not end with a '('
CHECK: Please don't use multiple blank lines
CHECK: Please use a blank line after function/struct/union/enum declarations
CHECK: Unbalanced braces around else statement
CHECK: Unnecessary parentheses around 'level >= 1'
CHECK: Unnecessary parentheses around 'pipe == PIPE_A'
CHECK: braces {} should be used on all arms of this statement
CHECK: multiple assignments should be avoided
CHECK: spaces preferred around that '*' (ctx:VxV)
CHECK: spaces preferred around that '<<' (ctx:VxV)
CHECK: spinlock_t definition without comment
CHECK: struct mutex definition without comment
ERROR: Missing Signed-off-by: line(s)
ERROR: Unrecognized email address: Foo Bar <foo.bar@example.com'
ERROR: code indent should use tabs where possible
ERROR: spaces required around that '=' (ctx:VxW)
ERROR: trailing whitespace
WARNING: 'consistancy' may be misspelled - perhaps 'consistency'?
WARNING: 'writting' may be misspelled - perhaps 'writing'?
WARNING: Missing a blank line after declarations
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
WARNING: Statements should start on a tabstop
WARNING: please, no space before tabs
WARNING: please, no spaces at the start of a line
WARNING: suspect code indent for conditional statements (8, 17)

These ones should be double checked by author/reviewer/committer. These
can't be automatically enforced:

CHECK: Macro argument reuse 'info' - possible side-effects?
ERROR: Macros with complex values should be enclosed in parentheses
WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
WARNING: Macros with flow control statements should be avoided
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

If we have the filtering in place in dim, we could require the committer
to pass a parameter to dim to apply patches with the above warnings.

>  4. Do we want to change what's already in the tree, for compliance?

With the exception of the BIT() macro, I still think not. But patch
series touching existing code should move towards compliance (for want
of a better word).


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Rodrigo Vivi]

On Thu, Mar 01, 2018 at 06:13:31PM +0200, Jani Nikula wrote:
> 
> I went through the recent checkpatch reports, and here's my take.
> 
> On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> >  2. Which of the checkpatch checks we want to disabled for i915?
> 
> I'd like to have these silenced:
> 
> CHECK: No space is necessary after a cast
> WARNING: line over 80 characters
> WARNING: quoted string split across lines
> 
> I'd prefer we conform to the last two too, but there's just too much
> noise and too many cases where we explicitly should ignore them.
> 
> For the time being, I think we may have to silence these ones too, but
> I'd like us to discuss enforcing them:
> 
> CHECK: Prefer kernel type 'u16' over 'uint16_t'
> CHECK: Prefer kernel type 'u32' over 'uint32_t'
> CHECK: Prefer kernel type 'u64' over 'uint64_t'
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
> CHECK: Prefer using the BIT macro
> 
> The BIT macros is one that I'd consider accepting a one-time conversion
> of i915_reg.h and after that use it exclusively. But up to debate.

For this one I just wonder if we would need to do a massive
change before. Because it would get ugly to have mixed cases.

ack on all the rest of the list here on this email.

> 
> >  3. How strongly do we want to enforce the rest?
> 
> It depends on the case. Some of the warnings are notes to check, will be
> emitted even for correct code, and can't be automatically enforced.
> 
> Of the recently reported ones, I'd like to enforce:
> 
> CHECK: Alignment should match open parenthesis
> CHECK: Blank lines aren't necessary after an open brace '{'
> CHECK: Lines should not end with a '('
> CHECK: Please don't use multiple blank lines
> CHECK: Please use a blank line after function/struct/union/enum declarations
> CHECK: Unbalanced braces around else statement
> CHECK: Unnecessary parentheses around 'level >= 1'
> CHECK: Unnecessary parentheses around 'pipe == PIPE_A'
> CHECK: braces {} should be used on all arms of this statement
> CHECK: multiple assignments should be avoided
> CHECK: spaces preferred around that '*' (ctx:VxV)
> CHECK: spaces preferred around that '<<' (ctx:VxV)
> CHECK: spinlock_t definition without comment
> CHECK: struct mutex definition without comment
> ERROR: Missing Signed-off-by: line(s)
> ERROR: Unrecognized email address: Foo Bar <foo.bar@example.com'
> ERROR: code indent should use tabs where possible
> ERROR: spaces required around that '=' (ctx:VxW)
> ERROR: trailing whitespace
> WARNING: 'consistancy' may be misspelled - perhaps 'consistency'?
> WARNING: 'writting' may be misspelled - perhaps 'writing'?
> WARNING: Missing a blank line after declarations
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> WARNING: Statements should start on a tabstop
> WARNING: please, no space before tabs
> WARNING: please, no spaces at the start of a line
> WARNING: suspect code indent for conditional statements (8, 17)
> 
> These ones should be double checked by author/reviewer/committer. These
> can't be automatically enforced:
> 
> CHECK: Macro argument reuse 'info' - possible side-effects?
> ERROR: Macros with complex values should be enclosed in parentheses
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()
> WARNING: Macros with flow control statements should be avoided
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> 
> If we have the filtering in place in dim, we could require the committer
> to pass a parameter to dim to apply patches with the above warnings.
> 
> >  4. Do we want to change what's already in the tree, for compliance?
> 
> With the exception of the BIT() macro, I still think not. But patch
> series touching existing code should move towards compliance (for want
> of a better word).
> 
> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dim-tools mailing list
> dim-tools@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dim-tools
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Chris Wilson]

Quoting Arkadiusz Hiler (2018-03-01 09:47:06)
> Hey all,
> 
> Since not so long ago our CI is running and reporting sparse and
> checkpatch. Sparse is doing just fine but I had to disable checkpatch
> for the time being - too much "false" positives causing people to
> complain. It's simply confusing to see one thing in the code, and
> fitting your change in only to get a report that it's wrong.

Another aspect is that we use the kernel coding style for igt as well.
checkpatch.pl should be able to pick up style issues on igt patches.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Joonas Lahtinen]

Quoting Rodrigo Vivi (2018-03-01 20:00:07)
> On Thu, Mar 01, 2018 at 06:13:31PM +0200, Jani Nikula wrote:
> > 
> > I went through the recent checkpatch reports, and here's my take.
> > 
> > On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> > >  2. Which of the checkpatch checks we want to disabled for i915?
> > 
> > I'd like to have these silenced:
> > 
> > CHECK: No space is necessary after a cast
> > WARNING: line over 80 characters
> > WARNING: quoted string split across lines
> > 
> > I'd prefer we conform to the last two too, but there's just too much
> > noise and too many cases where we explicitly should ignore them.
> > 
> > For the time being, I think we may have to silence these ones too, but
> > I'd like us to discuss enforcing them:
> > 
> > CHECK: Prefer kernel type 'u16' over 'uint16_t'
> > CHECK: Prefer kernel type 'u32' over 'uint32_t'
> > CHECK: Prefer kernel type 'u64' over 'uint64_t'
> > CHECK: Prefer kernel type 'u8' over 'uint8_t'
> > CHECK: Prefer using the BIT macro
> > 
> > The BIT macros is one that I'd consider accepting a one-time conversion
> > of i915_reg.h and after that use it exclusively. But up to debate.
> 
> For this one I just wonder if we would need to do a massive
> change before. Because it would get ugly to have mixed cases.

Yep, the mixed cases are bit tough to automatically enforce. So the
transitional phase will always be troublesome, and trying to make that
shorter makes some sense to me.

Traditionally we've avoided mass changes just for the changes, but we
have to assess the value of doing it against what we get. That is
getting automatic enforcement, once we've converted over.

We're not that far off the mark with u(32|16|8) vs uint(32|16|8)_t:

i915$ git grep -E "uint(32|16|8)_t" | wc -l
852
i915$ git grep -E "u(32|16|8)" | wc -l
3857

I don't consider that undoable.

BIT() is in the minority at the moment, so it might benefit even more as
people often cargo-cult the programming style from other places in the code.

I think it might be worthy doing these two changes to get the automatic
enforemend and avoid the codebase staying in limbo. Machine overlords are
way better at enforcing any code checks than us humans.

Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

On Fri, 02 Mar 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote:
> Quoting Rodrigo Vivi (2018-03-01 20:00:07)
>> On Thu, Mar 01, 2018 at 06:13:31PM +0200, Jani Nikula wrote:
>> > 
>> > I went through the recent checkpatch reports, and here's my take.
>> > 
>> > On Thu, 01 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
>> > >  2. Which of the checkpatch checks we want to disabled for i915?
>> > 
>> > I'd like to have these silenced:
>> > 
>> > CHECK: No space is necessary after a cast
>> > WARNING: line over 80 characters
>> > WARNING: quoted string split across lines
>> > 
>> > I'd prefer we conform to the last two too, but there's just too much
>> > noise and too many cases where we explicitly should ignore them.
>> > 
>> > For the time being, I think we may have to silence these ones too, but
>> > I'd like us to discuss enforcing them:
>> > 
>> > CHECK: Prefer kernel type 'u16' over 'uint16_t'
>> > CHECK: Prefer kernel type 'u32' over 'uint32_t'
>> > CHECK: Prefer kernel type 'u64' over 'uint64_t'
>> > CHECK: Prefer kernel type 'u8' over 'uint8_t'
>> > CHECK: Prefer using the BIT macro
>> > 
>> > The BIT macros is one that I'd consider accepting a one-time conversion
>> > of i915_reg.h and after that use it exclusively. But up to debate.
>> 
>> For this one I just wonder if we would need to do a massive
>> change before. Because it would get ugly to have mixed cases.
>
> Yep, the mixed cases are bit tough to automatically enforce. So the
> transitional phase will always be troublesome, and trying to make that
> shorter makes some sense to me.
>
> Traditionally we've avoided mass changes just for the changes, but we
> have to assess the value of doing it against what we get. That is
> getting automatic enforcement, once we've converted over.
>
> We're not that far off the mark with u(32|16|8) vs uint(32|16|8)_t:
>
> i915$ git grep -E "uint(32|16|8)_t" | wc -l
> 852
> i915$ git grep -E "u(32|16|8)" | wc -l
> 3857

Doing a bit of git grep -cE with those seems to indicate that code with
uint(32|16|8)_t promotes *more* use of those. It's natural and it goes
by the rule of following the style surrounding your changes.

> I don't consider that undoable.

It's doable. Only a question of whether we want to do that or not.

> BIT() is in the minority at the moment, so it might benefit even more as
> people often cargo-cult the programming style from other places in the code.

FWIW I think BIT(n) is simply better than open-coding (1 << n), while
the kernel vs. C types is more of an aestheical thing.

> I think it might be worthy doing these two changes to get the automatic
> enforemend and avoid the codebase staying in limbo. Machine overlords are
> way better at enforcing any code checks than us humans.

Agreed on the machine overlords.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

On Thu, 01 Mar 2018, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> Does checkpatch support disabling checks or do you have to filter them
> out from the output?

Turns out it does. There's an --ignore option. For starters, I sent a
patch [1] to show the warning types in the output, so we can more
accurately discuss the ignores.

Alas it's probably not as simple as just slamming the ignores to dim:
I'll probably want to use the strictest set both when I'm developing and
applying patches (just to get an idea about the warnings, and I try to
keep my patches under 80 columns, etc.). Some other drivers and drm core
might have different sets of rules, and dim is no longer an Intel-only
tool. So we probably need to be able to specify different rule sets.

BR,
Jani.

PS. We need to get dim-tools and igt-dev etc. subscribed to the mail
archive and marc etc.


[1] id:20180302155800.6874-1-jani.nikula@intel.com



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Daniel Vetter]

On Thu, Mar 01, 2018 at 11:47:06AM +0200, Arkadiusz Hiler wrote:
> Hey all,
> 
> Since not so long ago our CI is running and reporting sparse and
> checkpatch. Sparse is doing just fine but I had to disable checkpatch
> for the time being - too much "false" positives causing people to
> complain. It's simply confusing to see one thing in the code, and
> fitting your change in only to get a report that it's wrong.
> 
> We are explicitly going against couple of the recommendations it tries
> to enforce, e.g. not using BIT macro, splitting quoted strings:
> https://lists.freedesktop.org/archives/intel-gfx/2018-February/156613.html
> 
> IMHO we should make a couple of decisions here:
>  1. Do we really want to use the checkpatch / have CI reports?
>  2. Which of the checkpatch checks we want to disabled for i915?
>  3. How strongly do we want to enforce the rest?
>  4. Do we want to change what's already in the tree, for compliance?
> 
> Recent-ish drm-tip, vanilla checkpatch on i915 reports:
> total: 399 errors, 3573 warnings, 209374 lines checked

I'd recommend not making checkpatch ever fail CI, but at most warning.
Plus silence the ones we obviously think are silly (or currently
inconsistent in our code).

The reason is that checkpatch isn't really maintained by a consensus
approach, but it's mostly just Joe Perches doing what he feels like. It's
better again, but in past kernel releases it has become opionated about
pointless bikesheds to the point I simply had to ignore most of it. It'd
be great if we have an authoritative answer to all code layout questions,
but in reality we don't.

I think the ingore list is probably best kept within maintainer-tools
itself, that way we at least have visibility into it from committers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

On Mon, 05 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
> I'd recommend not making checkpatch ever fail CI, but at most warning.

Agreed. But we want the automated warnings on the list, neutrally from a
bot instead of a developer spending time nitpicking this stuff. And the
committers should pay attention before pushing.

Really, everyone should be running checkpatch themselves locally before
sending patches, ignoring the irrelevant warnings with good taste...

> Plus silence the ones we obviously think are silly (or currently
> inconsistent in our code).
>
> I think the ingore list is probably best kept within maintainer-tools
> itself, that way we at least have visibility into it from committers.

Agreed, but as I wrote in [1] we need to add checkpatch profiles or
config or something, because I want *all* the warnings when I run it
locally. And if we decide to, say, enforce kernel types in i915 but
drm-misc decides otherwise, that's also another config.

BR,
Jani.


[1] http://mid.mail-archive.com/87zi3qtq9f.fsf@intel.com



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Arkadiusz Hiler]

On Mon, Mar 05, 2018 at 01:10:21PM +0200, Jani Nikula wrote:
> On Mon, 05 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
> > I'd recommend not making checkpatch ever fail CI, but at most warning.
> 
> Agreed. But we want the automated warnings on the list, neutrally from a
> bot instead of a developer spending time nitpicking this stuff. And the
> committers should pay attention before pushing.

We are never failing CI because of it. We are sending it simply as a
warning (if there's anything to report).

> Really, everyone should be running checkpatch themselves locally before
> sending patches, ignoring the irrelevant warnings with good taste...
> 
> > Plus silence the ones we obviously think are silly (or currently
> > inconsistent in our code).
> >
> > I think the ingore list is probably best kept within maintainer-tools
> > itself, that way we at least have visibility into it from committers.
> 
> Agreed, but as I wrote in [1] we need to add checkpatch profiles or
> config or something, because I want *all* the warnings when I run it
> locally. And if we decide to, say, enforce kernel types in i915 but
> drm-misc decides otherwise, that's also another config.
> 
> BR,
> Jani.
> 
> 
> [1] http://mid.mail-archive.com/87zi3qtq9f.fsf@intel.com

Good. CI is using dim and I want too keep it that way. I prefer a cmd
line switch over .dimrc. Keeping track of an additional file for the
builder would be an annoyance.

- Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Arkadiusz Hiler]

On Thu, Mar 01, 2018 at 11:17:50PM +0000, Chris Wilson wrote:
> Quoting Arkadiusz Hiler (2018-03-01 09:47:06)
> > Hey all,
> > 
> > Since not so long ago our CI is running and reporting sparse and
> > checkpatch. Sparse is doing just fine but I had to disable checkpatch
> > for the time being - too much "false" positives causing people to
> > complain. It's simply confusing to see one thing in the code, and
> > fitting your change in only to get a report that it's wrong.
> 
> Another aspect is that we use the kernel coding style for igt as well.
> checkpatch.pl should be able to pick up style issues on igt patches.
> -Chris

I was thinking the same. It should be doable with couple of ignores here
and there (e.g. complaining about new files and MAINTAINERS file).

I will get to it once we will have figured out how to checkpatch i915
properly.

- Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915 vs checkpatch

[Jani Nikula]

On Mon, 05 Mar 2018, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote:
> On Mon, Mar 05, 2018 at 01:10:21PM +0200, Jani Nikula wrote:
>> On Mon, 05 Mar 2018, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > I'd recommend not making checkpatch ever fail CI, but at most warning.
>> 
>> Agreed. But we want the automated warnings on the list, neutrally from a
>> bot instead of a developer spending time nitpicking this stuff. And the
>> committers should pay attention before pushing.
>
> We are never failing CI because of it. We are sending it simply as a
> warning (if there's anything to report).
>
>> Really, everyone should be running checkpatch themselves locally before
>> sending patches, ignoring the irrelevant warnings with good taste...
>> 
>> > Plus silence the ones we obviously think are silly (or currently
>> > inconsistent in our code).
>> >
>> > I think the ingore list is probably best kept within maintainer-tools
>> > itself, that way we at least have visibility into it from committers.
>> 
>> Agreed, but as I wrote in [1] we need to add checkpatch profiles or
>> config or something, because I want *all* the warnings when I run it
>> locally. And if we decide to, say, enforce kernel types in i915 but
>> drm-misc decides otherwise, that's also another config.
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] http://mid.mail-archive.com/87zi3qtq9f.fsf@intel.com
>
> Good. CI is using dim and I want too keep it that way. I prefer a cmd
> line switch over .dimrc. Keeping track of an additional file for the
> builder would be an annoyance.

To follow-up, I sent some patches to implement this [1].

BR,
Jani.

PS. The Mail Archive seems to be pretty slow at times, please use the
message-id if you can't find them.

[1] http://mid.mail-archive.com/20180313113010.13078-1-jani.nikula@intel.com

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx