Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
Hi Jiri,
On Mon, Apr 8, 2024 at 7:37 AM 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.
(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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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, -- js suse labs
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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, -- js suse labs
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
Finn Thain 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
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
On Fri, Apr 5, 2024 at 6:06 AM Michael Ellerman wrote:
> Andy Shevchenko writes:
> > On Thu, Apr 4, 2024 at 2:57 AM Finn Thain wrote:
> >> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
> >
> >> > > Cc: Benjamin Herrenschmidt
> >> > > Cc: Michael Ellerman
> >> > > Cc: Nicholas Piggin
> >> > > Cc: Christophe Leroy
> >> > > Cc: "Aneesh Kumar K.V"
> >> > > Cc: "Naveen N. Rao"
> >> > > Cc: linux-m...@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
> >> > > ---
> >> > (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@concordia.ellerman.id.au/
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
On Fri, Apr 5, 2024 at 1:15 AM Finn Thain 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?). -- With Best Regards, Andy Shevchenko
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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.
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
Andy Shevchenko writes:
> On Thu, Apr 4, 2024 at 2:57 AM Finn Thain wrote:
>> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
>
>> > > Cc: Benjamin Herrenschmidt
>> > > Cc: Michael Ellerman
>> > > Cc: Nicholas Piggin
>> > > Cc: Christophe Leroy
>> > > Cc: "Aneesh Kumar K.V"
>> > > Cc: "Naveen N. Rao"
>> > > Cc: linux-m...@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
>> > > ---
>> > (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@concordia.ellerman.id.au/
cheers
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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?
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
On Thu, Apr 4, 2024 at 2:57 AM Finn Thain wrote:
> On Thu, 4 Apr 2024, Andy Shevchenko wrote:
...
> > > Cc: Benjamin Herrenschmidt
> > > Cc: Michael Ellerman
> > > Cc: Nicholas Piggin
> > > Cc: Christophe Leroy
> > > Cc: "Aneesh Kumar K.V"
> > > Cc: "Naveen N. Rao"
> > > Cc: linux-m...@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
> > > ---
> > (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.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
On Thu, Apr 4, 2024 at 8:07 AM Jiri Slaby wrote: > On 04. 04. 24, 0:29, Andy Shevchenko wrote: > >> Cc: Benjamin Herrenschmidt > >> Cc: Michael Ellerman > >> Cc: Nicholas Piggin > >> Cc: Christophe Leroy > >> Cc: "Aneesh Kumar K.V" > >> Cc: "Naveen N. Rao" > >> Cc: linux-m...@lists.linux-m68k.org > > > > Second, please move these Cc to be after the '---' line > > Sorry, but why? Really need to create a Q 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). -- With Best Regards, Andy Shevchenko
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
On 04. 04. 24, 0:29, Andy Shevchenko wrote: Cc: Benjamin Herrenschmidt Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Christophe Leroy Cc: "Aneesh Kumar K.V" Cc: "Naveen N. Rao" Cc: linux-m...@lists.linux-m68k.org Second, please move these Cc to be after the '---' line Sorry, but why? -- js suse labs
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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
> > Cc: Michael Ellerman
> > Cc: Nicholas Piggin
> > Cc: Christophe Leroy
> > Cc: "Aneesh Kumar K.V"
> > Cc: "Naveen N. Rao"
> > Cc: linux-m...@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
> > ---
> (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.
Re: [PATCH] serial/pmac_zilog: Remove flawed mitigation for rx irq flood
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.56] ttyPZ0: pmz: rx irq flood ! > [ 14.56] BUG: spinlock recursion on CPU#0, swapper/0 > [ 14.56] lock: 0x62e140, .magic: dead4ead, .owner: swapper/0, > .owner_cpu: 0 > [ 14.56] CPU: 0 PID: 0 Comm: swapper Not tainted > 6.8.0-mac-dbg-preempt-4-g4143b7b9144a #1 > [ 14.56] Stack from 0059bcc4: > [ 14.56] 0059bcc4 0056316f 0056316f 2700 004b6444 0059bce4 > 004ad8c6 0056316f > [ 14.56] 0059bd10 004a6546 00556759 0062e140 dead4ead 0059f892 > > [ 14.56] 0062e140 0059bde8 005c03d0 0059bd24 0004daf6 0062e140 > 005567bf 0062e140 > [ 14.56] 0059bd34 004b64c2 0062e140 0001 0059bd50 002e15ea > 0062e140 0001 > [ 14.56] 0059bde7 0059bde8 005c03d0 0059bdac 0005124e 005c03d0 > 005cdc00 002b > [ 14.56] 005a3caa 005a3caa 0059bde8 0004ff00 0059be8b > 00038200 000529ba > [ 14.56] Call Trace: [<2700>] ret_from_kernel_thread+0xc/0x14 > [ 14.56] [<004b6444>] _raw_spin_lock+0x0/0x28 > [ 14.56] [<004ad8c6>] dump_stack+0x10/0x16 > [ 14.56] [<004a6546>] spin_dump+0x6e/0x7c > [ 14.56] [<0004daf6>] do_raw_spin_lock+0x9c/0xa6 > [ 14.56] [<004b64c2>] _raw_spin_lock_irqsave+0x2a/0x34 > [ 14.56] [<002e15ea>] pmz_console_write+0x32/0x9a > [ 14.56] [<0005124e>] console_flush_all+0x112/0x3a2 > [ 14.56] [<0004ff00>] console_trylock+0x0/0x7a > [ 14.56] [<00038200>] parameq+0x48/0x6e > [ 14.56] [<000529ba>] __printk_safe_enter+0x0/0x36 > [ 14.56] [<0005113c>] console_flush_all+0x0/0x3a2 > [ 14.56] [<000542c4>] prb_read_valid+0x0/0x1a > [ 14.56] [<004b65a4>] _raw_spin_unlock+0x0/0x38 > [ 14.56] [<0005151e>] console_unlock+0x40/0xb8 > [ 14.56] [<00038200>] parameq+0x48/0x6e > [ 14.56] [<002c778c>] __tty_insert_flip_string_flags+0x0/0x14e > [ 14.56] [<00051798>] vprintk_emit+0x156/0x238 > [ 14.56] [<00051894>] vprintk_default+0x1a/0x1e > [ 14.56] [<000529a8>] vprintk+0x74/0x86 > [ 14.56] [<004a6596>] _printk+0x12/0x16 > [ 14.56] [<002e23be>] pmz_receive_chars+0x1cc/0x394 > [ 14.56] [<004b6444>] _raw_spin_lock+0x0/0x28 > [ 14.56] [<00038226>] parse_args+0x0/0x3a6 > [ 14.56] [<004b6466>] _raw_spin_lock+0x22/0x28 > [ 14.56] [<002e26b4>] pmz_interrupt+0x12e/0x1e0 > [ 14.56] [<00048680>] arch_cpu_idle_enter+0x0/0x8 > [ 14.56] [<00054ebc>] __handle_irq_event_percpu+0x24/0x106 > [ 14.56] [<004ae576>] default_idle_call+0x0/0x46 > [ 14.56] [<00055020>] handle_irq_event+0x30/0x90 > [ 14.56] [<00058320>] handle_simple_irq+0x5e/0xc0 > [ 14.56] [<00048688>] arch_cpu_idle_exit+0x0/0x8 > [ 14.56] [<00054800>] generic_handle_irq+0x3c/0x4a > [ 14.56] [<2978>] do_IRQ+0x24/0x3a > [ 14.56] [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e > [ 14.56] [<2874>] auto_irqhandler_fixup+0x4/0xc > [ 14.56] [<004ae508>] cpu_idle_poll.isra.0+0x0/0x6e > [ 14.56] [<004ae576>] default_idle_call+0x0/0x46 > [ 14.56] [<004ae598>] default_idle_call+0x22/0x46 > [ 14.56] [<00048710>] do_idle+0x6a/0xf0 > [ 14.56] [<000486a6>] do_idle+0x0/0xf0 > [ 14.56] [<000367d2>] find_task_by_pid_ns+0x0/0x2a > [ 14.56] [<0005d064>] __rcu_read_lock+0x0/0x12 > [ 14.56] [<00048a5a>] cpu_startup_entry+0x18/0x1c > [ 14.56] [<00063a06>] __rcu_read_unlock+0x0/0x26 > [ 14.56] [<004ae65a>] kernel_init+0x0/0xfa > [ 14.56] [<0049c5a8>] strcpy+0x0/0x1e > [ 14.56] [<004a6584>] _printk+0x0/0x16 > [ 14.56] [<0049c72a>] strlen+0x0/0x22 > [ 14.56] [<006452d4>] memblock_alloc_try_nid+0x0/0x82 > [ 14.56] [<0063939a>] arch_post_acpi_subsys_init+0x0/0x8 > [ 14.56] [<0063991e>] console_on_rootfs+0x0/0x60 > [ 14.56] [<00638410>] _sinittext+0x410/0xadc > [ 14.56] First of all, please read this https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
