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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux