From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751489AbdCPHSt (ORCPT ); Thu, 16 Mar 2017 03:18:49 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:35785 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751299AbdCPHSk (ORCPT ); Thu, 16 Mar 2017 03:18:40 -0400 Date: Thu, 16 Mar 2017 08:17:04 +0100 From: Ingo Molnar To: Lu Baolu Cc: Greg Kroah-Hartman , Ingo Molnar , Mathias Nyman , tglx@linutronix.de, peterz@infradead.org, linux-usb@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 0/5] usb: early: add support for early printk through USB3 debug port Message-ID: <20170316071704.GA4237@gmail.com> References: <1487039241-596-1-git-send-email-baolu.lu@linux.intel.com> <58B7809E.4020108@linux.intel.com> <20170302064047.GA23335@gmail.com> <58B927BD.40002@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58B927BD.40002@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Lu Baolu wrote: > Hi Ingo, > > On 03/02/2017 02:40 PM, Ingo Molnar wrote: > > * Lu Baolu 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