Hi, On Fri, May 24, 2024 at 06:06:21PM +0200, Petr Mladek wrote: > I have finally found time to looks at this again. Great good to hear. > First, about breaking the preferred console: > > The patchset still causes the regression with /dev/console association > as already reported for v3, see > https://lore.kernel.org/r/ZWnvc6-LnXdjOQLY@alley Thanks and sorry for missing this issue. I thought I had this issue already handled, but looking at what I tested with earlier, looks like I had the console options the wrong way around. > I used the following kernel command line: > > earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M > > The patchset caused that /dev/console became associated with > ttyS0 instead of tty0, see the "C" flag: > > original # cat /proc/consoles > tty0 -WU (EC ) 4:1 > ttyS0 -W- (E p a) 4:64 > > vs. > > patched # cat /proc/consoles > ttyS0 -W- (EC p a) 4:64 > tty0 -WU (E ) 4:1 OK > I have added some debugging messages which nicely show the reason. > In the original code, __add_preferred_console() is called twice > when processing the command line: > > [ 0.099312] __add_preferred_console[0]: ttyS, 0 (preferrred_console == 0) > [ 0.099982] __add_preferred_console[1]: tty, 0 (preferrred_console == 1) OK thanks for tracking down where things go wrong. > The code thinks that "ttyS0" has been mentioned on the command line > once again. And preferred_console is _wrongly_ set back to '0'. > > My view: > > The delayed __add_preferred_console() is a way to hell. > > The preferences are defined by the ordering on the command line. > All entries have to be added when the command line options are > being proceed to keep the order. To me it seems we can fix this by keeping track of the console position in the kernel command line. I'll send a fix for this to discuss. > A solution might be to store "devname" separately in > struct console_cmdline and allow empty "name". We could > implement then a function similar to > add_preferred_console_match() which would try to match > "devname" and set/update "name", "index" value when matched. > > Note that we might also need to add some synchronization > if it might be possible to modify struct console_cmdline > in parallel. OK certainly no objection from me if we can make this happen without making things more complex :) > Second, about the possible duplication: > > I might get it wrong. IMHO, in principle, this patchset tries > to achieve similar thing as the "match()" callback, see > the commit c7cef0a84912cab3c9 ("console: Add extensible > console matching"). > > The .match() callback in struct console is to match, for example, > console=uart8250,io,0x3f8,115200 when the uart8250 driver > calls register_console() when it is being properly initialized > as "ttyS". > > BTW: The .match() needs saved options because it internally calls > .setup() callback. IMHO, this is a very ugly detail > which complicates design of the register_console() code. > > > Both approaches try to match a "driver/device-specific name" with > the generic "ttySX". > > console=uart8250,io,0x3f8,115200 => ttyS0 > vs. > console=00:00:0.0,115200 => ttyS0 > > > Where console=uart8250,io,0x3f8,115200 is handled by: > > - "uart" is added to console_cmdline[] > - matched directly via newcon->match() callback > > vs. console=00:00:0.0,115200 > > - 00:00:0.0 is added to conopt[] > - "ttyS0" added to console_cmdline[] when "00:00:0.0" initialized > - "ttyS0" is then matched directly > > > Question: Would it it be able to use the existing .match() callback > also to match the DEVNAME? > > Or somehow reuse the approach? Thanks, I'll take a look if .match(), or some parts related to it, can be used. > Could register_console() know how to generate possible > DEVNAME for the given struct console? I don't think we can make much assumptions about the devices early on, and we also have the console index -1 issue. Regards, Tony
