From: Aleksey Makarov <aleksey.makarov@linaro.org> To: Yury Norov <ynorov@caviumnetworks.com>, Peter Hurley <peter@hurleysoftware.com> 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> Subject: Re: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console Date: Tue, 22 Mar 2016 20:08:03 +0300 [thread overview] Message-ID: <56F17BF3.7090608@linaro.org> (raw) In-Reply-To: <20160322165122.GB10616@yury-N73SV> 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
WARNING: multiple messages have this Message-ID (diff)
From: aleksey.makarov@linaro.org (Aleksey Makarov) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 3/6] ACPI: parse SPCR and enable matching console Date: Tue, 22 Mar 2016 20:08:03 +0300 [thread overview] Message-ID: <56F17BF3.7090608@linaro.org> (raw) In-Reply-To: <20160322165122.GB10616@yury-N73SV> 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
next prev parent reply other threads:[~2016-03-22 17:08 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-03-22 10:46 [PATCH v5 0/6] ACPI: parse the SPCR table Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov [not found] ` <1458643595-14719-1-git-send-email-aleksey.makarov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2016-03-22 10:46 ` [PATCH v5 1/6] of/serial: move earlycon early_param handling to serial Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 11:15 ` Rob Herring 2016-03-22 11:15 ` Rob Herring 2016-03-22 11:15 ` Rob Herring 2016-03-22 16:55 ` Aleksey Makarov 2016-03-22 16:55 ` Aleksey Makarov 2016-03-22 16:55 ` Aleksey Makarov [not found] ` <1458643595-14719-2-git-send-email-aleksey.makarov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2016-03-22 12:28 ` kbuild test robot 2016-03-22 12:28 ` kbuild test robot 2016-03-22 12:28 ` kbuild test robot 2016-03-22 10:46 ` [PATCH v5 2/6] ACPI: add definitions of DBG2 subtypes Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 10:46 ` [PATCH v5 3/6] ACPI: parse SPCR and enable matching console Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 11:09 ` Andy Shevchenko 2016-03-22 11:09 ` Andy Shevchenko 2016-03-22 16:07 ` Peter Hurley 2016-03-22 16:07 ` Peter Hurley 2016-03-22 17:04 ` Aleksey Makarov 2016-03-22 17:04 ` Aleksey Makarov 2016-03-22 17:21 ` Peter Hurley 2016-03-22 17:21 ` Peter Hurley 2016-03-22 12:26 ` Yury Norov 2016-03-22 12:26 ` Yury Norov 2016-03-22 12:26 ` Yury Norov 2016-03-22 14:57 ` Peter Hurley 2016-03-22 14:57 ` Peter Hurley 2016-03-22 16:51 ` Yury Norov 2016-03-22 16:51 ` Yury Norov 2016-03-22 16:51 ` Yury Norov 2016-03-22 17:08 ` Aleksey Makarov [this message] 2016-03-22 17:08 ` Aleksey Makarov 2016-03-22 17:32 ` Peter Hurley 2016-03-22 17:32 ` Peter Hurley 2016-03-22 10:46 ` [PATCH v5 4/6] ACPI: enable ACPI_SPCR_TABLE on ARM64 Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 10:46 ` [PATCH v5 5/6] serial: pl011: add console matching function Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 10:46 ` [PATCH v5 6/6] serial: pl011: add EARLYCON_DECLARE Aleksey Makarov 2016-03-22 10:46 ` Aleksey Makarov 2016-03-22 16:15 ` Peter Hurley 2016-03-22 16:15 ` Peter Hurley 2016-03-22 17:09 ` Aleksey Makarov 2016-03-22 17:09 ` Aleksey Makarov 2016-03-22 17:41 ` Peter Hurley 2016-03-22 17:41 ` Peter Hurley
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=56F17BF3.7090608@linaro.org \ --to=aleksey.makarov@linaro.org \ --cc=ahs3@redhat.com \ --cc=cov@codeaurora.org \ --cc=graeme.gregory@linaro.org \ --cc=gregkh@linuxfoundation.org \ --cc=jslaby@suse.com \ --cc=leif.lindholm@linaro.org \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=lv.zheng@intel.com \ --cc=peter@hurleysoftware.com \ --cc=rjw@rjwysocki.net \ --cc=ynorov@caviumnetworks.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.