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@xxxxxxxxx>
> > ---
> >  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