Re: [PATCH v2] x86/earlyprintk: Add a force option for pciserial device

[Feng Tang]

Hi Borislav,

On Sat, Sep 29, 2018 at 06:34:58PM +0200, Borislav Petkov wrote:
> On Fri, Sep 28, 2018 at 05:40:08PM +0800, Feng Tang wrote:
> > "pciserial" earlyprintk helps much on many modern x86 platforms,
> > but unfortunately there are still some platforms whose PCI UART
> > devices have wrong PCI class code, which will be blocked by current
> > class code check.
> > 
> > Add a option "force" so that developer could still use a UART device
> > even it has wrong class code, with format "",B:D.F,force,baud". And
> 
> This new parameter and its meaning needs to be documented where
> pciserial is documented.

Ok, will change the kernel-parameters.txt in next version.

> 
> > the original format ",B:D.F,baud" is kept unchanged.
> > 
> > Suggested-by: Borislav Petkov <bp@xxxxxxxxx>
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
> > ---
> >  arch/x86/kernel/early_printk.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> > index 5e801c8..35d0f66 100644
> > --- a/arch/x86/kernel/early_printk.c
> > +++ b/arch/x86/kernel/early_printk.c
> > @@ -213,8 +213,10 @@ static unsigned int mem32_serial_in(unsigned long addr, int offset)
> >   * early_pci_serial_init()
> >   *
> >   * This function is invoked when the early_printk param starts with "pciserial"
> > - * The rest of the param should be ",B:D.F,baud" where B, D & F describe the
> > - * location of a PCI device that must be a UART device.
> > + * The rest of the param should be ",B:D.F,baud" or ",B:D.F,force,baud", where
> > + * B, D & F describe the location of a PCI device that must be a UART device,
> > + * "force" is optional and means insisting using a UART device with a wrong
> > + * pci class code.
> >   */
> >  static __init void early_pci_serial_init(char *s)
> >  {
> > @@ -224,6 +226,7 @@ static __init void early_pci_serial_init(char *s)
> >  	u32 classcode, bar0;
> >  	u16 cmdreg;
> >  	char *e;
> > +	int force = 0;
> >  
> >  
> >  	/*
> > @@ -252,6 +255,15 @@ static __init void early_pci_serial_init(char *s)
> >  	if (*s == ',')
> >  		s++;
> >  
> > +	/* User may insist to use a UART device with wrong class code */
> > +	if (!strncmp(s, "force", 5)) {
> > +		force = 1;
> > +		s += 5;
> > +
> > +		if (*s == ',')
> > +			s++;
> 
> No, you need to force the presence of "," otherwise cmdlines like this:
> 
> earlyprintk=pciserial,0:XX.X,forcedoodoo
> 
> work too.

Got your point. As the following [,baudrate] is also optional, and this
"force" could be the last option for pciserial. The check may be

	if (!strncmp(s, "force,", 6) || !strncmp(s, "force ", 6)) {
		force = 1;
		s += 6;
	}


> > +	}
> > +
> >  	/*
> >  	 * Second, find the device from the BDF
> >  	 */
> > @@ -262,10 +274,12 @@ static __init void early_pci_serial_init(char *s)
> >  	/*
> >  	 * Verify it is a UART type device
> >  	 */
> > -	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> > -	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> > -	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> > -		return;
> > +	if (!force) {
> > +		if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
> > +		     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
> > +		   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */
> > +			return;
> 
> Move the force check in here ^

Do you mean this? 

	if (((classcode >> 16 != PCI_CLASS_COMMUNICATION_MODEM) &&
	     (classcode >> 16 != PCI_CLASS_COMMUNICATION_SERIAL)) ||
	   (((classcode >> 8) & 0xff) != 0x02)) /* 16550 I/F at BAR0 */ {
		if (!force)
			return;
	}

Thanks,
Feng