Subject: Re: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console
To: Peter Hurley <peter@hurleysoftware.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
References: <1458643595-14719-1-git-send-email-aleksey.makarov@linaro.org>
 <1458643595-14719-4-git-send-email-aleksey.makarov@linaro.org>
 <CAHp75VfsayWWSgkvpgrxcER6c2rZyykbvOtm7wBQROR6SFFfWw@mail.gmail.com>
 <56F16DC2.7030901@hurleysoftware.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>, Len Brown <lenb@kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        Russell King <linux@arm.linux.org.uk>,
        Leif Lindholm <leif.lindholm@linaro.org>,
        Graeme Gregory <graeme.gregory@linaro.org>, Al Stone <ahs3@redhat.com>,
        Christopher Covington <cov@codeaurora.org>,
        Yury Norov <ynorov@caviumnetworks.com>,
        "Zheng, Lv" <lv.zheng@intel.com>, Jiri Slaby <jslaby@suse.com>
From: Aleksey Makarov <aleksey.makarov@linaro.org>
Message-ID: <56F17B0B.306@linaro.org>
Date: Tue, 22 Mar 2016 20:04:11 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: <56F16DC2.7030901@hurleysoftware.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2051
Lines: 59


On 03/22/2016 07:07 PM, Peter Hurley wrote:
> On 03/22/2016 04:09 AM, Andy Shevchenko wrote:
>> On Tue, Mar 22, 2016 at 12:46 PM, Aleksey Makarov
>> <aleksey.makarov@linaro.org> wrote:

>>> +       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?

>>>         /*
>>> -        * 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();
>>
>>> +               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