Re: [PATCH RFC] x86/earlyprintk: Don't fail the pciserial device with incorrect class code

From: Borislav Petkov <bp@alien8.de>
To: Feng Tang <feng.tang@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	H Peter Anvin <hpa@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Stuart R . Anderson" <stuart.r.anderson@intel.com>,
	alan@linux.intel.com, x86@kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC] x86/earlyprintk: Don't fail the pciserial device with incorrect class code
Date: Thu, 27 Sep 2018 15:30:50 +0200	[thread overview]
Message-ID: <20180927133050.GA19687@zn.tnic> (raw)
In-Reply-To: <20180927124320.13419-1-feng.tang@intel.com>

On Thu, Sep 27, 2018 at 08:43:20PM +0800, Feng Tang wrote:
> "pciserial" earlyprintk helps much on many modern x86 platforms,
> but unfortunately there are some platforms whose PCI UART devices
> have wrong PCI class code, which will be blocked by current check.
> 
> So loose the class code check by giving a warning message instead.
> This should be fine, as a developer who can give the accurate
> BDF should know whether it's a usable UART device.

BDF? No.

Please write it out what it means.

> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  arch/x86/kernel/early_printk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index 5e801c8..abe1d08 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -265,7 +265,8 @@ static __init void early_pci_serial_init(char *s)
>  	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
>  	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
>  	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> -		return;
> +		pr_warn("earlyprintk: classcode for pcidev %d:%d:%d shows it's not a UART like device, please check!\n",
> +			bus, slot, func);

So where did the return statement go?

What are you trying to do here? If the device is still an UART device
then we don't need the check at all as you're basically overriding it
and only the class code is wrong.

If so, why do we need the pr_warn at all? What can the user do about the
class code? Nothing, I'd say. I don't see her "fixing" PCI config space
so that the device has a correct class code.

But what happens if the user really supplies the wrong BDF? We end up
poking in PCI config space of *some* device and then the cat might catch
fire.

So it sounds to me like we need:

earlyprintk=pciserial,force,00:18.1,115200

where "force" allows the function to continue even if the classcode is
wrong. Or

earlyprintk=pciserial,i_know_what_im_doing,00:18.1,115200

:-)

Also, if you're going to use pr_* variants, convert the file to use
pr_fmt() - grep the kernel sources for examples.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.