Re: [PATCH v3 3/3] serial: core: Rename preferred console handling for match and update

[Tony Lindgren]

On Tue, Jun 18, 2024 at 03:51:44PM +0200, Petr Mladek wrote:
> On Tue 2024-06-18 07:54:50, Tony Lindgren wrote:
> > We are now matching and updating the preferred console, not adding it.
> > Let's update the naming accordingly to avoid confusion.
> > 
> > --- a/drivers/tty/serial/serial_base_bus.c
> > +++ b/drivers/tty/serial/serial_base_bus.c
> > @@ -304,7 +305,7 @@ int serial_base_add_preferred_console(struct uart_driver *drv,
> 
> I was curious whether this patch renamed everything. And it seems
> that it did not rename serial_base_add_preferred_console().

Oops, will update the naming for that too.

> >  	const char *port_match __free(kfree) = NULL;
> >  	int ret;
> >  
> > -	ret = serial_base_add_prefcon(drv->dev_name, port->line);
> > +	ret = serial_base_match_and_update_prefcon(drv->dev_name, port->line);
> >  	if (ret)
> >  		return ret;
> >  
> 
> Honestly, I do not understand what are all these layers for.
> Especially, serial_base_match_and_update_prefcon() looks suspicious:
> 
> static int serial_base_match_and_update_prefcon(const char *name, int idx)
> {
> 	const char *char_match __free(kfree) = NULL;
> 	const char *nmbr_match __free(kfree) = NULL;
> 	int ret;
> 
> 	/* Handle ttyS specific options */
> 	if (strstarts(name, "ttyS")) {
> 		/* No name, just a number */
> 		nmbr_match = kasprintf(GFP_KERNEL, "%i", idx);
> 		if (!nmbr_match)
> 			return -ENODEV;
> 
> 		ret = serial_base_match_and_update_one_prefcon(nmbr_match, name, idx);
> 		if (ret)
> 			return ret;
> 
> 		/* Sparc ttya and ttyb */
> 		ret = serial_base_add_sparc_console(name, idx);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	/* Handle the traditional character device name style console=ttyS0 */
> 	char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx);
> 	if (!char_match)
> 		return -ENOMEM;
> 
> 	return serial_base_match_and_update_one_prefcon(char_match, name, idx);
> }
> 
> It seems to try whether c->devname matches a number "X", or "ttySX".
> It even tries the sparc-specific transformations in
> serial_base_add_sparc_console()
> 
> But this is the original format which does _not_ include ":".
> It never will be stored in c->devname and will never match.

Good catch, this won't do anything now with console_setup()
checking for ":" for deferred consoles. So we should revert commit
a0f32e2dd998 ("serial: core: Handle serial console options").

> I think that it has been the case even before this patchset.

For the earlier case, I tested things with serial handling removed
from console_setup() to let the serial layer handle the quirks.

With the new handling, we could just eventually defer the serial
consoles in console_setup(), and let the serial core do the quirk
handling. No immediate need for that though, that would be just
longer term clean-up.

> I think that we should remove these layers and check just
> the "DEVNAME:X.Y" format, aka "%s:%d.%d" [*].

Yes let's revert the quirk handling.
 
> [*] It would be nice to use the same printf format "%s:%d.%d"
>     in both serial_base_device_init() and also in the functions
>     matching the devname to make it clear that these are
>     the same names. Heh, I just guess that these are the same
>     names.

I'll do a separate clean-up patch for that, and that can then
be used for the match() too eventually.

Regards,

Tony