Messages in this thread | | | From | Miguel Ojeda <> | Date | Tue, 16 Feb 2021 13:42:19 +0100 | Subject | Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage. |
| |
On Tue, Feb 16, 2021 at 11:28 AM Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > Could we please avoid documenting the obvious? It is more or less common > knowledge that the write callback (like any other) is preemptible user > context (in which write occurs). The same is true for register/probe > functions. The non-preemptible / atomic is mostly the exception because > of the callback. Like from a timer or an interrupt.
It is not so much about documenting the obvious, but about stating that 1) the precondition was properly taken into account and that 2) nothing non-obvious is undocumented. When code is changed later on, it is much more likely assumptions are broken if not documented.
In fact, from a quick git blame, that seems to be what happened here: originally the function could be called from a public function intended to be used from inside the kernel; so I assume it was the intention to allow calls from softirq contexts. Then it was refactored and the check never removed. In this case, the extra check is not a big deal, but going in the opposite direction can happen too, and then we will have a bug.
In general, when a patch for a fix is needed, it's usually a good idea to add a comment right in the code. Even if only to avoid someone else having to backtrack the calls to see it is only called form fs_ops etc.
Cheers, Miguel
| |