[v9,4/4] serial: pl011: add console matching function

Message ID 20160811153152.755-5-aleksey.makarov@linaro.org
State Superseded
Headers show

Commit Message

Aleksey Makarov Aug. 11, 2016, 3:31 p.m.
This patch adds function pl011_console_match() that implements
method match of struct console.  It allows to match consoles against
data specified in a string, for example taken from command line or
compiled by ACPI SPCR table handler.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

---
 drivers/tty/serial/amba-pl011.c | 56 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Aleksey Makarov Aug. 15, 2016, 1:10 p.m. | #1
On 08/15/2016 03:39 PM, Russell King - ARM Linux wrote:
> On Thu, Aug 11, 2016 at 06:31:41PM +0300, Aleksey Makarov wrote:

>> +/**

>> + *	pl011_console_match - non-standard console matching

>> + *	@co:	  registering console

>> + *	@name:	  name from console command line

>> + *	@idx:	  index from console command line

>> + *	@options: ptr to option string from console command line

>> + *

>> + *	Only attempts to match console command lines of the form:

>> + *	    console=pl011,mmio|mmio32,<addr>[,<options>]

>> + *	    console=pl011,0x<addr>[,<options>]

>> + *	This form is used to register an initial earlycon boot console and

>> + *	replace it with the amba_console at pl011 driver init.

>> + *

>> + *	Performs console setup for a match (as required by interface)

>> + *	If no <options> are specified, then assume the h/w is already setup.

>> + *

>> + *	Returns 0 if console matches; otherwise non-zero to use default matching

>> + */

>> +static int __init pl011_console_match(struct console *co, char *name, int idx,

>> +				      char *options)

>> +{

>> +	char match[] = "pl011";	/* pl011-specific earlycon name */

> 

> This is inefficient - the compiler will probably store "pl011" in the

> rodata section, then allocate an array on the stack, and them memcpy()

> it onto the stack.  This is really a false optimisation.

> 

>> +	unsigned char iotype;

>> +	unsigned long addr;

>> +	int i;

>> +

>> +	if (strncmp(name, match, 5) != 0)

> 

> Just do:

> 

> 	if (strncmp(name, "pl011", 5) != 0)

> 

> here, and let the compiler work it out - it'll probably place "pl011"

> in the rodata section, and use a pointer to it rather than messing around

> with the stack.


Thank you, I will change it.

> What if "console=pl011x,..." is passed?  Should this be matched too?

> Maybe this should compare with "pl011," to ensure that the name is

> correctly terminated?


You are right, if the argument 'name' is longer than just "pl011" it
will wrongly match.  But note that it's always just name of the console
without ',' and parameters that is passed in the 'name' argument.  So I
think we should just use strcmp() here.

BTW, the function univ8250_console_match() from
drivers/tty/serial/8250/8250_core.c probably has the same issue.

Thank you
Aleksey Makarov
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8a9e213..d6a51cb 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2288,12 +2288,68 @@  static int __init pl011_console_setup(struct console *co, char *options)
 	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
 }
 
+/**
+ *	pl011_console_match - non-standard console matching
+ *	@co:	  registering console
+ *	@name:	  name from console command line
+ *	@idx:	  index from console command line
+ *	@options: ptr to option string from console command line
+ *
+ *	Only attempts to match console command lines of the form:
+ *	    console=pl011,mmio|mmio32,<addr>[,<options>]
+ *	    console=pl011,0x<addr>[,<options>]
+ *	This form is used to register an initial earlycon boot console and
+ *	replace it with the amba_console at pl011 driver init.
+ *
+ *	Performs console setup for a match (as required by interface)
+ *	If no <options> are specified, then assume the h/w is already setup.
+ *
+ *	Returns 0 if console matches; otherwise non-zero to use default matching
+ */
+static int __init pl011_console_match(struct console *co, char *name, int idx,
+				      char *options)
+{
+	char match[] = "pl011";	/* pl011-specific earlycon name */
+	unsigned char iotype;
+	unsigned long addr;
+	int i;
+
+	if (strncmp(name, match, 5) != 0)
+		return -ENODEV;
+
+	if (uart_parse_earlycon(options, &iotype, &addr, &options))
+		return -ENODEV;
+
+	if (iotype != UPIO_MEM && iotype != UPIO_MEM32)
+		return -ENODEV;
+
+	/* try to match the port specified on the command line */
+	for (i = 0; i < ARRAY_SIZE(amba_ports); i++) {
+		struct uart_port *port;
+
+		if (!amba_ports[i])
+			continue;
+
+		port = &amba_ports[i]->port;
+
+		if (port->mapbase != addr)
+			continue;
+
+		co->index = i;
+		port->cons = co;
+		return pl011_console_setup(co, options);
+	}
+
+	return -ENODEV;
+}
+
 static struct uart_driver amba_reg;
 static struct console amba_console = {
 	.name		= "ttyAMA",
 	.write		= pl011_console_write,
 	.device		= uart_console_device,
 	.setup		= pl011_console_setup,
+	.match		= pl011_console_match,
 	.flags		= CON_PRINTBUFFER,
 	.index		= -1,
 	.data		= &amba_reg,