On 03/22/2016 07:07 PM, Peter Hurley wrote:

On 03/22/2016 04:09 AM, Andy Shevchenko wrote:

quoted

On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov
[off-list ref] wrote:

quoted

quoted

+       sprintf(opts, "%s,%s,0x%llx,%d", uart, iotype,
+               table->serial_port.address, baud_rate);

You may use snprintf(), though my question here what would happen on
32-bit kernel when you supply 64-bit address as an option?

Yeah this should probably use %pa for the printf specifier.

But note this exposes underlying bug in the earlycon support, because
that was originally written without 32/64-mixed bitness in mind; ie.,
the address is parsed and handled as unsigned long in most places.

I don't quite follow this.  table->serial_port.address is explicitly u64,
not pointer, so, according to printk-formats.txt %llx is ok here, %pa is wrong.
Am I missing something?

quoted

quoted

        /*
-        * Just 'earlycon' is a valid param for devicetree earlycons;
-        * don't generate a warning from parse_early_params() in that case
+        * Just 'earlycon' is a valid param for devicetree and ACPI SPCR
+        * earlycons; don't generate a warning from parse_early_params()
+        * in that case
         */
-       if (!buf || !buf[0])
-               return early_init_dt_scan_chosen_serial();
+       if (!buf || !buf[0]) {
+               init_spcr_earlycon();

quoted

+               early_init_dt_scan_chosen_serial();
+               return 0;

And you hide an error?

Well, this is a little bit tricky because "earlycon" early parameter with
missing /chosen/stdout-path node is no longer an error, since ACPI may be
specifying the earlycon instead.

Agree, but note the email by Rob Herring.  The code should be like this:

if (!buf || !buf[0]) {
	if (acpi_disabled) {
		return early_init_dt_scan_chosen_serial();
	} else {
		init_spcr_earlycon();
		return 0;
	}
}

But that requires to have made ACPI/DT decision at this point.

Thank you
Aleksey