Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port

[Lu Baolu]

Hi Ingo,

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