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

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

[Feng Tang]

"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.

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);
 
 	/*
 	 * Determine if it is IO or memory mapped
-- 
2.7.4

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

[Borislav Petkov]

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.

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

[Feng Tang]

Hi Borislav,

Thanks for the review.

On Thu, Sep 27, 2018 at 03:30:50PM +0200, Borislav Petkov wrote:
> 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.

Will do.

> > 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.

That's true. We can hardly do anything from OS side,

> 
> 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.

Valid point, I can not argue on this :)

> 
> So it sounds to me like we need:
> 
> earlyprintk=pciserial,force,00:18.1,115200

I would follow this way, thanks

-Feng