Date:	Wed, 27 May 2015 20:21:21 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Bin Gao <bin.gao@...ux.intel.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] serial_core: add pci uart early console support

On 05/27/2015 07:14 PM, Bin Gao wrote:
> Peter Hurley,
> First of all, thank you for your reviewing.
> Please see my answers below.
> 
> On Tue, May 26, 2015 at 01:12:34PM -0400, Peter Hurley wrote:
>> Hi Bin,
>>
>> Please don't drop lists (or other addressees) from patch revisions.
>>
>> [ +cc linux-serial]
> Will fix this.
> 
> 
>> Please update Documentation/kernel-parameters.txt with pci-specific
>> earlycon parameter formats.
> Will do.
> 
>  
>> Please move this hunk to patch 2/2.
> It's already in 2/2. Or I'm not quite understanding?

I meant that the patch hunk below should be moved to patch
2/2, and the purpose of patch 2/2 should be to replace x86-specific
earlyprintk=pciserial with arch-independent earlyprintk=pciserial.

There are 2 reasons for my suggestion:
1) Changes to x86 earlyprintk should get acks from the x86 maintainers.
   Borislav Petkov <bp@...e.de> is particularly involved in the
   'early' earlyprintk proposal. Patch 2/2 should cc them and the
   author of the original earlyprintk=pciserial patch.
2) Changes to x86 earlyprintk may be rejected.


> +/* x86 uses "earlyprintk=xxx", so we keep the compatibility here */
> +#ifdef CONFIG_X86
> +static int __init param_setup_earlycon_x86(char *buf)
> +{
> +	return param_setup_earlycon(buf);
> +}
> +early_param("earlyprintk", param_setup_earlycon_x86);
> +#endif
> +



>> FYI, there is a proposal to evaluate "earlyprintk=" earlier than
>> parse_early_params(), which this would break.
>>
>> https://lkml.org/lkml/2015/5/18/127
> No, it wouldn't. The proposal only moves earlyprintk=serial(legacy 0x3f8).
> If you look into arch/x86/kernel/early_printk.c, you'll find many other
> options other than serial, e.g. vga, dbgp, xen, efi, pciserial, etc.
> uart8250(previously pciserial) is one of these "others".

I see; so when re-evaluating earlyprintk= for earlycon, there is no danger 
of corrupting the device state for earlyprintk?

However, the namespace will now be shared so that is an important change
that maintainers and future earlycon/earlyprintk authors need to know.

