Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Feb 2022 14:05:16 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     Aaron Tomlin <atomlin@redhat.com>,
        "mcgrof@kernel.org" <mcgrof@kernel.org>,
        "cl@linux.com" <cl@linux.com>, "mbenes@suse.cz" <mbenes@suse.cz>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "jeyu@kernel.org" <jeyu@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "void@manifault.com" <void@manifault.com>,
        "atomlin@atomlin.com" <atomlin@atomlin.com>,
        "allen.lkml@gmail.com" <allen.lkml@gmail.com>,
        "joe@perches.com" <joe@perches.com>,
        "msuchanek@suse.de" <msuchanek@suse.de>,
        "oleksandr@natalenko.name" <oleksandr@natalenko.name>
Subject: Re: [PATCH v8 04/13] module: Move livepatch support to a separate
 file
Message-ID: <YhzIjPYNw2tA4GmS@alley>
References: <20220222141303.1392190-1-atomlin@redhat.com>
 <20220222141303.1392190-5-atomlin@redhat.com>
 <Yhiik2ledqAfGuN2@alley>
 <fb1bb248-bd3f-0990-cdfd-d186b7579411@csgroup.eu>
 <YhyqaGO+vbGOifpR@alley>
 <ed8bb968-0a88-39cf-f388-032e8c205df7@csgroup.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ed8bb968-0a88-39cf-f388-032e8c205df7@csgroup.eu>
Precedence: bulk

On Mon 2022-02-28 11:46:33, Christophe Leroy wrote:
> 
> 
> Le 28/02/2022 ? 11:56, Petr Mladek a ?crit?:
> > On Fri 2022-02-25 16:49:31, Christophe Leroy wrote:
> >> Le 25/02/2022 ? 10:34, Petr Mladek a ?crit?:
> >>>
> >>> 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) {

Only with --strict option. Alias of this option is --subjective...

> 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 see.

From my POV, checkpatch is an useful tool for finding obvious mistakes.
But it is just a best effort approach. It has false positives. And
some complains are controversial.

BTW: I have never heard about --strict/--subjective option. I wonder
if some maintainer requires it.

> > 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.

Exactly. It is a real pain to find changes in moved functions. It is
much easier when the author just shuffled the code. Anyway, the less
changes the better.

Best Regards,
Petr