Re: [PATCH] Documentation/i2c: direct users to i2c-tools regarding i2c/smbus.h
Hi Sam, Please don't include the same persons in To: and Cc:. It makes the message delivered twice to them. On Thu, 19 Apr 2018 15:23:56 -0700, Sam Hansen wrote: > The current examples reference i2c/smbus.h, which is the first reference > in Documentation/i2c/dev-interface to anything related to the i2c-tools > project. This moves the existing reference to i2c-tools up into the > C-example, directing the user to the project's git repository. > > Signed-off-by: Sam Hansen <hans...@google.com> > --- > Documentation/i2c/dev-interface | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/Documentation/i2c/dev-interface b/Documentation/i2c/dev-interface > index fbed645ccd75..ed20f7b7851a 100644 > --- a/Documentation/i2c/dev-interface > +++ b/Documentation/i2c/dev-interface > @@ -23,6 +23,10 @@ First, you need to include these two headers: >#include >#include > > +The i2c/smbus header file is provided by the i2c-tools project. For more > info > +about i2c-tools, see: > +https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/ > + > Now, you have to decide which adapter you want to access. You should > inspect /sys/class/i2c-dev/ or run "i2cdetect -l" to decide this. > Adapter numbers are assigned somewhat dynamically, so you can not > @@ -163,10 +167,6 @@ what happened. The 'write' transactions return 0 on > success; the > returns the number of values read. The block buffers need not be longer > than 32 bytes. > > -The above functions are made available by linking against the libi2c library, > -which is provided by the i2c-tools project. See: > -https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/. > - I'm fine with moving the pointer to i2c-tools up in the document. However I find it unfortunate to remove the only reference to libi2c. Typically, distributions will only include the tools in i2c-tools, and they are not relevant in this case. The library will be in a separate package (libi2c or similar), and the header file in yet another package (libi2c-devel or similar.) Although the exact package names are distribution specific, it would be convenient to hint the reader at which packages he is supposed to install. You could also mention that the C program must be compiled with -li2c. > > Implementation details > == -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/i2c: sync docs with current state of i2c-tools.
Hi Wolfram, Sam, On Fri, 13 Apr 2018 00:24:57 +0200, Wolfram Sang wrote: > On Thu, Apr 12, 2018 at 02:33:42PM -0700, Sam Hansen wrote: > > Currently, Documentation/i2c/dev-interface describes the use of i2c_smbus_* > > helper routines as static inlined functions provided by linux/i2c-dev.h. > > Work > > has been done to refactor the linux/i2c-dev.h file in the i2c-tools project > > out into its own library. As a result, these docs have become stale. > > Thanks for fixing this! > > > This patch corrects the discrepancy and directs the reader to the i2c-tools > > project for more information. Additionally, some trailing-whitespace > > cleanups > > were made. > > Minor nit: Having the whitespace changes in a seperate patch is a tad > easier to review. > > > - /* Using I2C Write, equivalent of > > + /* Using I2C Write, equivalent of > > i2c_smbus_write_word_data(file, reg, 0x6543) */ > > Maybe change to Kernel coding style comments while here? > > > - Not meant to be called directly; instead, use the access functions > > - below. > > + If possible, use the provided i2c_smbus_* methods described below in > > favor > > + of issuing direct ioctls. > > Why this change? I'm also not sure if "in favor of" is right. "instead of" would sound better to me, but I'm no native English speaker, I could be wrong. > > -The above functions are all inline functions, that resolve to calls to > > -the i2c_smbus_access function, that on its turn calls a specific ioctl > > -with the data in a specific format. Read the source code if you > > -want to know what happens behind the screens. > > +The above functions are made available by linking against the libi2c > > library, > > +which is provided by the i2c-tools project. See: > > +https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/. > > This is fine with me. Maybe Jean has a comment on this? Fixing the documentation is always welcome, thanks Sam for stepping in. However we really want separate patches for whitespace fixes and actual contents change, as Wolfram already mentioned above. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "CodingStyle: Clarify and complete chapter 7" in docs-next
On Thu, 22 Sep 2016 10:49:47 -0700, Joe Perches wrote: > On Thu, 2016-09-22 at 13:57 +0200, Jean Delvare wrote: > > Sure. But I'm afraid you keep changing topics and I have no idea where > > you are going. We started with "should there be a space before jump > > labels", then out of nowhere we were discussing the wording of the > > output of checkpatch (how is that related?) and now you pull statistics > > out of your hat, like these numbers imply anything. > > No, not out of a hat. Those are the results of a silly script that > runs checkpatch on every .[ch] kernel file (but not tools/) with: > > --show-types --terse --emacs --strict --no-summary --quiet -f Silly is the key word here. Just don't do it. > The magnitude of "ERRORS" is high and it's not necessary or useful > to modify old or obsolete code just to reduce that magnitude. I agree. Just don't do it. > > checkpatch was called checkPATCH for a reason. > > That's why I promote the --force option to limit using checkpatch on > files outside of staging. > > https://patchwork.kernel.org/patch/9332205/ > > Andrew? Are you going to apply that one day? I hope not. Looks plain wrong to me. This wont prevents idiots from being idiots. All it does is make things more difficult for the rest of us. > > ERROR means that the new code isn't allowed to do that. Period. > > Disagree. The compiler doesn't care. Which is good, because this has nothing to do with the compiler. > The value of consistency in reducing defects is very hard to quantify. That's not the only purpose of consistency. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "CodingStyle: Clarify and complete chapter 7" in docs-next
On Thu, 22 Sep 2016 14:11:03 +0100, Al Viro wrote: > On Thu, Sep 22, 2016 at 01:57:58PM +0200, Jean Delvare wrote: > > > > > > MUST is much stronger language than I would prefer. > > > > That's what error means, really. When your compiler fails with an > > error, you have no choice but to fix your code. Warnings on the other > > hand may be ignored sometimes. > > And they are errors, because of...? Because we decided they were important. Of course there is some subjectivity to this decision, much like all gcc warnings could be errors and some errors could be warnings. Which is why gcc allows enabling/disabling individually. I assume the current set of errors vs. warnings is the result of some consensus amongst kernel developers. It can certainly be adjusted if the consensus changes for whatever reason. > > Sure. But I'm afraid you keep changing topics and I have no idea where > > you are going. We started with "should there be a space before jump > > labels", then out of nowhere we were discussing the wording of the > > output of checkpatch (how is that related?) and now you pull statistics > > out of your hat, like these numbers imply anything. > > > > checkpatch was called checkPATCH for a reason. It's main intent was to > > prevent NEW (coding-style mostly) errors from creeping into the kernel. > > The fact that old code does now always follow these recommendations is > > unfortunate but that doesn't make checkpatch wrong or bad. > > > > ERROR means that the new code isn't allowed to do that. Period. > > The main intent of checkpatch these days appears to be providing an easy > way of thoughtless inflation of commit counts, everything else be damned. > Make-work, in other words. Yes, I've noticed the trend too :-( But that's a problem with the people using the tool, mostly, not with the tool itself. > The _only_ criterion for adding new checks should be a strong consensus in > the core kernel. IOW, it should be descriptive, not prescriptive. "Some > people do it this way, some - that" is not a valid reason for "let's make it > uniform; that way is just better, so from now on it's a new requirement". > Especially when the rationale behind the choice has all the intellectual > rigour of feng shui. Some of these checks are common-sense, some are > absolutely arbitrary, there are far too many of them (...) That I agree with, I've complained about the same before. But that doesn't mean checkpatch is not a wonderfully useful tool. It really is, if used properly. > and elevating them > to the level of compiler errors like you seem to do is rather dishonest. I see. Well clearly this discussion isn't going anywhere so I'll go back working on things that really matter. See you. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "CodingStyle: Clarify and complete chapter 7" in docs-next
Hi Jani, On Thu, 22 Sep 2016 13:43:42 +0300, Jani Nikula wrote: > On Thu, 22 Sep 2016, Jean Delvare <jdelv...@suse.de> wrote: > > You need to think in terms of actual use cases. Who uses checkpatch and > > why? I think there are 3 groups of users: > > * Beginners. They won't run the script by themselves, instead they will > > submit a patch which infringes a lot of coding style rules, and the > > maintainer will point them to checkpatch and ask for a resubmission > > which makes checkpatch happy. Being beginners, they can only rely on > > the script itself to only report things which need to be fixed, by > > default. > > * Experienced developers. Who simply want to make sure they did not > > overlook anything before they post their work for review. They have > > the knowledge to decide if they want to ignore some of the warnings. > > * People with too much spare time, looking for anything they could > > "contribute" to the kernel. They will use --subjective and piss off > > every maintainer they can find. > > > > Sadly there's not much we can do about the third category, short of > > killing option --subjective altogether. > > You could make checkpatch have different defaults for patches and files, > to encourage better style in new code, but to discourage finding > problems in existing code. Fixing old code isn't wrong per se. It's good actually. But only if done the right way by the right person. I don't think it makes any sense to use this task as an introduction to kernel development for newcomers. It doesn't teach them anything about the kernel, really. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "CodingStyle: Clarify and complete chapter 7" in docs-next
On Thu, 22 Sep 2016 03:42:10 -0700, Joe Perches wrote: > On Thu, 2016-09-22 at 11:24 +0200, Jean Delvare wrote: > > I would rather suggest: > > > > ERROR -> MUST_FIX > > WARNING -> SHOULD_FIX > > CHECK -> MAY_FIX > > MUST is much stronger language than I would prefer. That's what error means, really. When your compiler fails with an error, you have no choice but to fix your code. Warnings on the other hand may be ignored sometimes. > There are still about a quarter million ERRORs just for > spacing issues in the kernel tree. > > Here are the top 10 ERROR checkpatch messages treewide as of > a few days ago, > > $ grep ERROR checkpatch.short_sorted_20160917 > 268308 ERROR:SPACING > 37340 ERROR:CODE_INDENT > 27678 ERROR:TRAILING_WHITESPACE > 21024 ERROR:COMPLEX_MACRO > 14048 ERROR:POINTER_LOCATION > 12207 ERROR:TRAILING_STATEMENTS > 11079 ERROR:OPEN_BRACE >6802 ERROR:ASSIGN_IN_IF >3940 ERROR:RETURN_PARENTHESES >2322 ERROR:NON_OCTAL_PERMISSIONS > > Maybe there could be some better classifications of the various > messages. > > But there are about two million checkpatch messages overall in > the kernel tree. > > That's a lot. Sure. But I'm afraid you keep changing topics and I have no idea where you are going. We started with "should there be a space before jump labels", then out of nowhere we were discussing the wording of the output of checkpatch (how is that related?) and now you pull statistics out of your hat, like these numbers imply anything. checkpatch was called checkPATCH for a reason. It's main intent was to prevent NEW (coding-style mostly) errors from creeping into the kernel. The fact that old code does now always follow these recommendations is unfortunate but that doesn't make checkpatch wrong or bad. ERROR means that the new code isn't allowed to do that. Period. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: Remove space-before-label guidance from CodingStyle
Hi Jonathan, On Wed, 21 Sep 2016 15:54:01 -0600, Jonathan Corbet wrote: > Recent discussion has made it clear that there is no community consensus > on this particular rule. Remove it now, lest it inspire yet another set > of unwanted "cleanup" patches. > > This partially reverts 865a1caa4b6b (CodingStyle: Clarify and complete > chapter 7). > (...) > @@ -481,7 +478,7 @@ The rationale for using gotos is: > goto out_buffer; > } > ... > - out_free_buffer: > + out_free_buffer: > kfree(buffer); > return result; > } > @@ -490,7 +487,7 @@ A common type of bug to be aware of is ``one err > bugs`` which look like this: > .. code-block:: c > > - err: > + err: > kfree(foo->bar); > kfree(foo); > return ret; There are 2 more occurrences after this point, which you probably want to change too. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "CodingStyle: Clarify and complete chapter 7" in docs-next
Hi Joe, On Mon, 19 Sep 2016 23:32:03 -0700, Joe Perches wrote: > On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote: > > I think it is better to be clear. CHECK was never really clear to me, > > especially if you see it in isolation, on a file that doesn't also have > > ERROR or WARNING. NITS is a common word in this context, but not FLEAS > > and GNATS, as far as I know. > > There could also be a severity level: high medium and low > > I agree clarity is good. > > The seriousness with which some beginners take these message > types though is troublesome, You need to think in terms of actual use cases. Who uses checkpatch and why? I think there are 3 groups of users: * Beginners. They won't run the script by themselves, instead they will submit a patch which infringes a lot of coding style rules, and the maintainer will point them to checkpatch and ask for a resubmission which makes checkpatch happy. Being beginners, they can only rely on the script itself to only report things which need to be fixed, by default. * Experienced developers. Who simply want to make sure they did not overlook anything before they post their work for review. They have the knowledge to decide if they want to ignore some of the warnings. * People with too much spare time, looking for anything they could "contribute" to the kernel. They will use --subjective and piss off every maintainer they can find. Sadly there's not much we can do about the third category, short of killing option --subjective altogether. The default settings should work out of the box for for the first category. > Maybe prefix various different types of style messages. > > Something like: > > ERROR -> CODE_STYLE_DEFECT > WARNING -> CODE_STYLE_UNPREFERRED > CHECK -> CODE_STYLE_NIT I don't think you clarify anything with these changes, as they do not carry the requirement from a submitter's perspective. If you really want to change the names, I would rather suggest: ERROR -> MUST_FIX WARNING -> SHOULD_FIX CHECK -> MAY_FIX Or explain the categories in plain English, see below. > I doubt additional external documentation would help much. I agree. If anything needs to be explained, it should be included in the output of checkpatch directly. For example, if any ERROR was printed, include the following after the count summary: "ERROR means the code is infringing a core coding style rule. This MUST be fixed before the patch is submitted upstream." And if any WARNING was printed, include the following: "WARNING means the code is not following the best practice. This SHOULD be fixed before the patch is submitted upstream, unless the maintainer agreed otherwise." Not sure if we need something for CHECK, as these messages are not printed by default so I'd assume people who get them know what they asked for. But apparently these confused Julia so maybe a similar explanation would be needed for them too. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/17] SubmitChecklist: rename to RST and add to the development-proccess book
On Wed, 14 Sep 2016 20:05:52 -0300, Mauro Carvalho Chehab wrote:
> Add it to the development-process book, by renaming it to
> SubmitChecklist.rst and move it to the development-process
> directory.
>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---
> .../{SubmitChecklist => development-process/SubmitChecklist.rst} | 2 +-
> Documentation/development-process/index.rst | 1 +
> Documentation/hwmon/submitting-patches| 2 +-
> Documentation/ja_JP/SubmitChecklist | 4
> ++--
> Documentation/ja_JP/SubmittingPatches | 2 +-
> Documentation/zh_CN/SubmittingPatches | 2 +-
> 6 files changed, 7 insertions(+), 6 deletions(-)
> rename Documentation/{SubmitChecklist =>
> development-process/SubmitChecklist.rst} (98%)
> (...)
For the hwmon part:
Reviewed-by: Jean Delvare <jdelv...@suse.de>
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] CodingStyle.rst: use the proper tag for verbatim font
On Tue, 13 Sep 2016 16:24:48 -0300, Mauro Carvalho Chehab wrote: > Em Tue, 13 Sep 2016 19:45:23 +0200 > Jean Delvare <jdelv...@suse.de> escreveu: > > > Hi Mauro, > > > > On Mon, 12 Sep 2016 11:47:57 -0300, Mauro Carvalho Chehab wrote: > > > Instead of using "foo", use ``foo`` for the names that are > > > literal. > > > > Because...? > > Because it is an usual typographic convention to use monospaced > fonts for functions and language commands on documents. > > On Sphinx/ReST, ``foo`` means that foo will be displayed using a > monospaced font. My point was that such an explanation should be part of your patch description, so that the change can be accepted (is the reviewer agrees with your reason) or discussed (if he/she doesn't.) And I do agree with the change, for what it's worth :-) Thanks, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] CodingStyle.rst: use the proper tag for verbatim font
Hi Mauro, On Mon, 12 Sep 2016 11:47:57 -0300, Mauro Carvalho Chehab wrote: > Instead of using "foo", use ``foo`` for the names that are > literal. Because...? > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> > --- > Documentation/kernel-development/CodingStyle.rst | 98 > > 1 file changed, 49 insertions(+), 49 deletions(-) > > diff --git a/Documentation/kernel-development/CodingStyle.rst > b/Documentation/kernel-development/CodingStyle.rst > index 3bc022b43ed8..4b2ba1008b11 100644 > --- a/Documentation/kernel-development/CodingStyle.rst > +++ b/Documentation/kernel-development/CodingStyle.rst > @@ -38,8 +38,8 @@ benefit of warning you when you're nesting your functions > too deep. > Heed that warning. > > The preferred way to ease multiple indentation levels in a switch statement > is > -to align the "switch" and its subordinate "case" labels in the same column > -instead of "double-indenting" the "case" labels. E.g.: > +to align the ``switch`` and its subordinate ``case`` labels in the same > column > +instead of ``double-indenting`` the ``case`` labels. E.g.: > > .. code-block:: c > (...) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cpufreq-stats: Minor documentation fix
On Fri, 9 Sep 2016 00:38:59 +0200, Rafael J. Wysocki wrote:
> On 9/8/2016 1:20 PM, Viresh Kumar wrote:
> > On 08-09-16, 12:39, Jean Delvare wrote:
> >> The cpufreq-stats code can no longer be built as a module, so it now
> >> appears with square brackets in menuconfig.
> >>
> >> Signed-off-by: Jean Delvare <jdelv...@suse.de>
> >> Fixes: 1aefc75b2449 ("cpufreq: stats: Make the stats code non-modular")
> >> Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> >> Cc: Viresh Kumar <viresh.ku...@linaro.org>
> >> ---
> >> Documentation/cpu-freq/cpufreq-stats.txt |2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> --- linux-4.8-rc5.orig/Documentation/cpu-freq/cpufreq-stats.txt
> >> 2016-09-04 23:31:46.0 +0200
> >> +++ linux-4.8-rc5/Documentation/cpu-freq/cpufreq-stats.txt 2016-09-08
> >> 11:34:34.805606601 +0200
> >> @@ -103,7 +103,7 @@ Config Main Menu
> >>Power management options (ACPI, APM) --->
> >>CPU Frequency scaling --->
> >>[*] CPU Frequency scaling
> >> - <*> CPU frequency translation statistics
> >> + [*] CPU frequency translation statistics
> >>[*] CPU frequency translation statistics details
> > Acked-by: Viresh Kumar <viresh.ku...@linaro.org>
> >
> Applied, but please CC PM material to linux-pm too. That makes it much
> easier to handle.
OK. MAINTAINERS didn't tell me so, I'll send an update in a minute.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cpufreq-stats: Minor documentation fix
The cpufreq-stats code can no longer be built as a module, so it now
appears with square brackets in menuconfig.
Signed-off-by: Jean Delvare <jdelv...@suse.de>
Fixes: 1aefc75b2449 ("cpufreq: stats: Make the stats code non-modular")
Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Cc: Viresh Kumar <viresh.ku...@linaro.org>
---
Documentation/cpu-freq/cpufreq-stats.txt |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-4.8-rc5.orig/Documentation/cpu-freq/cpufreq-stats.txt 2016-09-04
23:31:46.0 +0200
+++ linux-4.8-rc5/Documentation/cpu-freq/cpufreq-stats.txt 2016-09-08
11:34:34.805606601 +0200
@@ -103,7 +103,7 @@ Config Main Menu
Power management options (ACPI, APM) --->
CPU Frequency scaling --->
[*] CPU Frequency scaling
- <*> CPU frequency translation statistics
+ [*] CPU frequency translation statistics
[*] CPU frequency translation statistics details
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()
Hi Peter, On Mon, 5 Sep 2016 13:58:38 +0200, Peter Zijlstra wrote: > On Mon, Sep 05, 2016 at 01:54:45PM +0200, Jean Delvare wrote: > > On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote: > > > I have it in my local .gitconfig, and recommend it to people who send me > > > patches. > > > > What does it look like, please? > > [diff "default"] > xfuncname = "^[[:alpha:]$_].*[^:]$" OK, I see. As mentioned somewhere else, it fails for labels which have comments. I was also surprised by the $ but apparently it's valid in identifiers for at least some incarnations of C o.O My worry is that you recommending it to contributors on a individual and opportunity basis, doesn't scale. Basing coding style recommendations on a personal quirk doesn't strike me as the best idea ever in the long run. The reason why I proposed an update to CodingStyle regarding this topic was precisely to avoid having to repeat the same to contributors, like you do (although our recommendations are different.) While looking at the syntax of your example, I have found something which looks more promising. git already has predefined xfuncname definitions for various languages, including C. These can be enabled based on file name patterns via gitattributes. The following .gitattribute file placed at the root of the kernel source tree achieves what you want: *.c diff=cpp *.h diff=cpp The major difference between git config and gitattributes is that the latter can be part of the project itself, just like gitignore. So we could just push that .gitattribute file upstream, and then labels without leading spaces would no longer be a problem, at least within git. It would still be a problem for me as an inveterate quilt user, at least until GNU diff gets "fixed." Which I did not even try, as I'm not sure if upstream really considers this a bug in the first place. And just for completeness, git's "cpp" predefined pattern doesn't actually support $ as part of identifiers. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()
On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote: > On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote: > > Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter > > Zijlstra reportedly changed the behavior of "diff -p" so that it > > handles unindented C labels nicely. If this actually happens, it could > > change my point of view. However I can't find this commit in upstream > > diffutils. Peter, can you please clarify the situation? Is it just a > > local hack on your own instance of "diff"? > > I have it in my local .gitconfig, and recommend it to people who send me > patches. What does it look like, please? > I've never tried to get diffutils fixed, although maybe I should. If you don't want one-space-indented labels to become the norm, then yes you should. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()
Hi Daniel, Preliminary note: the SNR of Markus Elfring is incredibly low. I advise you just ignore him. On Sun, 04 Sep 2016 11:56:58 +0200, Daniel Borkmann wrote: > I don't want to drag this thread onwards for (way) too long, but clearly "it > is > advised to indent labels with a single space (not tab)" (from diff in above > commit) > doesn't really reflect the majority of kernel practice we have in-tree today > and > actually rather adds more confusion than any clarification whatsoever: > >$ git grep -n "^\ [a-z_]*:" -- '*.[ch]' | wc -l >4919 >$ git grep -n "^[a-z_]*:" -- '*.[ch]' | wc -l >54686 Well the documentation update in question has not hit mainline yet, so it's not really surprising. > A CodingStyle document should document what's regarded as a general consensus > of > kernel coding practices, and thus should represent the /majority/ of coding > style, I beg to disagree. Recommendations are not meant to document what people are currently doing but what we think they should be doing. By your reasoning, we would have killed all the devm infrastructure, because at some point in time (and it might still be the case) most drivers were not using it. There is a rationale for the leading space, it is given in the patch, but sadly you decided to not quote it above. > which (if I didn't screw up my git-grep line completely) above 9% does not > really > reflect at all. So, new folks starting with kernel hacking reading this are > rather Your grep patterns are slightly inaccurate. Space doesn't need to be escaped, labels can use capital letters, and except for the first character, digits are accepted too. Also to be completely fair, you should also count the labels which are intended using tabs. But it doesn't change the balance noticeably anyway, so no big deal. > misguided, and code-wise it just adds up to have more inconsistencies from new > patches, or worse, have noisy patches (like this one) flying around that try > to > brute-force everything into this advice. Guys who like to waste our time with pointless patches will always find a way to do that, sadly, so I don't think this point is relevant. The acceptance of an optional single space before labels dates back to at least June 2007, as supported by the very first incarnation of checkpatch.pl. So nothing really new here, except for a preference (my preference, admittedly, but I'm know I'm not alone) being expressed in the coding style document. My assumption was that the behavior of "diff -p" would never change, as despite the language-specificity of its long option name, it seemed too generic to be loaded with C-specific rules. Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter Zijlstra reportedly changed the behavior of "diff -p" so that it handles unindented C labels nicely. If this actually happens, it could change my point of view. However I can't find this commit in upstream diffutils. Peter, can you please clarify the situation? Is it just a local hack on your own instance of "diff"? Even if upstream diff is ever changed, it will take some time until new versions propagate to all developers. And until this happens, my preference for one-space-indented labels will remain. Also git has its own implementation of "diff", so any change in the behavior of GNU diff's -p and/or --show-c-function options should be reflected there as well for consistency. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: staging: ks7010: Rename jump labels
Hi Markus, On Thu, 21 Jul 2016 22:11:53 +0200, SF Markus Elfring wrote: > >> How do you generally think about jump label renaming? > > > > Renaming from "out0:", "out1:" etc to something meaningful, yes. > > I suggest to take another look at such identifiers. > > Would you like to support the renaming of a label like "error_out1" > (in the function "ks7010_upload_firmware" for example)? They should be renamed too. Anything using numbers instead of explicit labels should be updated. I included the reasons in the patch I just sent, hopefully the documentation is clearer now. > Will the software evolution be continued also with information from the topic > "Source code review around jump label usage"? > https://lkml.org/lkml/2015/12/11/378 > http://article.gmane.org/gmane.linux.kernel/2106190 Personally I see no value in such statistics. Either labels are wrong (either wrong indentation or wrong name) and should be fixed, or they are correct and you should not touch them. Whether the same label name is used somewhere else is irrelevant. Labels are local by nature, so uniqueness isn't a goal at all, only correctness. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] CodingStyle: Clarify and complete chapter 7
Chapter 7 (Centralized exiting of functions) of the coding style documentation is unclear at times, and lacks some information (such as the possibility to indent labels with a single space.) Clarify and complete it. Signed-off-by: Jean Delvare <jdelv...@suse.de> Cc: Markus Elfring <elfr...@users.sourceforge.net> Cc: Jonathan Corbet <cor...@lwn.net> --- Documentation/CodingStyle | 25 +++-- 1 file changed, 19 insertions(+), 6 deletions(-) --- linux-4.7-rc7.orig/Documentation/CodingStyle2016-07-04 08:01:00.0 +0200 +++ linux-4.7-rc7/Documentation/CodingStyle 2016-07-25 14:25:12.498328631 +0200 @@ -396,9 +396,13 @@ locations and some common work such as c cleanup needed then just return directly. Choose label names which say what the goto does or why the goto exists. An -example of a good name could be "out_buffer:" if the goto frees "buffer". Avoid -using GW-BASIC names like "err1:" and "err2:". Also don't name them after the -goto location like "err_kmalloc_failed:" +example of a good name could be "out_free_buffer:" if the goto frees "buffer". +Avoid using GW-BASIC names like "err1:" and "err2:", as you would have to +renumber them if you ever add or remove exit paths, and they make correctness +difficult to verify anyway. + +It is advised to indent labels with a single space (not tab), so that +"diff -p" does not confuse labels with functions. The rationale for using gotos is: @@ -425,20 +429,29 @@ The rationale for using gotos is: goto out_buffer; } ... - out_buffer: +out_free_buffer: kfree(buffer); return result; } A common type of bug to be aware of is "one err bugs" which look like this: - err: +err: kfree(foo->bar); kfree(foo); return ret; The bug in this code is that on some exit paths "foo" is NULL. Normally the -fix for this is to split it up into two error labels "err_bar:" and "err_foo:". +fix for this is to split it up into two error labels "err_free_bar:" and +"err_free_foo:": + +err_free_bar: + kfree(foo->bar); +err_free_foo: + kfree(foo); + return ret; + +Ideally you should simulate errors to test all exit paths. Chapter 8: Commenting -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support
On Mon, 18 Jul 2016 17:59:02 +0200, Benjamin Tissoires wrote: > On Jul 18 2016 or thereabouts, Jean Delvare wrote: > > You provide stubs for SMBus Host Notify support if CONFIG_I2C_SMBUS is > > not selected. There are no such stubs for SMBus Alert support, for which > > I assumed drivers would select I2C_SMBUS if they have support. Which is > > what you are actually doing for i2c-i801 in a latter patch. > > > > Is there any reason for this difference? For consistency I'd rather > > provide stubs for all or none. My preference being for none, unless you > > have a use case which requires them. > > Looks like you are right. There is no need for the stubs and they can be > dropped. I think I had them in the first place for a previous > implementation, and they just stayed here. > > Given that you already sent a few cleanup patches, do you want to send > this fix also, or do you expect me to send it? (I don't think there will > be a conflict, so either is fine). I can do it, I just wanted to make sure it was OK with you first. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support
Hi Benjamin, Wolfram,
Now that I have reviewed the i2c-i801 part of the implementation, I'm
wondering...
On Thu, 9 Jun 2016 16:53:48 +0200, Benjamin Tissoires wrote:
> +/**
> + * i2c_setup_smbus_host_notify - Allocate a new smbus_host_notify for the
> given
> + * I2C adapter.
> + * @adapter: the adapter we want to associate a Host Notify function
> + *
> + * Returns a struct smbus_host_notify pointer on success, and NULL on
> failure.
> + * The resulting smbus_host_notify must not be freed afterwards, it is a
> + * managed resource already.
> + */
> +struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter
> *adap)
> +{
> + struct smbus_host_notify *host_notify;
> +
> + host_notify = devm_kzalloc(>dev, sizeof(struct smbus_host_notify),
> +GFP_KERNEL);
> + if (!host_notify)
> + return NULL;
> +
> + host_notify->adapter = adap;
> +
> + spin_lock_init(_notify->lock);
> + INIT_WORK(_notify->work, smbus_host_notify_work);
Here we initialize a workqueue.
> +
> + return host_notify;
> +}
> +EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
> +
> +/**
> + * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
> + * I2C client.
> + * @host_notify: the struct host_notify attached to the relevant adapter
> + * @data: the Host Notify data which contains the payload and address of the
> + * client
> + * Context: can't sleep
> + *
> + * Helper function to be called from an I2C bus driver's interrupt
> + * handler. It will schedule the Host Notify work, in turn calling the
> + * corresponding I2C device driver's alert function.
> + *
> + * host_notify should be a valid pointer previously returned by
> + * i2c_setup_smbus_host_notify().
> + */
> +int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> + unsigned short addr, unsigned int data)
> +{
> + unsigned long flags;
> + struct i2c_adapter *adapter;
> +
> + if (!host_notify || !host_notify->adapter)
> + return -EINVAL;
> +
> + adapter = host_notify->adapter;
> +
> + spin_lock_irqsave(_notify->lock, flags);
> +
> + if (host_notify->pending) {
> + spin_unlock_irqrestore(_notify->lock, flags);
> + dev_warn(>dev, "Host Notify already scheduled.\n");
> + return -EBUSY;
> + }
> +
> + host_notify->payload = data;
> + host_notify->addr = addr;
> +
> + /* Mark that there is a pending notification and release the lock */
> + host_notify->pending = true;
> + spin_unlock_irqrestore(_notify->lock, flags);
> +
> + return schedule_work(_notify->work);
And here we use it.
> +}
> +EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
But what happens on i2c_adapter removal? What prevents the following
sequence from happening?
1* A Host Notify event happens.
2* The event is handled and queued by i2c_handle_smbus_host_notify().
3* Someone tears down the underlying i2c_adapter (for example "rmmod
i2c-i801".)
4* The workqueue is processed, accessing memory which has already been
freed.
Of course it would be back luck, but that's pretty much the definition
of a race condition ;-)
To be on the safe side, don't we need a teardown function in i2c-smbus,
that could be called before i2c_del_adapter, which would remove the
host notify handle and flush the workqueue?
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/4] i2c-smbus: add SMBus Host Notify support
[As it doesn't look like this message was delivered, I am sending it
again. I apologize if this is a duplicate for some of you.]
Hi Benjamin, Wolfram,
Sorry for being late to the party. I finally found some time to look at
the patches. Looks good overall, with just two minor comments:
On jeu., 2016-06-09 at 16:53 +0200, Benjamin Tissoires wrote:
> SMBus Host Notify allows a slave device to act as a master on a bus to
> notify the host of an interrupt. On Intel chipsets, the functionality
> is directly implemented in the firmware. We just need to export a
> function to call .alert() on the proper device driver.
>
> i2c_handle_smbus_host_notify() behaves like i2c_handle_smbus_alert().
> When called, it schedules a task that will be able to sleep to go through
> the list of devices attached to the adapter.
>
> The current implementation allows one Host Notification to be scheduled
> while an other is running.
>
> Tested-by: Andrew Duggan <adug...@synaptics.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>
> ---
> (...)
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 3b6765a..f574995 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> (...)
> +/**
> + * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
> + * I2C client.
> + * @host_notify: the struct host_notify attached to the relevant adapter
> + * @data: the Host Notify data which contains the payload and address of the
> + * client
Doesn't look correct. The data parameter doesn't contain the address,
that would be in the (undocumented) address parameter. I'll send a
patch.
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index 8f1b086..4ac95bb 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> (...)
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter
> *adap);
> +int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> + unsigned short addr, unsigned int data);
> +#else
> +static inline struct smbus_host_notify *
> +i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> +{
> + return NULL;
> +}
> +
> +static inline int
> +i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> + unsigned short addr, unsigned int data)
> +{
> + return 0;
> +}
> +#endif /* I2C_SMBUS */
You provide stubs for SMBus Host Notify support if CONFIG_I2C_SMBUS is
not selected. There are no such stubs for SMBus Alert support, for which
I assumed drivers would select I2C_SMBUS if they have support. Which is
what you are actually doing for i2c-i801 in a latter patch.
Is there any reason for this difference? For consistency I'd rather
provide stubs for all or none. My preference being for none, unless you
have a use case which requires them.
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
