linux-kernel - Re: [PATCH v3] auxdisplay: Remove in

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72np-G3whePohyYazx3KpP6A+DsRwq-bjd7E7qKb1JG62w@mail.gmail.com>
Date:   Tue, 16 Feb 2021 21:21:07 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Willy Tarreau <w@....eu>
Subject: Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

On Tue, Feb 16, 2021 at 7:26 PM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
>
> That should be part of the commit message. You can always rewind to the
> commit message that introduce something and check if the commit message
> made sense or ignored a detail which made it wrong (or so).

No, it shouldn't. Commit messages are used to justify changes in the
past, but they shouldn't be used as a replacement for documenting the
present. The reason `in_interrupt()` is removed should be in the
commit message; while the reason the precondition for `cond_resched()`
is fulfilled should be in the code (assuming one finds the comment
necessary, see below).

> So it was needed once, it is not needed anymore. That was my arguing in
> v1 about. No word about general removing in_interrupt() from drivers.

Not sure what you mean by this.

> This is not a fix. It just removes not needed code. Also I don't think

It isn't a *bug* fix, yes, but it is a fix because the removal should
have happened when the code was refactored. We can call it a cleanup,
if you will.

> that this is a good idea to add a comment to avoid someone to backtrack
> / double check something. If you rely on a comment instead of double
> checking that you do is indeed correct you will rely one day on a stale
> comment and commit to a bug.

Sure, comments and documentation may contain bugs like anything else.
That does not mean comments and documentation are useless and we
should remove all of them. In fact, the kernel lacks quite a lot of
documentation.

On the stale point: one-liner comments contiguous to the line they
talk about and function docs are not as prone to get outdated as
out-of-line docs like `Documentation/`.

> To give you another example: If I would have come along and replaced
> GFP_ATOMIC with GFP_KERNEL would you ask for a comment?

Yes, I would, because if someone thought `GFP_ATOMIC` was needed at
some point, then it is likely there is something non-obvious going on.

Of course, it depends on the case. For instance, in the case of our
patch, having a comment is not a big deal because it is fairly clear,
specially for `charlcd_write()`. And `cond_resched()` has a
`might_sleep()` annotation which helps catching future bugs. So if you
don't add a comment, I will still take the patch since it is good
patch.

> Anyway, I'm posting a patch with changes as ordered in a jiffy.

It is not an order :-) i.e. don't feel pressured that you need to sign
off on the comment change -- I can submit the comment on my own later
on.

Cheers,
Miguel