| Message ID | dda2187e128bfaaf092351812e4538e2e41c17f6.1711599093.git.fthain@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial/pmac_zilog: Remove flawed mitigation for rx irq flood | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
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:)
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.
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?
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).
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.
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?
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
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.
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?).
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/
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
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,
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,
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
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 --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; }
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(-)