Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1602012 > unrolled thread
| Started by | Ingo Molnar <mingo@kernel.org> |
|---|---|
| First post | 2017-03-16 08:20 +0100 |
| Last post | 2017-03-17 03:40 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Ingo Molnar <mingo@kernel.org> - 2017-03-16 08:20 +0100
Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Lu Baolu <baolu.lu@linux.intel.com> - 2017-03-17 03:40 +0100
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-03-16 08:20 +0100 |
| Subject | Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port |
| Message-ID | <tlxRv-3e8-1@gated-at.bofh.it> |
* Lu Baolu <baolu.lu@linux.intel.com> wrote:
> Hi Ingo,
>
> On 03/02/2017 02:40 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> >> Hi Ingo,
> >>
> >> How about this version? Any further comments?
> > So I have re-read the review feedback I gave on Jan 19 and found at least one
> > thing I pointed out that you didn't address in the latest patches ...
>
> Do you mind telling me which one is not addressed? Is it one of below
> feedbacks?
So one piece of feedback I gave was:
| BTW., just a side note, some kernel developers (like PeterZ - and I do it
| sometimes too) remap early_printk to printk permanently and use it as their main
| printk facility - because printk() reliability has suffered over the last couple
| of years.
|
| So it's more than just early boot debugging - it's a very simple state-less
| logging facility to an external computer.
But the latest Kconfig help text still says this:
+config EARLY_PRINTK_USB_XDBC
+ bool "Early printk via the xHCI debug port"
+ depends on EARLY_PRINTK && PCI
+ select EARLY_PRINTK_USB
+ ---help---
+ Write kernel log output directly into the xHCI debug port.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized. For normal operation
+ it is not recommended because it looks ugly and doesn't cooperate
+ with klogd/syslogd or the X server. You should normally N here,
+ unless you want to debug such a crash.
... while in reality it's an alternative lockless logging facility that goes way
beyond debugging early boot crashes!
Granted, I qualified that with 'just a side note'. I guess something like this
would work:
+ One use for this feature is kernel debugging, for example when your
+ machine crashes very early before the regular console code is
+ initialized. Other uses include simpler, lockless logging instead of a
+ full-blown printk console driver + klogd.
+
+ For normal production environments this is normally not recommended,
+ because it doesn't feed events into klogd/syslogd and doesn't try to
+ print anything on the screen.
+
+ You should normally N here, unless you want to debug early crashes or
+ need a very simple printk logging facility.
Another piece of feedback I gave was:
> > +config USB_EARLY_PRINTK
> > + bool
>
> Also, could we standardize the nomencalture to not be a mixture of prefixes and
> postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig
> space) and rename this one to EARLY_PRINTK_USB or so?
yet your latest submission still includes the very same postfixed config switch
name that collides with the prefixed names such as EARLY_PRINTK_USB:
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO
config USB_EHCI_BIG_ENDIAN_DESC
bool
+config USB_EARLY_PRINTK
+ bool
+
menuconfig USB_SUPPORT
bool "USB support"
depends on HAS_IOMEM
The problem I tried to point out with my review feedback is that we thus have
both:
CONFIG_EARLY_PRINTK_USB=y
CONFIG_USB_EARLY_PRINTK=y
... which is confusing at the very least.
On a second look, this config switch appears to be unused - is it a leftover from
earlier patches?
The patches don't look too bad otherwise, so we are not far from having something
acceptable, IMHO.
Thanks,
Ingo
[toc] | [next] | [standalone]
| From | Lu Baolu <baolu.lu@linux.intel.com> |
|---|---|
| Date | 2017-03-17 03:40 +0100 |
| Message-ID | <tlPY5-7zR-3@gated-at.bofh.it> |
| In reply to | #1602012 |
Hi Ingo, On 03/16/2017 03:17 PM, Ingo Molnar wrote: > * Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> Hi Ingo, >> >> On 03/02/2017 02:40 PM, Ingo Molnar wrote: >>> * Lu Baolu <baolu.lu@linux.intel.com> wrote: >>> >>>> Hi Ingo, >>>> >>>> How about this version? Any further comments? >>> So I have re-read the review feedback I gave on Jan 19 and found at least one >>> thing I pointed out that you didn't address in the latest patches ... >> Do you mind telling me which one is not addressed? Is it one of below >> feedbacks? > So one piece of feedback I gave was: > > | BTW., just a side note, some kernel developers (like PeterZ - and I do it > | sometimes too) remap early_printk to printk permanently and use it as their main > | printk facility - because printk() reliability has suffered over the last couple > | of years. > | > | So it's more than just early boot debugging - it's a very simple state-less > | logging facility to an external computer. > > But the latest Kconfig help text still says this: > > +config EARLY_PRINTK_USB_XDBC > + bool "Early printk via the xHCI debug port" > + depends on EARLY_PRINTK && PCI > + select EARLY_PRINTK_USB > + ---help--- > + Write kernel log output directly into the xHCI debug port. > + > + This is useful for kernel debugging when your machine crashes very > + early before the console code is initialized. For normal operation > + it is not recommended because it looks ugly and doesn't cooperate > + with klogd/syslogd or the X server. You should normally N here, > + unless you want to debug such a crash. > > ... while in reality it's an alternative lockless logging facility that goes way > beyond debugging early boot crashes! > > Granted, I qualified that with 'just a side note'. I guess something like this > would work: > > + One use for this feature is kernel debugging, for example when your > + machine crashes very early before the regular console code is > + initialized. Other uses include simpler, lockless logging instead of a > + full-blown printk console driver + klogd. > + > + For normal production environments this is normally not recommended, > + because it doesn't feed events into klogd/syslogd and doesn't try to > + print anything on the screen. > + > + You should normally N here, unless you want to debug early crashes or > + need a very simple printk logging facility. Very appreciated for pointing this out. I will replace it. > > Another piece of feedback I gave was: > >>> +config USB_EARLY_PRINTK >>> + bool >> Also, could we standardize the nomencalture to not be a mixture of prefixes and >> postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig >> space) and rename this one to EARLY_PRINTK_USB or so? > yet your latest submission still includes the very same postfixed config switch > name that collides with the prefixed names such as EARLY_PRINTK_USB: > > --- a/drivers/usb/Kconfig > +++ b/drivers/usb/Kconfig > @@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO > config USB_EHCI_BIG_ENDIAN_DESC > bool > > +config USB_EARLY_PRINTK > + bool > + > menuconfig USB_SUPPORT > bool "USB support" > depends on HAS_IOMEM > > The problem I tried to point out with my review feedback is that we thus have > both: > > CONFIG_EARLY_PRINTK_USB=y > CONFIG_USB_EARLY_PRINTK=y > > ... which is confusing at the very least. > > On a second look, this config switch appears to be unused - is it a leftover from > earlier patches? Yes, it is a leftover from the earlier patches. I should remove it. Sorry about it. > > The patches don't look too bad otherwise, so we are not far from having something > acceptable, IMHO. > Thanks. For the typo and grammar issues, I will recheck the patches and ask some English speakers for review. Best regards, Lu Baolu
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web