[PATCH] 8250: Fix oops from setserial

[PATCH] 8250: Fix oops from setserial

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

[Impact: ok I just put this here to wind up Linus]

If you setserial a port which has never been initialised we change the type
but don't update the I/O method pointers. The same problem is true if you
change the io type of a port - but nobody ever does that so nobody noticed!

Remember the old type and when attaching if the type has changed reload the
port accessor pointers. We can't do it blindly as some 8250 drivers load custom
accessors and we must not stomp those.

Closes-bug: #13367
Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/serial/8250.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)


diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index b4b3981..a0127e9 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -137,6 +137,7 @@ struct uart_8250_port {
 	unsigned char		mcr;
 	unsigned char		mcr_mask;	/* mask of user bits */
 	unsigned char		mcr_force;	/* mask of forced bits */
+	unsigned char		cur_iotype;	/* Running I/O type */
 
 	/*
 	 * Some bits in registers are cleared on a read, so they must
@@ -471,6 +472,7 @@ static void io_serial_out(struct uart_port *p, int offset, int value)
 
 static void set_io_from_upio(struct uart_port *p)
 {
+	struct uart_8250_port *up = (struct uart_8250_port *)p;
 	switch (p->iotype) {
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
@@ -509,6 +511,8 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = io_serial_out;
 		break;
 	}
+	/* Remember loaded iotype */
+	up->cur_iotype = p->iotype;
 }
 
 static void
@@ -1937,6 +1941,9 @@ static int serial8250_startup(struct uart_port *port)
 	up->capabilities = uart_config[up->port.type].flags;
 	up->mcr = 0;
 
+	if (up->port.iotype != up->cur_iotype)
+		set_io_from_upio(port);
+
 	if (up->port.type == PORT_16C950) {
 		/* Wake up and initialize UART */
 		up->acr = 0;
@@ -2563,6 +2570,9 @@ static void serial8250_config_port(struct uart_port *port, int flags)
 	if (ret < 0)
 		probeflags &= ~PROBE_RSA;
 
+	if (up->port.iotype != up->cur_iotype)
+		set_io_from_upio(port);
+
 	if (flags & UART_CONFIG_TYPE)
 		autoconfig(up, probeflags);
 	if (up->port.type != PORT_UNKNOWN && flags & UART_CONFIG_IRQ)
@@ -2671,6 +2681,11 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 {
 	int i;
 
+	for (i = 0; i < nr_uarts; i++) {
+		struct uart_8250_port *up = &serial8250_ports[i];
+		up->cur_iotype = 0xFF;
+	}
+
 	serial8250_isa_init_ports();
 
 	for (i = 0; i < nr_uarts; i++) {

Re: [PATCH] 8250: Fix oops from setserial

[Andrew Morton]

On Thu, 28 May 2009 14:01:35 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> From: Alan Cox <alan@linux.intel.com>
> 
> [Impact: ok I just put this here to wind up Linus]
> 
> If you setserial a port which has never been initialised we change the type
> but don't update the I/O method pointers. The same problem is true if you
> change the io type of a port - but nobody ever does that so nobody noticed!
> 
> Remember the old type and when attaching if the type has changed reload the
> port accessor pointers. We can't do it blindly as some 8250 drivers load custom
> accessors and we must not stomp those.
> 
> Closes-bug: #13367

Please quote the full bug URL.

a) for consistency

b) because there are multiple bug tracking systems out there:

y:/usr/src/git26> git-log|grep bugzilla.redhat|wc -l
123

>  static void set_io_from_upio(struct uart_port *p)
>  {
> +	struct uart_8250_port *up = (struct uart_8250_port *)p;

container_of() is nicer, IMO.  It's clearer, and doesn't require that
the uart_port be the fist member.

Re: [PATCH] 8250: Fix oops from setserial

[Alan Cox]

> 
> >  static void set_io_from_upio(struct uart_port *p)
> >  {
> > +	struct uart_8250_port *up = (struct uart_8250_port *)p;
> 
> container_of() is nicer, IMO.  It's clearer, and doesn't require that
> the uart_port be the fist member.

See the rest of the driver - it was written that way years ago and this
is just following the existing design.

Re: [PATCH] 8250: Fix oops from setserial

[Andrew Morton]

On Sat, 30 May 2009 09:25:57 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > 
> > >  static void set_io_from_upio(struct uart_port *p)
> > >  {
> > > +	struct uart_8250_port *up = (struct uart_8250_port *)p;
> > 
> > container_of() is nicer, IMO.  It's clearer, and doesn't require that
> > the uart_port be the fist member.
> 
> See the rest of the driver - it was written that way years ago and this
> is just following the existing design.

Sure, huge numbers of drivers use the cast.

But I've never seen past sinnings as being a reason to continue sinning. 
Doing new code the right way doesn't result in worse code, and reduces
the chances of someone copying and pasting the wrong way.

Re: [PATCH] 8250: Fix oops from setserial

[Alan Cox]

> Sure, huge numbers of drivers use the cast.
> 
> But I've never seen past sinnings as being a reason to continue sinning. 
> Doing new code the right way doesn't result in worse code, and reduces
> the chances of someone copying and pasting the wrong way.

As I said please read the rest of the driver. It would ridiculous to have
one dereference done differently to the rest in several thousand lines of
code.

It's not a new driver, it's simply pasting a line from one place to
another to update a field.

Re: [PATCH] 8250: Fix oops from setserial

[Andrew Morton]

On Sat, 30 May 2009 11:46:32 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Sure, huge numbers of drivers use the cast.
> > 
> > But I've never seen past sinnings as being a reason to continue sinning. 
> > Doing new code the right way doesn't result in worse code, and reduces
> > the chances of someone copying and pasting the wrong way.
> 
> As I said please read the rest of the driver.

I'm not the reading-averse one here :(

> It would ridiculous to have
> one dereference done differently to the rest in several thousand lines of
> code.

Actually, it would be typical.  Many many drivers which originally did
something consistently wrong have, over time, grown to contain a mix of
right and wrong.  It's one of the costs of doing things wrong.

> It's not a new driver, it's simply pasting a line from one place to
> another to update a field.

You already said all this.

Look, it's a single silly line, but there's a non-trivial point here. 
It comes up quite regularly and I always encourage people to do things
the right way rather than matching the existing wrong code.  Because,
when you think about it, there's really no merit in having consistently
wrong code.  A mix of right and wrong is better than 100% wrong.

If it results in inconsistent-looking code then that's good.  It may
result in some drive-by developer noticing the inconsistency and fixing
everything up.

Have a think about it.