linux-kernel - Re: [PATCH v2 8/9] serial: 8250: add acpi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160212185633.GC10095@kroah.com>
Date:	Fri, 12 Feb 2016 10:56:33 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Aleksey Makarov <aleksey.makarov@...aro.org>
Cc:	linux-acpi@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Russell King <linux@....linux.org.uk>,
	"Rafael J . Wysocki" <rjw@...ysocki.net>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	Graeme Gregory <graeme.gregory@...aro.org>,
	Al Stone <ahs3@...hat.com>,
	Christopher Covington <cov@...eaurora.org>,
	Mark Salter <msalter@...hat.com>, Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH v2 8/9] serial: 8250: add acpi_match

On Fri, Feb 12, 2016 at 08:43:39PM +0300, Aleksey Makarov wrote:
> From: Mark Salter <msalter@...hat.com>
> 
> Add an implementation of acpi_match() to the 8250 driver.
> It allows console to check if it matches the console specified by
> ACPI SPCR table.
> 
> Signed-off-by: Mark Salter <msalter@...hat.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@...aro.org>
> ---
>  drivers/tty/serial/8250/8250_core.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 7775221..059338a 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -40,6 +40,7 @@
>  #ifdef CONFIG_SPARC
>  #include <linux/sunserialcore.h>
>  #endif
> +#include <linux/acpi.h>
>  
>  #include <asm/irq.h>
>  
> @@ -669,12 +670,34 @@ static int univ8250_console_match(struct console *co, char *name, int idx,
>  	return -ENODEV;
>  }
>  
> +static int __init univ8250_console_acpi_match(struct console *co,
> +					      struct acpi_table_spcr *spcr)
> +{
> +	int index = co->index >= 0 ? co->index : 0;
> +	struct uart_port *port = &serial8250_ports[index].port;
> +
> +	if (spcr->interface_type != ACPI_DBG2_16550_SUBSET &&
> +	    spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE)
> +		return -ENODEV;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +	    spcr->serial_port.address == (u64)port->mapbase)
> +		return 0;
> +
> +	if (spcr->serial_port.space_id == ACPI_ADR_SPACE_SYSTEM_IO &&
> +	    spcr->serial_port.address == (u64)port->iobase)
> +		return 0;
> +
> +	return -ENODEV;
> +}

Ick, I don't like this, you are doing almost the same thing that
univ8250_console_match does, yet you have to know the internals of the
acpi_table_spcr structure, which is insane (again, do you want to do
this for every firmware type?)

Isn't this something that resources are supposed to handle for us?  What
happened to getting this type of information from the firmware in a
non-firmware-specific way to pass to the drivers?  Please work on that
instead, then you don't have to modify each and every single driver you
want to work with your system, they will all "just work" automagically.

Please redo this series, as it is, I don't like it at all.

thanks,

greg k-h