>>> +static int parse_bdf(char *options, char **endp, char delimiter, u8 *val)
>>
>> If this function is only used from parse_pci_options(), please enclose it
>> in #ifdef CONFIG_PCI_EARLY scope with parse_pci_options().
> Yes, will fix it.
> 
> 
>>
>>> +{
>>> +	char str[4]; /* max 3 chars, plus a NULL terminator */
>>> +	char *p = options;
>>> +	int i = 0;
>>> +
>>> +	while (*p) {
>>> +		if (i >= 4)
>>> +			return -EINVAL;
>>> +
>>> +		if (*p == delimiter) {
>>> +			str[i++] = 0;
>>> +			if (endp)
>>> +				*endp = p + 1;
>>> +			return kstrtou8(str, 10, val); /* decimal, no hex */
>>> +		}
>>> +
>>> +		str[i++] = *p++;
>>> +	}
>>
>> Is all this to avoid using simple_strtoul()?
>> If yes, I'd rather you use simple_strtoul() like the rest of the console
>> code and ignore the (misguided) advice that simple_strtoul() is obsolete.
> Then scripts/checkpatch.pl should be updated to avoid this kind of "misguiding".
> Also the kstrto* API descriptions should be updated...

I know; I've already made that point to Joe.

>> The code above is exactly what is wrong with the kstrto* api.
> Can you elaborate a little bit? Thanks.

I don't mean there is anything wrong with _your_ code, per se.
Rather that it should not be necessary to write 14 lines of code, use temp
storage and visit the entire string three times simply to parse a string
into a number. IOW, the kstrto* API usefulness is limited, which is why
simple_strto* is _not_ obsolete.

 
>>> +	if (!early_pci_allowed()) {
>>> +		pr_err("earlycon pci not available(early pci not allowed)\n");
>>
>> Error message is redundant.
> "early pci not allowed" is the reason of "earlycon pci not available".

Only one or the other is necessary. For example,

	"earlycon: early pci not allowed\n"


>>> +	/*
>>> +	 * On these platforms class code in pci config is broken,
>>               ^^^^^^^^^^^^^^^
>> which platforms?
> On platforms where this code will run on.
> Do you prefer something like:
> On platforms with early pci serial class code in pci config is broken,

This statement doesn't make sense to me: the original earlyprintk=pciserial
implementation read the class code register, so it worked then.

Why not now?


>>> +	pr_info("Use 8250 uart at PCI 0000:%02u:%02u.%01u as early console\n",
>>> +							bus, dev, func);
>>
>> I think one earlycon banner is sufficient; you could make this pr_debug()
>> instead. Existing convention for earlycon messages is
>>
>> 	"earlycon: ...."
> Will use pr_debug().
> 
> 
>>>   *	@options: ptr for <options> field; NULL if not present (out)
>>>   *
>>>   *	Decodes earlycon kernel command line parameters of the form
>>> - *	   earlycon=<name>,io|mmio|mmio32,<addr>,<options>
>>> + *	   earlycon=<name>,io|mmio|mmio32|pci|pci32,<addr>,<options>
>>
>> Document the pci/pci32 format separately because
>> 	earlycon=uart8250,pci32,<addr>,<options> is not a valid form.
> Why it's not a valid form? It follows the existed syntax like this:
> earlycon=uart8250,<io type>,<addr>,<options>

Well, isn't <addr> being retrieved from BAR0? Why would you specify
the <addr> on the command line?

But I see now that's where the 'bus:device.function' goes.
I think if I was confused, others will be too. Further, in the context
of this code 'addr' is a variable to which the command line parameter
comment identifies, whereas 'bus:device.function' does not follow
the same convention.


>>> @@ -1829,12 +1939,23 @@ uart_get_console(struct uart_port *ports, int nr, struct console *co)
>>>  int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
>>>  			char **options)
>>>  {
>>> +	int pci = 0, ret;
>>> +	unsigned long phys;
>>> +
>>>  	if (strncmp(p, "mmio,", 5) == 0) {
>>>  		*iotype = UPIO_MEM;
>>>  		p += 5;
>>>  	} else if (strncmp(p, "mmio32,", 7) == 0) {
>>>  		*iotype = UPIO_MEM32;
>>>  		p += 7;
>>> +	} else if (strncmp(p, "pci,", 4) == 0) {
>>> +		pci = 1;
>>> +		p += 4;
>>> +		ret = parse_pci_options(p, &phys);
>>> +	} else if (strncmp(p, "pci32,", 6) == 0) {
>>> +		pci = 2;
>>> +		p += 6;
>>> +		ret = parse_pci_options(p, &phys);
>>>  	} else if (strncmp(p, "io,", 3) == 0) {
>>>  		*iotype = UPIO_PORT;
>>>  		p += 3;
>>> @@ -1844,7 +1965,27 @@ int uart_parse_earlycon(char *p, unsigned char *iotype, unsigned long *addr,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	*addr = simple_strtoul(p, NULL, 0);
>>> +	if (pci) {
>>> +		if (ret < 0) /* error */
>>> +			return ret;
>>> +
>>> +		/*
>>> +		 * Once PCI mem/io is read from PCI BAR, we can reuse
>>> +		 * mmio/mmio32/io type to minimize code change.
>>> +		 */
>>> +		if (ret > 0) /* PCI io */
>>> +			*iotype = UPIO_PORT;
>>> +		else { /* ret = 0: PCI mem */
>>> +			if (pci == 2)
>>> +				*iotype = UPIO_MEM32;
>>> +			else
>>> +				*iotype = UPIO_MEM;
>>> +		}
>>> +
>>> +		*addr = phys;
>>> +	} else
>>> +		*addr = simple_strtoul(p, NULL, 0);
>>> +
>>
>> I'd like to see this refactored without the logic steering locals.
>> Like this:
>>
>>  	if (strncmp(p, "mmio,", 5) == 0) {
>>  		*iotype = UPIO_MEM;
>>  		p += 5;
>> +		*addr = simple_strtoul(p, NULL, 0);
>>  	} else if (strncmp(p, "mmio32,", 7) == 0) {
>>  		*iotype = UPIO_MEM32;
>>  		p += 7;
>> +		*addr = simple_strtoul(p, NULL, 0);
>> +	} else if (strncmp(p, "pci,", 4) == 0) {
>> +		pci = 1;
>> +		p += 4;
>> +		ret = parse_pci_options(p, addr);
>> +		if (ret < 0)
>> +			return ret;
>> +		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM;
>> +	} else if (strncmp(p, "pci32,", 6) == 0) {
>> +		pci = 2;
>> +		p += 6;
>> +		ret = parse_pci_options(p, addr);
>> +		if (ret < 0)
>> +			return ret;
>> +		*iotype = (ret > 0) ? UPIO_PORT : UPIO_MEM32;
>>  	} else if (strncmp(p, "io,", 3) == 0) {
>>  		*iotype = UPIO_PORT;
>>  		p += 3;
>> +		*addr = simple_strtoul(p, NULL, 0);
>>
>>
>> Regards,
>> Peter Hurley
>>
>>> -	*addr = simple_strtoul(p, NULL, 0);
>>>  	p = strchr(p, ',');
>>>  	if (p)
>>>  		p++;
>>>
> Ah, this is kind of coding style. But I agree with your suggestion. Will fix it.

Thanks.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists