[PATCH] auxdisplay: Remove in_interrupt() usage.

[PATCH] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

charlcd_write() is invoked as a VFS->write() callback and as such it is
always invoked from preemptible context and may sleep.

charlcd_puts() is invoked from register/unregister callback which is
preemtible. The reboot notifier callback is also invoked from
preemptible context.

Therefore there is no need to use `in_interrupt()' to figure out if it
is save to sleep because it always is.
Using `schedule()' to schedule (an be friendly to others) is
discouraged and `cond_resched()' should be used instead.

Remove `in_interrupt()' and use `cond_resched()' to schedule every 32
iteration if needed.

Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/auxdisplay/charlcd.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index f43430e9dceed..fbfce95919f72 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -470,12 +470,8 @@ static ssize_t charlcd_write(struct file *file, const char __user *buf,
 	char c;
 
 	for (; count-- > 0; (*ppos)++, tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		if (get_user(c, tmp))
 			return -EFAULT;
@@ -537,12 +533,8 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
 	int count = strlen(s);
 
 	for (; count-- > 0; tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		charlcd_write_char(lcd, *tmp);
 	}
-- 
2.30.0

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

Hi Sebastian,

On Mon, Feb 8, 2021 at 6:59 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> charlcd_write() is invoked as a VFS->write() callback and as such it is
> always invoked from preemptible context and may sleep.
>
> charlcd_puts() is invoked from register/unregister callback which is
> preemtible. The reboot notifier callback is also invoked from

preemtible -> preemptible

> preemptible context.
>
> Therefore there is no need to use `in_interrupt()' to figure out if it
> is save to sleep because it always is.

save -> safe

Does it hurt to have `in_interrupt()`? Future patches could make it so
that it is no longer a preemptible context. Should it be moved to e.g.
a `WARN_ON()` instead?

> Using `schedule()' to schedule (an be friendly to others) is

an -> and

> discouraged and `cond_resched()' should be used instead.
>
> Remove `in_interrupt()' and use `cond_resched()' to schedule every 32
> iteration if needed.
>
> Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Cc'ing Geert and Willy too.

Thanks for the patch!

Cheers,
Miguel

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-08 19:38:10 [+0100], Miguel Ojeda wrote:
> Hi Sebastian,
Hi,

> > Therefore there is no need to use `in_interrupt()' to figure out if it
> > is save to sleep because it always is.
> 
> save -> safe
> 
> Does it hurt to have `in_interrupt()`? Future patches could make it so
Yes.

> that it is no longer a preemptible context. Should it be moved to e.g.
> a `WARN_ON()` instead?

No. If you know the context, pass it along like this is done for
kmalloc() for instance. The long term plan is not make it available to
divers (i.e. core code only where the context can not be known).

> Thanks for the patch!

I'm going to resend it with your corrections.

> Cheers,
> Miguel

Sebastian

[PATCH v2] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

charlcd_write() is invoked as a VFS->write() callback and as such it is
always invoked from preemptible context and may sleep.

charlcd_puts() is invoked from register/unregister callback which is
preemptible. The reboot notifier callback is also invoked from
preemptible context.

Therefore there is no need to use `in_interrupt()' to figure out if it
is safe to sleep because it always is.
Using `schedule()' to schedule (and be friendly to others) is
discouraged and `cond_resched()' should be used instead.

Remove `in_interrupt()' and use `cond_resched()' to schedule every 32
iteration if needed.

Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

v1…v2: Spelling fixes.

 drivers/auxdisplay/charlcd.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index f43430e9dceed..fbfce95919f72 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -470,12 +470,8 @@ static ssize_t charlcd_write(struct file *file, const char __user *buf,
 	char c;
 
 	for (; count-- > 0; (*ppos)++, tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		if (get_user(c, tmp))
 			return -EFAULT;
@@ -537,12 +533,8 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
 	int count = strlen(s);
 
 	for (; count-- > 0; tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		charlcd_write_char(lcd, *tmp);
 	}
-- 
2.30.0

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

On Mon, Feb 8, 2021 at 8:07 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> Yes.

In what way?

> No. If you know the context, pass it along like this is done for
> kmalloc() for instance.

What do you mean?

> The long term plan is not make it available to
> divers (i.e. core code only where the context can not be known).

Sounds good.

Cheers,
Miguel

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-08 21:14:54 [+0100], Miguel Ojeda wrote:
> On Mon, Feb 8, 2021 at 8:07 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > Yes.
> 
> In what way?

It hurts to keep in_interrupt() because it is desired to have it removed
from drivers. The problem is that pattern is often copied and people
sometimes get it wrong. For instance, the code here invoked schedule()
based on in_interrupt(). It did not check whether or not the interrupts
are disabled which is also important. It may work now, it can break in
future if an unrelated change is made. An example is commit
   c2d0f1a65ab9f ("scsi: libsas: Introduce a _gfp() variant of event notifiers")
   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git/commit/?id=c2d0f1a65ab9f

in_interrupt() is often used in old code that was written before
might_sleep() and lockdep was introduced.

> > No. If you know the context, pass it along like this is done for
> > kmalloc() for instance.
> 
> What do you mean?

What I meant was GFP_KERNEL for context which can sleep vs GFP_ATOMIC for
context which must not sleep. The commit above also eliminates the
in_interrupt() usage within the driver (in multiple steps).

> Cheers,
> Miguel

Sebastian

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

On Mon, Feb 8, 2021 at 9:41 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> It hurts to keep in_interrupt() because it is desired to have it removed
> from drivers. The problem is that pattern is often copied and people
> sometimes get it wrong. For instance, the code here invoked schedule()
> based on in_interrupt(). It did not check whether or not the interrupts
> are disabled which is also important. It may work now, it can break in
> future if an unrelated change is made. An example is commit
>    c2d0f1a65ab9f ("scsi: libsas: Introduce a _gfp() variant of event notifiers")
>    https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git/commit/?id=c2d0f1a65ab9f
>
> in_interrupt() is often used in old code that was written before
> might_sleep() and lockdep was introduced.

Thanks -- can you please add a Link: tag to a lore URL or the docs or
similar where more information can be found regarding the
proposal/discussion for removing `in_interrurt()` etc.? It is useful
to track why these things are happening around the kernel.

Also, `hacking.rst` (and related documentation) should be updated
before this is done, so that we can link to it.

> What I meant was GFP_KERNEL for context which can sleep vs GFP_ATOMIC for
> context which must not sleep. The commit above also eliminates the
> in_interrupt() usage within the driver (in multiple steps).

I was thinking something along those lines, but `in_interrupt()` nor
`cond_resched()` take an explicit context either, so I am confused.
Does `cond_reched()` always do the right thing regardless of context?
The docs are not really clear:

  "cond_resched() and cond_resched_lock(): latency reduction via
explicit rescheduling in places that are safe."

It could be read as "it will only resched whenever safe" or "only to
be called if it is safe".

Cheers,
Miguel

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-08 23:26:57 [+0100], Miguel Ojeda wrote:
> Thanks -- can you please add a Link: tag to a lore URL or the docs or
> similar where more information can be found regarding the
> proposal/discussion for removing `in_interrurt()` etc.? It is useful
> to track why these things are happening around the kernel.

If I post series with more than just one patch I have a cover letter
including:

|in the discussion about preempt count consistency across kernel
|configurations:
|
| https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/
|
|it was concluded that the usage of in_interrupt() and related context
|checks should be removed from non-core code.
|
|In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
|driver code completely.

since this patch was small, simple and removing not required code I kept
it out. Is this enough information for you?

> Also, `hacking.rst` (and related documentation) should be updated
> before this is done, so that we can link to it.

The information is not wrong, it doesn't say you have to use it it your
driver. It also does not mention that you should not. I will look into
this.

> > What I meant was GFP_KERNEL for context which can sleep vs GFP_ATOMIC for
> > context which must not sleep. The commit above also eliminates the
> > in_interrupt() usage within the driver (in multiple steps).
> 
> I was thinking something along those lines, but `in_interrupt()` nor
> `cond_resched()` take an explicit context either, so I am confused.
> Does `cond_reched()` always do the right thing regardless of context?
> The docs are not really clear:
> 
>   "cond_resched() and cond_resched_lock(): latency reduction via
> explicit rescheduling in places that are safe."
> 
> It could be read as "it will only resched whenever safe" or "only to
> be called if it is safe".

You should keep track of the context you are in and not attempt to sleep
if it is not allowed. If you are doing something that may monopolize the
CPU then you should use cond_resched(). The difference compared to
schedule() is that you don't blindly invoke the scheduler and that it is
optimized away on a preemptible kernel. But as you noticed, you must not
not use it if the context does not allow it (like in interrupt handler,
disabled preemption and so on).

> Cheers,
> Miguel

Sebastian

Re: [PATCH] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

Hi Sebastian,

On Tue, Feb 9, 2021 at 10:01 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> If I post series with more than just one patch I have a cover letter
> including:

Yeah, it is a bit confusing when reading without the context (it is
hard to keep up with everything going on unless you work full-time on
it :-)

> since this patch was small, simple and removing not required code I kept
> it out. Is this enough information for you?

If you don't mind, please add a quick sentence like (I can do it on my
side too):

    `in_interrupt()` and related context checks are being removed
    from non-core code.

Plus the tag:

    Link: https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

> The information is not wrong, it doesn't say you have to use it it your
> driver. It also does not mention that you should not. I will look into
> this.

Thanks!

Cheers,
Miguel

[PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

charlcd_write() is invoked as a VFS->write() callback and as such it is
always invoked from preemptible context and may sleep.

charlcd_puts() is invoked from register/unregister callback which is
preemtible. The reboot notifier callback is also invoked from
preemptible context.

Therefore there is no need to use `in_interrupt()' to figure out if it
is save to sleep because it always is. `in_interrupt()` and related
context checks are being removed from non-core code.
Using `schedule()' to schedule (an be friendly to others) is
discouraged and `cond_resched()' should be used instead.

Remove `in_interrupt()' and use `cond_resched()' to schedule every 32
iteration if needed.

Link: https://lkml.kernel.org/r/20200914204209.256266093@linutronix.de
Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: Extend the commit message as suggested by Miguel Ojeda.
v1…v2: Spelling fixes.

On 2021-02-10 22:46:23 [+0100], Miguel Ojeda wrote:
> Hi Sebastian,
Hi Miguel,

> Yeah, it is a bit confusing when reading without the context (it is
> hard to keep up with everything going on unless you work full-time on
> it :-)

Sorry for leaving it out. I though it is not needed since it was not
needed.

> > since this patch was small, simple and removing not required code I kept
> > it out. Is this enough information for you?
> 
> If you don't mind, please add a quick sentence like (I can do it on my
> side too):
> 
>     `in_interrupt()` and related context checks are being removed
>     from non-core code.
> 
> Plus the tag:
> 
>     Link: https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

Added as suggested.

 drivers/auxdisplay/charlcd.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index f43430e9dceed..fbfce95919f72 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -470,12 +470,8 @@ static ssize_t charlcd_write(struct file *file, const char __user *buf,
 	char c;
 
 	for (; count-- > 0; (*ppos)++, tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		if (get_user(c, tmp))
 			return -EFAULT;
@@ -537,12 +533,8 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
 	int count = strlen(s);
 
 	for (; count-- > 0; tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		charlcd_write_char(lcd, *tmp);
 	}
-- 
2.30.0

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

Hi Sebastian,

On Sat, Feb 13, 2021 at 5:50 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> charlcd_write() is invoked as a VFS->write() callback and as such it is
> always invoked from preemptible context and may sleep.

Can we put this sentence as a comment in the code, right before the
call to cond_resched()?

> charlcd_puts() is invoked from register/unregister callback which is
> preemtible. The reboot notifier callback is also invoked from

Same for this one.

In addition, somehow the spelling fixes got lost from the previous version.

Same for the "code quotes": some have no quotes, others have `` or `'.
No big deal, I can fix it on my side if needed, but just letting you
know! :-)

Thanks!

Cheers,
Miguel

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-16 10:32:15 [+0100], Miguel Ojeda wrote:
> Hi Sebastian,
Hi,

> On Sat, Feb 13, 2021 at 5:50 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > charlcd_write() is invoked as a VFS->write() callback and as such it is
> > always invoked from preemptible context and may sleep.
> 
> Can we put this sentence as a comment in the code, right before the
> call to cond_resched()?
> 
> > charlcd_puts() is invoked from register/unregister callback which is
> > preemtible. The reboot notifier callback is also invoked from
> 
> Same for this one.

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.

> In addition, somehow the spelling fixes got lost from the previous version.
> 
> Same for the "code quotes": some have no quotes, others have `` or `'.
> No big deal, I can fix it on my side if needed, but just letting you
> know! :-)

I'm so sorry. I must have taken the wrong patch while doing the update.
My apologies. Once we sorted out the above, I will provide an update.

> Thanks!
> 
> Cheers,
> Miguel

Sebastian

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

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

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-16 13:42:19 [+0100], Miguel Ojeda wrote:
> 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.

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

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

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.

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

This is not a fix. It just removes not needed code. Also I don't think
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.

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

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

> Cheers,
> Miguel

Sebastian

[PATCH v4] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

charlcd_write() is invoked as a VFS->write() callback and as such it is
always invoked from preemptible context and may sleep.

charlcd_puts() is invoked from register/unregister callback which is
preemptible. The reboot notifier callback is also invoked from
preemptible context.

Therefore there is no need to use in_interrupt() to figure out if it
is safe to sleep because it always is. in_interrupt() and related
context checks are being removed from non-core code.
Using schedule() to schedule (and be friendly to others) is
discouraged and cond_resched() should be used instead.

Remove in_interrupt() and use cond_resched() to schedule every 32
iteration if needed.

Link: https://lkml.kernel.org/r/20200914204209.256266093@linutronix.de
Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v4: - Spelling fixes got lot.
       - Add a comment before cond_resched() as requested by Miguel Ojeda.
       - Drop the ` ' comments.
v2…v3: Extend the commit message as suggested by Miguel Ojeda.
v1…v2: Spelling fixes.

 drivers/auxdisplay/charlcd.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index f43430e9dceed..95accc941023a 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -470,12 +470,14 @@ static ssize_t charlcd_write(struct file *file, const char __user *buf,
 	char c;
 
 	for (; count-- > 0; (*ppos)++, tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
+		if (((count + 1) & 0x1f) == 0) {
 			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
+			 * charlcd_write() is invoked as a VFS->write() callback
+			 * and as such it is > always invoked from preemptible
+			 * context and may sleep.
 			 */
-			schedule();
+			cond_resched();
+		}
 
 		if (get_user(c, tmp))
 			return -EFAULT;
@@ -537,12 +539,8 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
 	int count = strlen(s);
 
 	for (; count-- > 0; tmp++) {
-		if (!in_interrupt() && (((count + 1) & 0x1f) == 0))
-			/*
-			 * let's be a little nice with other processes
-			 * that need some CPU
-			 */
-			schedule();
+		if (((count + 1) & 0x1f) == 0)
+			cond_resched();
 
 		charlcd_write_char(lcd, *tmp);
 	}
-- 
2.30.0

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

On Tue, Feb 16, 2021 at 7:26 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.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

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Sebastian Andrzej Siewior]

On 2021-02-16 21:21:07 [+0100], Miguel Ojeda wrote:
…
> 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.

I assumed you are going to apply it but I don't see it in -next as of
today. Is there anything I need to do?

> Cheers,
> Miguel

Sebastian

Re: [PATCH v3] auxdisplay: Remove in_interrupt() usage.

[Miguel Ojeda]

On Wed, Mar 10, 2021 at 6:51 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> I assumed you are going to apply it but I don't see it in -next as of
> today. Is there anything I need to do?

Ah, since you said you were posting a patch, I kept this in the
waiting list. I will apply the latest one then.

Cheers,
Miguel