linux-kernel - Re: [PATCH 36/41] drivers: tty: serial: 8250: store mmio resource size in port struct

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: <20190428151848.GO9224@smile.fi.intel.com>
Date:   Sun, 28 Apr 2019 18:18:48 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "Enrico Weigelt, metux IT consult" <info@...ux.net>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        andrew@...id.au, macro@...ux-mips.org, vz@...ia.com,
        slemieux.tyco@...il.com, khilman@...libre.com, liviu.dudau@....com,
        sudeep.holla@....com, lorenzo.pieralisi@....com,
        davem@...emloft.net, jacmet@...site.dk, linux@...sktech.co.nz,
        matthias.bgg@...il.com, linux-mips@...r.kernel.org,
        linux-serial@...r.kernel.org, linux-ia64@...r.kernel.org,
        linux-amlogic@...ts.infradead.org, linuxppc-dev@...ts.ozlabs.org,
        sparclinux@...r.kernel.org
Subject: Re: [PATCH 36/41] drivers: tty: serial: 8250: store mmio resource
 size in port struct

On Sat, Apr 27, 2019 at 02:52:17PM +0200, Enrico Weigelt, metux IT consult wrote:
> The io resource size is currently recomputed when it's needed but this
> actually needs to be done once (or drivers could specify fixed values)

io -> IO

> 
> Simplify that by doing this computation only once and storing the result
> into the mapsize field. serial8250_register_8250_port() is now called
> only once on driver init, the previous call sites now just fetch the
> value from the mapsize field.

Do I understand correctly that this has no side effects?

Which hardware did you test this on?

> @@ -979,6 +979,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>  	if (up->port.uartclk == 0)
>  		return -EINVAL;
>  
> +	/* compute the mapsize in case the driver didn't specify one */
> +	up->mapsize = serial8250_port_size(up);

I don't know all quirks in 8250 drivers by heart, though can you guarantee that
at this point the device reports correct IO resource size? (If I'm not mistaken
some broken hardware needs some magic to be done before card can be properly
handled)

> -	unsigned int size = serial8250_port_size(up);
>  	struct uart_port *port = &up->port;

> -	int ret = 0;

This and Co is a separate change that can be done in its own patch.

> +			port->membase = ioremap_nocache(port->mapbase,
> +							port->mapsize);

You may increase readability by introducing temporary variables

	... mapbase = port->mapbase;
	... mapsize = port->mapsize;
	...
	port->membase = ioremap_nocache(mapbase, mapsize);
	...

-- 
With Best Regards,
Andy Shevchenko