Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753037AbcCVRKR (ORCPT <rfc822;w@1wt.eu>);
	Tue, 22 Mar 2016 13:10:17 -0400
Received: from mail-lf0-f48.google.com ([209.85.215.48]:34473 "EHLO
	mail-lf0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751326AbcCVRKK (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 22 Mar 2016 13:10:10 -0400
Subject: Re: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console
To: Yury Norov <ynorov@caviumnetworks.com>,
        Peter Hurley <peter@hurleysoftware.com>
References: <1458643595-14719-1-git-send-email-aleksey.makarov@linaro.org>
 <1458643595-14719-4-git-send-email-aleksey.makarov@linaro.org>
 <20160322122650.GA10616@yury-N73SV> <56F15D40.9020401@hurleysoftware.com>
 <20160322165122.GB10616@yury-N73SV>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>, Len Brown <lenb@kernel.org>,
        linux-serial@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org, 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>,
        "Zheng, Lv" <lv.zheng@intel.com>, Jiri Slaby <jslaby@suse.com>
From: Aleksey Makarov <aleksey.makarov@linaro.org>
Message-ID: <56F17BF3.7090608@linaro.org>
Date: Tue, 22 Mar 2016 20:08:03 +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: <20160322165122.GB10616@yury-N73SV>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1652
Lines: 63



On 03/22/2016 07:51 PM, Yury Norov wrote:
> On Tue, Mar 22, 2016 at 07:57:04AM -0700, Peter Hurley wrote:
> 
> [...]
> 
>>>> +static bool init_earlycon;
>>>> +
>>>> +void __init init_spcr_earlycon(void)
>>>> +{
>>>> +	init_earlycon = true;
>>>> +}
>>>> +
>>>
>>> 1. I see you keep in mind multiple access.
>>
>> Concurrent access is not a concern here: only the boot cpu is running
>> and intrs are off.
>>
>> The "init_earlycon" flag is used because parsing the "earlycon" early param
>> is earlier than parsing ACPI tables.
>>
> 
> OK got it. My concern is that it's generic code, and parse_spcr() is public
> function. I think corresponding comment is needed at least. The other option is
> to make it race-safe and forget. I prefer second one, moreover it's 2 simple
> changes.

I would not call this function public.  It is made non-static only to allow 
callin it from another compilation unit.  It should be called only once at initializatioin time.
I will add a comment about this, thank you.

>>  Then you'd worry about race
>>> conditions as well. In this case, I'd consider atomic access to
>>> variable.
>>> 2. It seems you need is_init() helper too.
>>>
>>>> +int __init parse_spcr(void)
>>>> +{
>>>> +	static char opts[64];
>>>> +	struct acpi_table_spcr *table;
>>>> +	acpi_size table_size;
>>>> +	acpi_status status;
>>>> +	char *uart;
>>>> +	char *iotype;
>>>> +	int baud_rate;
>>>> +	int err = 0;
>>>
>>> You can do not initialize 'err'.
>>
>> Why?
>>
> 
> Because there's no path here that doesn't init err with some value.
> So this initialization is useless waste of cycles.
> 
> [...]

I agree, will fix this.

Thank you
Aleksey