Le 28/02/2022 à 11:56, Petr Mladek a écrit :

On Fri 2022-02-25 16:49:31, Christophe Leroy wrote:

quoted

Le 25/02/2022 à 10:34, Petr Mladek a écrit :

quoted

Please do not do these small coding style changes. It complicates the
review and increases the risk of regressions. Different people
have different preferences. Just imagine that every half a year
someone update style of a code by his personal preferences. The
real changes will then get lost in a lot of noise.

I disagree here. We are not talking about people's preference here but
compliance with documented Linux kernel Codying Style and handling of
official checkpatch.pl script reports.

Really?

1. I restored

	+	if (mod->klp_info->secstrings == NULL) {

    and checkpatch.pl is happy.

On mainline's kernel/module.c checkpatch.pl tells me:

CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#2092: FILE: kernel/module.c:2092:
+	if (mod->klp_info->secstrings == NULL) {

2. I do not see anythinkg about if (xxx == NULL) checks in
    Documentation/process/coding-style.rst

3. $> git grep "if (.* == NULL" | wc -l
    15041

Commit b75ac618df75 ("checkpatch: add --strict "pointer comparison to 
NULL" test")

4. The result of
	-	mod->klp_info->sechdrs[symndx].sh_addr = \
	-		(unsigned long) mod->core_kallsyms.symtab;
	+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;

    is 90 characeters long and Documentation/process/coding-style.rst says:

Probably a misinterpretation of:

WARNING: Avoid unnecessary line continuations
#2107: FILE: kernel/module.c:2107:
+	mod->klp_info->sechdrs[symndx].sh_addr = \

	2) Breaking long lines and strings
	----------------------------------

	Coding style is all about readability and maintainability using commonly
	available tools.

	The preferred limit on the length of a single line is 80 columns.

	Statements longer than 80 columns should be broken into sensible chunks,
	unless exceeding 80 columns significantly increases readability and does
	not hide information.

    checkpatch.pl accepts lines up to 100 columns but 80 are still
    preferred.

quoted

You are right that randomly updating the style every half a year would
be a nightmare and would kill blamability of changes.

However when moving big peaces of code like this, blamability is broken
anyway and this is a very good opportunity to increase compliance of
kernel code to its own codying style. But doing it in several steps
increases code churn and has no real added value.

 From Documentation/process/submitting-patches.rst:

	One significant exception is when moving code from one file to
	another -- in this case you should not modify the moved code at all in
	the same patch which moves it.  This clearly delineates the act of
	moving the code and your changes.  This greatly aids review of the
	actual differences and allows tools to better track the history of
	the code itself.

quoted

quoted

Coding style changes might be acceptable only when the code is
reworked or when it significantly improves readability.

When code is moved around it is also a good opportunity.

No!

By the way some maintainers require checkpatch' clean patches even when 
this is only code move. I remember being requested to do that in the 
past, so now I almost always do it with my own patches.

I would not have complained if it did not complicate my review.
But it did!

Reviewing partial code move is not easy anyway, git is not very 
userfriendly with that.

Christophe