linux-kernel - Re: [PATCH] platform/x86: intel-vbtn: Fix code style issues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dj5vu3sfnct2jh53ztglh3tpoa4szs7oi77er3sjnihajwba73@2mb4bxzifqrt>
Date: Thu, 26 Jun 2025 18:09:48 +1000
From: Xiang Shen <turyshen@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hansg@...nel.org>, acelan.kao@...onical.com, 
	platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: intel-vbtn: Fix code style issues

On Wed, Jun 25, 2025 at 03:25:52PM +1000, Ilpo Järvinen wrote:
>On Wed, 25 Jun 2025, Xiang Shen wrote:
>
>> On Tue, Jun 24, 2025 at 01:35:57PM +1000, Ilpo Järvinen wrote:
>> > On Sun, 22 Jun 2025, Xiang Shen wrote:
>> > > On Fri, Jun 20, 2025 at 12:00:03PM +1000, Hans de Goede wrote:
>> > > > On 20-Jun-25 2:38 AM, Xiang Shen wrote:
>> > > > > Fix checkpatch code style errors:
>> > > > >
>> > > > > ERROR: do not use assignment in if condition
>> > > > > +	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev,
>> > > event))) {
>> > > > >
>> > > > > ERROR: do not use assignment in if condition
>> > > > > +	} else if ((ke =
>> > > sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
>> > > > >
>> > > > > Signed-off-by: Xiang Shen <turyshen@...il.com>
>> > > >
>> > > > Thank you for your patch, but this change really does not make
>> > > > the code more readable.
>> > > >
>> > > > The contrary the suggested changes are making the code harder
>> > > > to read, so NACK.
>> > > >
>> > > > Note checkpatch is just a tool, sometimes there are good reasons
>> > > > to deviate from the style checks done by checkpatch.
>> > > >
>> > > > Next time when submitting a patch to fix checkpatch issues please
>> > > > take a look at the resulting code after the patch and only submit
>> > > > the patch upstream if it actually is an improvement.
>> > > >
>> > > > Regards,
>> > > >
>> > > > Hans
>> > > >
>> > > Hi Hans,
>> > >
>> > > Thanks for the feedback.
>> > >
>> > > That's fine if breaking the "rule" is the only way to keep the file
>> > > readable.
>> > >
>> > > However, there are only three files (x86/sony-laptop.c and
>> > > x86/dell/dell_rbu.c) out of 273 files in the whole drivers/platform
>> > > folder that have such an error.
>> >
>> > Hi,
>> >
>> > Please don't call correct code "error" even if checkpatch may label it as
>> > such. The goal is NOT and will never be to have zero checkpatch warnings.
>> >
>> > The fact that the checkpatch "rule" is broken only a few times does not
>> > mean those 3 places have a problem, it just tells it's good rule for the
>> > general case. So I won't accept using such numbers as a leverage against
>> > the few places just for the sake of silencing checkpatch.
>> >
>>
>> I just thought there must be a reason that the checkpatch categories findings
>> as "ERROR", "WARNING" and "CHECK".
>
>The checkpatch change submitter just picked one of the levels. They're
>humans too. :-)
>
>> Sometimes the number does make sense and means the vast majority
>> follow the widely accepted "rule".
>>
>> Curiously, isn't it the contributor's due diligence to pass checkpatch
>> in the first place before sending?
>
>Hey, that code you're changing is not being submitted. We don't want to
>waste our time on "fixing" checkpatch warnings on existing code when the
>"fix" results in worse readability that before.
>
>Also, if you would be submitting a patch and checkpatch suggest you
>to make the patch worse, please don't follow checkpatch's advice even in
>that case!
>
>Checker tools are there to help find potential issues, not rulebooks. This
>is often overlooked by many checker tool focused developers and results in
>low quality patches (there are exceptions too where they developer really
>tries to understand all the related code to see if the change makes sense
>/ improves things or not).
>

Hi Ilpo,

I've learned a lot — thanks for the thorough explanation rather than
just brushing me off.

The rookies are certainly more likely than the veterans to treat
the "rulebooks" as ironclad disciplines.

>> Should any objection, submit a patch to
>> checkpatch itself,
>> instead of sneaking into the upstream quietly for the sake of "readability".
>>
>> > > Perhaps there are other approaches to make them more readable without
>> > > breaking the rule.
>> >
>> > Perhaps, but I'm not sure the effort spent to find one is worthwhile
>> > investment.
>> >
>>
>> Indeed, that's precisely why it might be worth sacrificing a bit of
>> "readability".
>
>Please stop trying to sneak your subpar change in.
>
>NO, this a hard NO from the acting maintainer. We'll not be accepting
>misguided changes that are to silence ANY checker that should have know
>better (those checkers will never learn enough to be relied 100% so
>please stop spreading such illusion).
>

I wish I had known those checking errors were intentionally ignored.
I'm putting a full stop here.