Message-ID: <20170316071704.GA4237@gmail.com>
Date:   Thu, 16 Mar 2017 08:17:04 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>,
        tglx@...utronix.de, peterz@...radead.org,
        linux-usb@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 0/5] usb: early: add support for early printk through
 USB3 debug port


* Lu Baolu <baolu.lu@...ux.intel.com> wrote:

> Hi Ingo,
> 
> On 03/02/2017 02:40 PM, Ingo Molnar wrote:
> > * Lu Baolu <baolu.lu@...ux.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

Powered by blists - more mailing lists