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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
