On 03/22/2016 07:51 PM, Yury Norov wrote:

On Tue, Mar 22, 2016 at 07:57:04AM -0700, Peter Hurley wrote:

[...]

quoted

quoted

quoted

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

quoted

 Then you'd worry about race

quoted

conditions as well. In this case, I'd consider atomic access to
variable.
2. It seems you need is_init() helper too.

quoted

+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