| Message ID | 1453722324-22407-2-git-send-email-aleksey.makarov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2016-01-25 at 17:45 +0600, Aleksey Makarov wrote: > The variable preferred_console is used only inside register_console() > and it's semantics is boolean. Make it clear. This loses the index of the preferred console. I'm not sure this is better.
On 25.01.2016 18:45, Joe Perches wrote: > On Mon, 2016-01-25 at 17:45 +0600, Aleksey Makarov wrote: >> The variable preferred_console is used only inside register_console() >> and it's semantics is boolean. Make it clear. > > This loses the index of the preferred console. > I'm not sure this is better. That index is not used anywhere. I believe the patch makes things clear. Also, it indexes the array console_cmdline. With introduction of the ACPI-selected console it does not have any sense and I would have to change it anyway. Thank you Aleksey Makarov
On Mon, 2016-01-25 at 18:51 +0600, Aleksey Makarov wrote: > > On 25.01.2016 18:45, Joe Perches wrote: > > On Mon, 2016-01-25 at 17:45 +0600, Aleksey Makarov wrote: > > > The variable preferred_console is used only inside register_console() > > > and it's semantics is boolean. Make it clear. > > > > This loses the index of the preferred console. > > I'm not sure this is better. > > That index is not used anywhere. I believe the patch makes things clear. Well, with this change the name and its use is a bit misleading. Maybe changing it to something like use_selected_console is better.
On 25.01.2016 19:23, Joe Perches wrote: > On Mon, 2016-01-25 at 18:51 +0600, Aleksey Makarov wrote: >> >> On 25.01.2016 18:45, Joe Perches wrote: >>> On Mon, 2016-01-25 at 17:45 +0600, Aleksey Makarov wrote: >>>> The variable preferred_console is used only inside register_console() >>>> and it's semantics is boolean. Make it clear. >>> >>> This loses the index of the preferred console. >>> I'm not sure this is better. >> >> That index is not used anywhere. I believe the patch makes things clear. > > Well, with this change the name and its use > is a bit misleading. Maybe changing it to > something like use_selected_console is better. Thank you. I will fix this in the next version.
On Mon, Jan 25, 2016 at 1:45 PM, Aleksey Makarov <aleksey.makarov@linaro.org> wrote: > The variable preferred_console is used only inside register_console() > and it's semantics is boolean. Make it clear. However the patch looks okay it brings imbalance to understanding how exactly the preferred console is chosen. Even in case of restricted usage I would leave things as is for now. > > Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> > --- > kernel/printk/printk.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 2ce8826..37e531f 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -143,7 +143,6 @@ static struct console *exclusive_console; > static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES]; > > static int selected_console = -1; > -static int preferred_console = -1; > int console_set_on_cmdline; > EXPORT_SYMBOL(console_set_on_cmdline); > > @@ -2456,6 +2455,7 @@ void register_console(struct console *newcon) > unsigned long flags; > struct console *bcon = NULL; > struct console_cmdline *c; > + static bool preferred_console; > > if (console_drivers) > for_each_console(bcon) > @@ -2482,15 +2482,15 @@ void register_console(struct console *newcon) > if (console_drivers && console_drivers->flags & CON_BOOT) > bcon = console_drivers; > > - if (preferred_console < 0 || bcon || !console_drivers) > - preferred_console = selected_console; > + if (!preferred_console || bcon || !console_drivers) > + preferred_console = selected_console >= 0; > > /* > * See if we want to use this console driver. If we > * didn't select a console we take the first one > * that registers here. > */ > - if (preferred_console < 0) { > + if (!preferred_console) { > if (newcon->index < 0) > newcon->index = 0; > if (newcon->setup == NULL || > @@ -2498,7 +2498,7 @@ void register_console(struct console *newcon) > newcon->flags |= CON_ENABLED; > if (newcon->device) { > newcon->flags |= CON_CONSDEV; > - preferred_console = 0; > + preferred_console = true; > } > } > } > @@ -2533,7 +2533,7 @@ void register_console(struct console *newcon) > newcon->flags |= CON_ENABLED; > if (i == selected_console) { > newcon->flags |= CON_CONSDEV; > - preferred_console = selected_console; > + preferred_console = true; > } > break; > } > -- > 2.7.0 >
On 25.01.2016 20:24, Andy Shevchenko wrote: > On Mon, Jan 25, 2016 at 1:45 PM, Aleksey Makarov > <aleksey.makarov@linaro.org> wrote: >> The variable preferred_console is used only inside register_console() >> and it's semantics is boolean. Make it clear. > > However the patch looks okay it brings imbalance to understanding how > exactly the preferred console is chosen. On the contrary, I would say it makes things more clear. Also please consider this patch preparatory for the further changes. See my replies to Joe Perches.
On 01/25/2016 05:28 AM, Aleksey Makarov wrote: > > > On 25.01.2016 19:23, Joe Perches wrote: >> On Mon, 2016-01-25 at 18:51 +0600, Aleksey Makarov wrote: >>> >>> On 25.01.2016 18:45, Joe Perches wrote: >>>> On Mon, 2016-01-25 at 17:45 +0600, Aleksey Makarov wrote: >>>>> The variable preferred_console is used only inside register_console() >>>>> and it's semantics is boolean. Make it clear. >>>> >>>> This loses the index of the preferred console. >>>> I'm not sure this is better. >>> >>> That index is not used anywhere. I believe the patch makes things clear. >> >> Well, with this change the name and its use >> is a bit misleading. Maybe changing it to >> something like use_selected_console is better. > > Thank you. I will fix this in the next version. Ideally, the transform should be preferred_console => has_preferred selected_console => preferred_console This would match the actual use of add_preferred_console() Regards, Peter Hurley
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 2ce8826..37e531f 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -143,7 +143,6 @@ static struct console *exclusive_console; static struct console_cmdline console_cmdline[MAX_CMDLINECONSOLES]; static int selected_console = -1; -static int preferred_console = -1; int console_set_on_cmdline; EXPORT_SYMBOL(console_set_on_cmdline); @@ -2456,6 +2455,7 @@ void register_console(struct console *newcon) unsigned long flags; struct console *bcon = NULL; struct console_cmdline *c; + static bool preferred_console; if (console_drivers) for_each_console(bcon) @@ -2482,15 +2482,15 @@ void register_console(struct console *newcon) if (console_drivers && console_drivers->flags & CON_BOOT) bcon = console_drivers; - if (preferred_console < 0 || bcon || !console_drivers) - preferred_console = selected_console; + if (!preferred_console || bcon || !console_drivers) + preferred_console = selected_console >= 0; /* * See if we want to use this console driver. If we * didn't select a console we take the first one * that registers here. */ - if (preferred_console < 0) { + if (!preferred_console) { if (newcon->index < 0) newcon->index = 0; if (newcon->setup == NULL || @@ -2498,7 +2498,7 @@ void register_console(struct console *newcon) newcon->flags |= CON_ENABLED; if (newcon->device) { newcon->flags |= CON_CONSDEV; - preferred_console = 0; + preferred_console = true; } } } @@ -2533,7 +2533,7 @@ void register_console(struct console *newcon) newcon->flags |= CON_ENABLED; if (i == selected_console) { newcon->flags |= CON_CONSDEV; - preferred_console = selected_console; + preferred_console = true; } break; }
The variable preferred_console is used only inside register_console() and it's semantics is boolean. Make it clear. Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org> --- kernel/printk/printk.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)