On 03/16/2017 04:54 PM, Petr Mladek wrote:
> On Thu 2017-03-16 13:36:35, Aleksey Makarov wrote:
>>
>>
>> On 03/15/2017 07:58 PM, Petr Mladek wrote:
>>> On Wed 2017-03-15 13:28:52, Aleksey Makarov wrote:
[..]
> I personally prefer v4. The braille console adds an unexpected
> twist into the code because it skips the rest of the function.
> IMHO, it is better when it is is tested separately with clear
> conditions and comments.
>
> I would personally add the following into
> kernel/printk/console_cmdline.h
>
> #define for_each_console_cmdline(c, i) \
> for (i = 0, c = console_cmdline; \
> i < MAX_CMDLINECONSOLES && c->name[0]; \
> i++, c++)
>
> It can be used in both __add_preferred_console()
> and register_console().
Ok
> Also I would add the following into kernel/printk/braille.h
>
> static inline bool
> is_braille_console(struct console_cmdline *c)
> {
> return !!c->brl_options;
> }
>
> Then the braille-specific code in register_console() might
> look like:
>
> /*
> * Braille console is handled a very special way.
> * Is is not listed in the console_drivers list.
> * Instead it registers its own keyboard and vt notifiers.
> *
> * Be careful and avoid calling c->match() here
> * because it might have side effects!
> */
> if (is_braille_console(c) &&
> match_console_name(newcon, c) &&
> _braille_register_console(newcon, c))
> return;
Yes, this is exactly what I was going to do.
> Finally, I would prefer to move
>
> newcon->flags |= CON_ENABLED;
>
> outside the match_console() function. The function will still
> have side effects because of the c->match() calls. But we should
> not make it worse. In fact, it would be great to remove side effects
> from the c->match() functions in the long term (not in this patch set).
I see the motivation but I am afraid this is not possible until
side effects are removed from newcon->match() and match_console().
The point is that match_console() also calls newcon->setup() (side effect)
and this CON_ENABLED flag indicates if this call was successful.
>> I am going to fix v5 preserving both the check for duplicates
>> and the invariant, but tell me please if you prefer the v4 approach.
>
> I guess that you planed to shuffle console_cmdline entries to keep
> the preferred console first/last. I am afraid that it won't be
> a nice code either. But I might be wrong.
You are right, I tried that, the code was awful.
Thank you
Aleksey Makarov
>>> The
>>> newcon->setup() call is called only when the console matches.
>>> AFAIK, there is only one braille console. We should be on
>>> the safe side if this one does not implement the match()
>>> callback. Or is it even more complicated?
>>
>> As you can see from the original code, the check for braille console
>> was performed in that branch of code where we missed newcon->match(),
>> so yes, it looks like braille console(s) does not have the match() method.
>> I used that in v4 to factor out matching for braille from the loop.
>
> The check for blr_options is sufficient and better.
>
> I suggest to wait at least two days until you spend to much time
> on reshuffling the code. It is possible that others would prefer
> v5 or suggest even other solution.
>
> Best Regards,
> Petr
> --
> 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
>
--
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
