Re: [Intel-gfx] i915 vs checkpatch
On Mon, 05 Mar 2018, Arkadiusz Hilerwrote: > On Mon, Mar 05, 2018 at 01:10:21PM +0200, Jani Nikula wrote: >> On Mon, 05 Mar 2018, Daniel Vetter 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] 87zi3qtq9f.fsf@intel.com">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] 20180313113010.13078-1-jani.nikula@intel.com">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
Re: [Intel-gfx] i915 vs checkpatch
On Thu, Mar 01, 2018 at 11:17:50PM +, 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: [Intel-gfx] i915 vs checkpatch
On Mon, Mar 05, 2018 at 01:10:21PM +0200, Jani Nikula wrote: > On Mon, 05 Mar 2018, Daniel Vetterwrote: > > 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] 87zi3qtq9f.fsf@intel.com">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: [Intel-gfx] i915 vs checkpatch
On Mon, 05 Mar 2018, Daniel Vetterwrote: > 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] 87zi3qtq9f.fsf@intel.com">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: [Intel-gfx] i915 vs checkpatch
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: [Intel-gfx] i915 vs checkpatch
On Thu, 01 Mar 2018, Jani Nikulawrote: > 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.nik...@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: [Intel-gfx] i915 vs checkpatch
On Fri, 02 Mar 2018, Joonas Lahtinenwrote: > 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 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: [Intel-gfx] i915 vs checkpatch
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 Hilerwrote: > > > 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: [Intel-gfx] i915 vs checkpatch
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: [Intel-gfx] i915 vs checkpatch
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 Hilerwrote: > > 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 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-to...@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: [Intel-gfx] i915 vs checkpatch
I went through the recent checkpatch reports, and here's my take. On Thu, 01 Mar 2018, Arkadiusz Hilerwrote: > 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 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: [Intel-gfx] i915 vs checkpatch
On Thu, Mar 01, 2018 at 12:43:22PM +0200, Jani Nikula wrote: > On Thu, 01 Mar 2018, Arkadiusz Hilerwrote: > > 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-to...@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: [Intel-gfx] i915 vs checkpatch
On Thu, 01 Mar 2018, Arkadiusz Hilerwrote: > 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
[Intel-gfx] i915 vs checkpatch
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