diff mbox series

serial/pmac_zilog: Remove flawed mitigation for rx irq flood

Message ID dda2187e128bfaaf092351812e4538e2e41c17f6.1711599093.git.fthain@linux-m68k.org
State Superseded
Headers show
Series serial/pmac_zilog: Remove flawed mitigation for rx irq flood | expand

Commit Message

Finn Thain March 28, 2024, 4:11 a.m. UTC
The mitigation was intended to stop the irq completely. That might have
been better than a hard lock-up but it turns out that you get a crash
anyway if you're using pmac_zilog as a serial console.

That's because the pr_err() call in pmz_receive_chars() results in
pmz_console_write() attempting to lock a spinlock already locked in
pmz_interrupt(). With CONFIG_DEBUG_SPINLOCK=y, this produces a fatal
BUG splat like the one below. (The spinlock at 0x62e140 is the one in
struct uart_port.)

Even when it's not fatal, the serial port rx function ceases to work.
Also, the iteration limit doesn't play nicely with QEMU. Please see
bug report linked below.

A web search for reports of the error message "pmz: rx irq flood" didn't
produce anything. So I don't think this code is needed any more. Remove it.

[   14.560000] ttyPZ0: pmz: rx irq flood !
[   14.560000] BUG: spinlock recursion on CPU#0, swapper/0
[   14.560000]  lock: 0x62e140, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
[   14.560000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.8.0-mac-dbg-preempt-00004-g4143b7b9144a #1
[   14.560000] Stack from 0059bcc4:
[   14.560000]         0059bcc4 0056316f 0056316f 00002700 004b6444 0059bce4 004ad8c6 0056316f
[   14.560000]         0059bd10 004a6546 00556759 0062e140 dead4ead 0059f892 00000000 00000000
[   14.560000]         0062e140 0059bde8 005c03d0 0059bd24 0004daf6 0062e140 005567bf 0062e140
[   14.560000]         0059bd34 004b64c2 0062e140 00000001 0059bd50 002e15ea 0062e140 00000001
[   14.560000]         0059bde7 0059bde8 005c03d0 0059bdac 0005124e 005c03d0 005cdc00 0000002b
[   14.560000]         005a3caa 005a3caa 00000000 0059bde8 0004ff00 0059be8b 00038200 000529ba
[   14.560000] Call Trace: [<00002700>] ret_from_kernel_thread+0xc/0x14
[   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
[   14.560000]  [<004ad8c6>] dump_stack+0x10/0x16
[   14.560000]  [<004a6546>] spin_dump+0x6e/0x7c
[   14.560000]  [<0004daf6>] do_raw_spin_lock+0x9c/0xa6
[   14.560000]  [<004b64c2>] _raw_spin_lock_irqsave+0x2a/0x34
[   14.560000]  [<002e15ea>] pmz_console_write+0x32/0x9a
[   14.560000]  [<0005124e>] console_flush_all+0x112/0x3a2
[   14.560000]  [<0004ff00>] console_trylock+0x0/0x7a
[   14.560000]  [<00038200>] parameq+0x48/0x6e
[   14.560000]  [<000529ba>] __printk_safe_enter+0x0/0x36
[   14.560000]  [<0005113c>] console_flush_all+0x0/0x3a2
[   14.560000]  [<000542c4>] prb_read_valid+0x0/0x1a
[   14.560000]  [<004b65a4>] _raw_spin_unlock+0x0/0x38
[   14.560000]  [<0005151e>] console_unlock+0x40/0xb8
[   14.560000]  [<00038200>] parameq+0x48/0x6e
[   14.560000]  [<002c778c>] __tty_insert_flip_string_flags+0x0/0x14e
[   14.560000]  [<00051798>] vprintk_emit+0x156/0x238
[   14.560000]  [<00051894>] vprintk_default+0x1a/0x1e
[   14.560000]  [<000529a8>] vprintk+0x74/0x86
[   14.560000]  [<004a6596>] _printk+0x12/0x16
[   14.560000]  [<002e23be>] pmz_receive_chars+0x1cc/0x394
[   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
[   14.560000]  [<00038226>] parse_args+0x0/0x3a6
[   14.560000]  [<004b6466>] _raw_spin_lock+0x22/0x28
[   14.560000]  [<002e26b4>] pmz_interrupt+0x12e/0x1e0
[   14.560000]  [<00048680>] arch_cpu_idle_enter+0x0/0x8
[   14.560000]  [<00054ebc>] __handle_irq_event_percpu+0x24/0x106
[   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
[   14.560000]  [<00055020>] handle_irq_event+0x30/0x90
[   14.560000]  [<00058320>] handle_simple_irq+0x5e/0xc0
[   14.560000]  [<00048688>] arch_cpu_idle_exit+0x0/0x8
[   14.560000]  [<00054800>] generic_handle_irq+0x3c/0x4a
[   14.560000]  [<00002978>] do_IRQ+0x24/0x3a
[   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
[   14.560000]  [<00002874>] auto_irqhandler_fixup+0x4/0xc
[   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
[   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
[   14.560000]  [<004ae598>] default_idle_call+0x22/0x46
[   14.560000]  [<00048710>] do_idle+0x6a/0xf0
[   14.560000]  [<000486a6>] do_idle+0x0/0xf0
[   14.560000]  [<000367d2>] find_task_by_pid_ns+0x0/0x2a
[   14.560000]  [<0005d064>] __rcu_read_lock+0x0/0x12
[   14.560000]  [<00048a5a>] cpu_startup_entry+0x18/0x1c
[   14.560000]  [<00063a06>] __rcu_read_unlock+0x0/0x26
[   14.560000]  [<004ae65a>] kernel_init+0x0/0xfa
[   14.560000]  [<0049c5a8>] strcpy+0x0/0x1e
[   14.560000]  [<004a6584>] _printk+0x0/0x16
[   14.560000]  [<0049c72a>] strlen+0x0/0x22
[   14.560000]  [<006452d4>] memblock_alloc_try_nid+0x0/0x82
[   14.560000]  [<0063939a>] arch_post_acpi_subsys_init+0x0/0x8
[   14.560000]  [<0063991e>] console_on_rootfs+0x0/0x60
[   14.560000]  [<00638410>] _sinittext+0x410/0xadc
[   14.560000]

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
Cc: linux-m68k@lists.linux-m68k.org
Link: https://github.com/vivier/qemu-m68k/issues/44
Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
 drivers/tty/serial/pmac_zilog.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Andy Shevchenko April 3, 2024, 10:29 p.m. UTC | #1
Thu, Mar 28, 2024 at 03:11:33PM +1100, Finn Thain kirjoitti:
> The mitigation was intended to stop the irq completely. That might have
> been better than a hard lock-up but it turns out that you get a crash
> anyway if you're using pmac_zilog as a serial console.
> 
> That's because the pr_err() call in pmz_receive_chars() results in
> pmz_console_write() attempting to lock a spinlock already locked in
> pmz_interrupt(). With CONFIG_DEBUG_SPINLOCK=y, this produces a fatal
> BUG splat like the one below. (The spinlock at 0x62e140 is the one in
> struct uart_port.)
> 
> Even when it's not fatal, the serial port rx function ceases to work.
> Also, the iteration limit doesn't play nicely with QEMU. Please see
> bug report linked below.
> 
> A web search for reports of the error message "pmz: rx irq flood" didn't
> produce anything. So I don't think this code is needed any more. Remove it.

> [   14.560000] ttyPZ0: pmz: rx irq flood !
> [   14.560000] BUG: spinlock recursion on CPU#0, swapper/0
> [   14.560000]  lock: 0x62e140, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
> [   14.560000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.8.0-mac-dbg-preempt-00004-g4143b7b9144a #1
> [   14.560000] Stack from 0059bcc4:
> [   14.560000]         0059bcc4 0056316f 0056316f 00002700 004b6444 0059bce4 004ad8c6 0056316f
> [   14.560000]         0059bd10 004a6546 00556759 0062e140 dead4ead 0059f892 00000000 00000000
> [   14.560000]         0062e140 0059bde8 005c03d0 0059bd24 0004daf6 0062e140 005567bf 0062e140
> [   14.560000]         0059bd34 004b64c2 0062e140 00000001 0059bd50 002e15ea 0062e140 00000001
> [   14.560000]         0059bde7 0059bde8 005c03d0 0059bdac 0005124e 005c03d0 005cdc00 0000002b
> [   14.560000]         005a3caa 005a3caa 00000000 0059bde8 0004ff00 0059be8b 00038200 000529ba
> [   14.560000] Call Trace: [<00002700>] ret_from_kernel_thread+0xc/0x14
> [   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
> [   14.560000]  [<004ad8c6>] dump_stack+0x10/0x16
> [   14.560000]  [<004a6546>] spin_dump+0x6e/0x7c
> [   14.560000]  [<0004daf6>] do_raw_spin_lock+0x9c/0xa6
> [   14.560000]  [<004b64c2>] _raw_spin_lock_irqsave+0x2a/0x34
> [   14.560000]  [<002e15ea>] pmz_console_write+0x32/0x9a
> [   14.560000]  [<0005124e>] console_flush_all+0x112/0x3a2
> [   14.560000]  [<0004ff00>] console_trylock+0x0/0x7a
> [   14.560000]  [<00038200>] parameq+0x48/0x6e
> [   14.560000]  [<000529ba>] __printk_safe_enter+0x0/0x36
> [   14.560000]  [<0005113c>] console_flush_all+0x0/0x3a2
> [   14.560000]  [<000542c4>] prb_read_valid+0x0/0x1a
> [   14.560000]  [<004b65a4>] _raw_spin_unlock+0x0/0x38
> [   14.560000]  [<0005151e>] console_unlock+0x40/0xb8
> [   14.560000]  [<00038200>] parameq+0x48/0x6e
> [   14.560000]  [<002c778c>] __tty_insert_flip_string_flags+0x0/0x14e
> [   14.560000]  [<00051798>] vprintk_emit+0x156/0x238
> [   14.560000]  [<00051894>] vprintk_default+0x1a/0x1e
> [   14.560000]  [<000529a8>] vprintk+0x74/0x86
> [   14.560000]  [<004a6596>] _printk+0x12/0x16
> [   14.560000]  [<002e23be>] pmz_receive_chars+0x1cc/0x394
> [   14.560000]  [<004b6444>] _raw_spin_lock+0x0/0x28
> [   14.560000]  [<00038226>] parse_args+0x0/0x3a6
> [   14.560000]  [<004b6466>] _raw_spin_lock+0x22/0x28
> [   14.560000]  [<002e26b4>] pmz_interrupt+0x12e/0x1e0
> [   14.560000]  [<00048680>] arch_cpu_idle_enter+0x0/0x8
> [   14.560000]  [<00054ebc>] __handle_irq_event_percpu+0x24/0x106
> [   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
> [   14.560000]  [<00055020>] handle_irq_event+0x30/0x90
> [   14.560000]  [<00058320>] handle_simple_irq+0x5e/0xc0
> [   14.560000]  [<00048688>] arch_cpu_idle_exit+0x0/0x8
> [   14.560000]  [<00054800>] generic_handle_irq+0x3c/0x4a
> [   14.560000]  [<00002978>] do_IRQ+0x24/0x3a
> [   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
> [   14.560000]  [<00002874>] auto_irqhandler_fixup+0x4/0xc
> [   14.560000]  [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e
> [   14.560000]  [<004ae576>] default_idle_call+0x0/0x46
> [   14.560000]  [<004ae598>] default_idle_call+0x22/0x46
> [   14.560000]  [<00048710>] do_idle+0x6a/0xf0
> [   14.560000]  [<000486a6>] do_idle+0x0/0xf0
> [   14.560000]  [<000367d2>] find_task_by_pid_ns+0x0/0x2a
> [   14.560000]  [<0005d064>] __rcu_read_lock+0x0/0x12
> [   14.560000]  [<00048a5a>] cpu_startup_entry+0x18/0x1c
> [   14.560000]  [<00063a06>] __rcu_read_unlock+0x0/0x26
> [   14.560000]  [<004ae65a>] kernel_init+0x0/0xfa
> [   14.560000]  [<0049c5a8>] strcpy+0x0/0x1e
> [   14.560000]  [<004a6584>] _printk+0x0/0x16
> [   14.560000]  [<0049c72a>] strlen+0x0/0x22
> [   14.560000]  [<006452d4>] memblock_alloc_try_nid+0x0/0x82
> [   14.560000]  [<0063939a>] arch_post_acpi_subsys_init+0x0/0x8
> [   14.560000]  [<0063991e>] console_on_rootfs+0x0/0x60
> [   14.560000]  [<00638410>] _sinittext+0x410/0xadc
> [   14.560000]

First of all, please read this
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
and amend the commit message accordingly.

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> Cc: linux-m68k@lists.linux-m68k.org

Second, please move these Cc to be after the '---' line

> Link: https://github.com/vivier/qemu-m68k/issues/44
> Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/

Missed Fixes tag?

> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
(here is a good location for Cc:)
Finn Thain April 3, 2024, 11:59 p.m. UTC | #2
On Thu, 4 Apr 2024, Andy Shevchenko wrote:

> ...
> 
> First of all, please read this
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> and amend the commit message accordingly.
> 

Right -- the call chain is described in the commit log message so the 
backtrace does not add value. And the timestamps, stack dump etc. are 
irrelevant.

> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> > Cc: linux-m68k@lists.linux-m68k.org
> 
> Second, please move these Cc to be after the '---' line
> 

I thought they were placed above the line for audit (and signing) 
purposes. There are thousands of Cc lines in the mainline commit messages 
since v6.8.

> > Link: https://github.com/vivier/qemu-m68k/issues/44
> > Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
> 
> Missed Fixes tag?
> 

Would this be ok: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
I have to ask because some reviewers do not like to see a Fixes tag cite 
that commit.

> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> (here is a good location for Cc:)
> 

Documentation/process/submitting-patches.rst indicats that it should be 
above the "---" separator together with Acked-by etc. Has this convention 
changed recently?

Thanks for your review.
Jiri Slaby April 4, 2024, 5:07 a.m. UTC | #3
On 04. 04. 24, 0:29, Andy Shevchenko wrote:
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
>> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
>> Cc: linux-m68k@lists.linux-m68k.org
> 
> Second, please move these Cc to be after the '---' line

Sorry, but why?
Andy Shevchenko April 4, 2024, 9:20 a.m. UTC | #4
On Thu, Apr 4, 2024 at 8:07 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> On 04. 04. 24, 0:29, Andy Shevchenko wrote:

> >> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> Cc: Michael Ellerman <mpe@ellerman.id.au>
> >> Cc: Nicholas Piggin <npiggin@gmail.com>
> >> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> >> Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> >> Cc: linux-m68k@lists.linux-m68k.org
> >
> > Second, please move these Cc to be after the '---' line
>
> Sorry, but why?

Really need to create a Q&A entry for this.

This will pollute the commit messages with irrelevant (to some extent)
information. Since we have a lore mail archive there is no need to
have this (the email itself will be sent to the list of people,
otherwise the Cc email headers can be tracked in the mail archive).
Also note, some developers may read git history on the mobile devices,
meaning small screens, this just (as for backtraces) simply blurs the
information with a high potential to lose significant piece(s) of
information). Last, but not least is environmentally friendly approach
(I'm not joking): having it on thousands of computers, scrolling with
longer time, power for compressing - decompressing -- all of this
wastes a lot of energy (maybe kWh:s per such a Cc list).
Andy Shevchenko April 4, 2024, 9:24 a.m. UTC | #5
On Thu, Apr 4, 2024 at 2:57 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Thu, 4 Apr 2024, Andy Shevchenko wrote:

...

> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > > Cc: Nicholas Piggin <npiggin@gmail.com>
> > > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> > > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> > > Cc: linux-m68k@lists.linux-m68k.org
> >
> > Second, please move these Cc to be after the '---' line
>
> I thought they were placed above the line for audit (and signing)
> purposes.

I didn't get this, sorry.

> There are thousands of Cc lines in the mainline commit messages
> since v6.8.

Having thousands of mistaken cases does not prove it's a good thing to
follow. I answered Jiri why it's better the way I suggested.

> > > Link: https://github.com/vivier/qemu-m68k/issues/44
> > > Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
> >
> > Missed Fixes tag?
>
> Would this be ok: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> I have to ask because some reviewers do not like to see a Fixes tag cite
> that commit.

Yes, or you even may dig into the history.git from history group (see
git.kernel.org) for the real first patch that brought it.

> > > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > > ---
> > (here is a good location for Cc:)
>
> Documentation/process/submitting-patches.rst indicats that it should be
> above the "---" separator together with Acked-by etc. Has this convention
> changed recently?

I see, I will prepare a patch to discuss this aspect.
Finn Thain April 4, 2024, 10:17 p.m. UTC | #6
On Thu, 4 Apr 2024, Andy Shevchenko wrote:

> 
> > > > ---
> > > (here is a good location for Cc:)
> >
> > Documentation/process/submitting-patches.rst indicats that it should 
> > be above the "---" separator together with Acked-by etc. Has this 
> > convention changed recently?
> 
> I see, I will prepare a patch to discuss this aspect.
> 

If you are going to veto patches on the basis of rules yet unwritten, I 
think you risk turning the kernel development process into a lottery.

How many other patches presently under review will need to be dropped just 
in case they don't conform with possible future rules?
Michael Ellerman April 5, 2024, 3:06 a.m. UTC | #7
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Thu, Apr 4, 2024 at 2:57 AM Finn Thain <fthain@linux-m68k.org> wrote:
>> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
>
>> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
>> > > Cc: Nicholas Piggin <npiggin@gmail.com>
>> > > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> > > Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
>> > > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
>> > > Cc: linux-m68k@lists.linux-m68k.org
>> >
>> > Second, please move these Cc to be after the '---' line
>>
>> I thought they were placed above the line for audit (and signing)
>> purposes.
>
> I didn't get this, sorry.
>
>> There are thousands of Cc lines in the mainline commit messages
>> since v6.8.
>
> Having thousands of mistaken cases does not prove it's a good thing to
> follow. I answered Jiri why it's better the way I suggested.
>
>> > > Link: https://github.com/vivier/qemu-m68k/issues/44
>> > > Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
>> >
>> > Missed Fixes tag?
>>
>> Would this be ok: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> I have to ask because some reviewers do not like to see a Fixes tag cite
>> that commit.
>
> Yes, or you even may dig into the history.git from history group (see
> git.kernel.org) for the real first patch that brought it.
>
>> > > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
>> > > ---
>> > (here is a good location for Cc:)
>>
>> Documentation/process/submitting-patches.rst indicats that it should be
>> above the "---" separator together with Acked-by etc. Has this convention
>> changed recently?

The docs don't really say where to put the Cc: tags, although they are
mentioned along with other tags which clearly are intended to go above
the separator.

> I see, I will prepare a patch to discuss this aspect.

FYI there was a discussion about this several years ago, where at least
some maintainers agreed that Cc: tags don't add much value and are
better placed below the --- separator.

Thread starts here: https://lore.kernel.org/all/87y31eov1l.fsf@concordia.ellerman.id.au/

cheers
Finn Thain April 5, 2024, 3:46 a.m. UTC | #8
On Fri, 5 Apr 2024, Michael Ellerman wrote:

> >> > > ---
> >> > (here is a good location for Cc:)
> >>
> >> Documentation/process/submitting-patches.rst indicats that it should 
> >> be above the "---" separator together with Acked-by etc. Has this 
> >> convention changed recently?
> 
> The docs don't really say where to put the Cc: tags, although they are 
> mentioned along with other tags which clearly are intended to go above 
> the separator.
> 

I see no ambiguity there. What's the point of copying the message headers 
into the message body unless it was intended that they form part of the 
commit log?

> > I see, I will prepare a patch to discuss this aspect.
> 
> FYI there was a discussion about this several years ago, where at least 
> some maintainers agreed that Cc: tags don't add much value and are 
> better placed below the --- separator.
> 

Maintainers who merge patches almost always add tags. And they can use the 
Cc tags from the message headers if they wish to. Or they can omit them or 
remove them. I don't mind. And I'd be happy to resubmit the patch with 
different tags if that's what is needed by the workflow used by Jiri Slaby 
or Greg Kroah-Hartman.
Andy Shevchenko April 5, 2024, 5:05 a.m. UTC | #9
On Fri, Apr 5, 2024 at 1:15 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
>
> > > > > ---
> > > > (here is a good location for Cc:)
> > >
> > > Documentation/process/submitting-patches.rst indicats that it should
> > > be above the "---" separator together with Acked-by etc. Has this
> > > convention changed recently?
> >
> > I see, I will prepare a patch to discuss this aspect.
>
> If you are going to veto patches on the basis of rules yet unwritten, I
> think you risk turning the kernel development process into a lottery.

It's already a lottery, if you haven't noticed, i.e. it highly relies
on the style preferences of the maintainers and is yet undocumented (a
few years ago it was a new section introduced for closing this gap).

> How many other patches presently under review will need to be dropped just
> in case they don't conform with possible future rules?

What you are saying is pure speculation.

I rely on at least two things (besides already explained):
- the fact that Submitting Patches refers to the commit message
reduction due to the unnecessariness of some lines
- my experience and common sense (why duplicate the data?).
Andy Shevchenko April 5, 2024, 5:09 a.m. UTC | #10
On Fri, Apr 5, 2024 at 6:06 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Thu, Apr 4, 2024 at 2:57 AM Finn Thain <fthain@linux-m68k.org> wrote:
> >> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
> >
> >> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> > > Cc: Michael Ellerman <mpe@ellerman.id.au>
> >> > > Cc: Nicholas Piggin <npiggin@gmail.com>
> >> > > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> > > Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
> >> > > Cc: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
> >> > > Cc: linux-m68k@lists.linux-m68k.org
> >> >
> >> > Second, please move these Cc to be after the '---' line
> >>
> >> I thought they were placed above the line for audit (and signing)
> >> purposes.
> >
> > I didn't get this, sorry.
> >
> >> There are thousands of Cc lines in the mainline commit messages
> >> since v6.8.
> >
> > Having thousands of mistaken cases does not prove it's a good thing to
> > follow. I answered Jiri why it's better the way I suggested.
> >
> >> > > Link: https://github.com/vivier/qemu-m68k/issues/44
> >> > > Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
> >> >
> >> > Missed Fixes tag?
> >>
> >> Would this be ok: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> I have to ask because some reviewers do not like to see a Fixes tag cite
> >> that commit.
> >
> > Yes, or you even may dig into the history.git from history group (see
> > git.kernel.org) for the real first patch that brought it.
> >
> >> > > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> >> > > ---
> >> > (here is a good location for Cc:)
> >>
> >> Documentation/process/submitting-patches.rst indicats that it should be
> >> above the "---" separator together with Acked-by etc. Has this convention
> >> changed recently?
>
> The docs don't really say where to put the Cc: tags, although they are
> mentioned along with other tags which clearly are intended to go above
> the separator.

He-h... Documentation needs constant updates too, for one reason or another.
This is normal process and in particular Cc (rather long) lists needs to be
reconsidered.

> > I see, I will prepare a patch to discuss this aspect.
>
> FYI there was a discussion about this several years ago, where at least
> some maintainers agreed that Cc: tags don't add much value and are
> better placed below the --- separator.

Thanks, I'll definitely read this.
But I'm 100% sure the environment aspect and mobile device screen
sizes were not discussed there.

> Thread starts here: https://lore.kernel.org/all/87y31eov1l.fsf@concordia.ellerman.id.au/
Michael Ellerman April 8, 2024, 5:29 a.m. UTC | #11
Finn Thain <fthain@linux-m68k.org> writes:
> On Fri, 5 Apr 2024, Michael Ellerman wrote:
>> >> > > ---
>> >> > (here is a good location for Cc:)
>> >>
>> >> Documentation/process/submitting-patches.rst indicats that it should 
>> >> be above the "---" separator together with Acked-by etc. Has this 
>> >> convention changed recently?
>> 
>> The docs don't really say where to put the Cc: tags, although they are 
>> mentioned along with other tags which clearly are intended to go above 
>> the separator.
>
> I see no ambiguity there. What's the point of copying the message headers 
> into the message body unless it was intended that they form part of the 
> commit log?

In many cases I think it's the reverse. The Cc headers are in the commit
log *so that* git-send-email will add them to the Cc list when the patch
is sent.

In that case they can sit below the separator and still function.

IMO there is no value in having them in the commit log. The fact that
someone was Cc'ed on a patch 10 years ago is not interesting. If it ever
was interesting, for some forensic purpose, the mail archives would be
the place to look anyway.

>> > I see, I will prepare a patch to discuss this aspect.
>> 
>> FYI there was a discussion about this several years ago, where at least 
>> some maintainers agreed that Cc: tags don't add much value and are 
>> better placed below the --- separator.
>> 
>
> Maintainers who merge patches almost always add tags. And they can use the 
> Cc tags from the message headers if they wish to. Or they can omit them or 
> remove them. I don't mind. And I'd be happy to resubmit the patch with 
> different tags if that's what is needed by the workflow used by Jiri Slaby 
> or Greg Kroah-Hartman.

Many maintainers won't drop Cc: tags if they are there in the submitted
patch. So I agree with Andy that we should encourage folks not to add
them in the first place.

But that's getting very off topic for your submission :)

cheers
Jiri Slaby April 8, 2024, 5:32 a.m. UTC | #12
On 08. 04. 24, 7:29, Michael Ellerman wrote:
> Many maintainers won't drop Cc: tags if they are there in the submitted
> patch. So I agree with Andy that we should encourage folks not to add
> them in the first place.

But fix the docs first.

I am personally not biased to any variant (as in: I don't care where CCs 
live in a patch).

regards,
Jiri Slaby April 8, 2024, 5:37 a.m. UTC | #13
On 08. 04. 24, 7:32, Jiri Slaby wrote:
> On 08. 04. 24, 7:29, Michael Ellerman wrote:
>> Many maintainers won't drop Cc: tags if they are there in the submitted
>> patch. So I agree with Andy that we should encourage folks not to add
>> them in the first place.
> 
> But fix the docs first.
> 
> I am personally not biased to any variant (as in: I don't care where CCs 
> live in a patch).

OTOH, as a submitter, it's a major PITA to carry CCs in notes (to have 
those under the --- line). Esp. when I have patches in a queue for years.

How do people handle that? (Like rebases on current kernel.)

> regards,
Greg KH April 8, 2024, 5:44 a.m. UTC | #14
On Mon, Apr 08, 2024 at 07:37:22AM +0200, Jiri Slaby wrote:
> On 08. 04. 24, 7:32, Jiri Slaby wrote:
> > On 08. 04. 24, 7:29, Michael Ellerman wrote:
> > > Many maintainers won't drop Cc: tags if they are there in the submitted
> > > patch. So I agree with Andy that we should encourage folks not to add
> > > them in the first place.
> > 
> > But fix the docs first.
> > 
> > I am personally not biased to any variant (as in: I don't care where CCs
> > live in a patch).
> 
> OTOH, as a submitter, it's a major PITA to carry CCs in notes (to have those
> under the --- line). Esp. when I have patches in a queue for years.

Agreed, let's keep them where they are in the signed-off-by area, it's
not hurting or harming anything to have them there.

thanks,

greg k-h
Geert Uytterhoeven April 8, 2024, 8:32 a.m. UTC | #15
Hi Jiri,

On Mon, Apr 8, 2024 at 7:37 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> On 08. 04. 24, 7:32, Jiri Slaby wrote:
> > On 08. 04. 24, 7:29, Michael Ellerman wrote:
> >> Many maintainers won't drop Cc: tags if they are there in the submitted
> >> patch. So I agree with Andy that we should encourage folks not to add
> >> them in the first place.
> >
> > But fix the docs first.
> >
> > I am personally not biased to any variant (as in: I don't care where CCs
> > live in a patch).
>
> OTOH, as a submitter, it's a major PITA to carry CCs in notes (to have
> those under the --- line). Esp. when I have patches in a queue for years.

(Good to discover I'm not the only one carrying Very Old Patches ;-)

> How do people handle that? (Like rebases on current kernel.)

Keep them under the --- line in the actual commits, just like your
changelog? All of that is retained when rebasing.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index c8bf08c19c64..77691fbbf779 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -210,7 +210,6 @@  static bool pmz_receive_chars(struct uart_pmac_port *uap)
 {
 	struct tty_port *port;
 	unsigned char ch, r1, drop, flag;
-	int loops = 0;
 
 	/* Sanity check, make sure the old bug is no longer happening */
 	if (uap->port.state == NULL) {
@@ -291,24 +290,11 @@  static bool pmz_receive_chars(struct uart_pmac_port *uap)
 		if (r1 & Rx_OVR)
 			tty_insert_flip_char(port, 0, TTY_OVERRUN);
 	next_char:
-		/* We can get stuck in an infinite loop getting char 0 when the
-		 * line is in a wrong HW state, we break that here.
-		 * When that happens, I disable the receive side of the driver.
-		 * Note that what I've been experiencing is a real irq loop where
-		 * I'm getting flooded regardless of the actual port speed.
-		 * Something strange is going on with the HW
-		 */
-		if ((++loops) > 1000)
-			goto flood;
 		ch = read_zsreg(uap, R0);
 		if (!(ch & Rx_CH_AV))
 			break;
 	}
 
-	return true;
- flood:
-	pmz_interrupt_control(uap, 0);
-	pmz_error("pmz: rx irq flood !\n");
 	return true;
 }