Re: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console

[Aleksey Makarov]

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