From: "Thomas Pfaff" <[email protected]>
While porting a RS485 driver from 2.6.29 to 3.14, i noticed that the serial tty
driver could break it by using uart ports that it does not own :
1. uart_change_pm ist called during uart_open and calls the uart pm function
without checking for PORT_UNKNOWN.
2. uart_shutdown is called from uart_set_info and does not check it either.
3. The return code from the uart request_port call in uart_set_info is not
handled properly, leading to the situation that the serial driver also
thinks it owns the uart ports.
This can triggered by doing following actions :
setserial /dev/ttyS0 uart none # release the uart ports
modprobe lirc-serial # or any other device that uses the uart
setserial /dev/ttyS0 uart 16550 # gives no error and the uart tty driver
# can use the ports as well
Signed-off-by: Thomas Pfaff <[email protected]>
---
diff -urp linux-3.14.org/drivers/tty/serial/serial_core.c linux-3.14/drivers/tty/serial/serial_core.c
--- linux-3.14.org/drivers/tty/serial/serial_core.c 2014-03-31 05:40:15.000000000 +0200
+++ linux-3.14/drivers/tty/serial/serial_core.c 2014-04-08 16:51:28.176459065 +0200
@@ -225,6 +225,9 @@ static void uart_shutdown(struct tty_str
struct uart_port *uport = state->uart_port;
struct tty_port *port = &state->port;
+ if (uport->type == PORT_UNKNOWN)
+ return;
+
/*
* Set the TTY IO error marker
*/
@@ -826,25 +829,29 @@ static int uart_set_info(struct tty_stru
* If we fail to request resources for the
* new port, try to restore the old settings.
*/
- if (retval && old_type != PORT_UNKNOWN) {
+ if (retval) {
uport->iobase = old_iobase;
uport->type = old_type;
uport->hub6 = old_hub6;
uport->iotype = old_iotype;
uport->regshift = old_shift;
uport->mapbase = old_mapbase;
- retval = uport->ops->request_port(uport);
- /*
- * If we failed to restore the old settings,
- * we fail like this.
- */
- if (retval)
- uport->type = PORT_UNKNOWN;
- /*
- * We failed anyway.
- */
- retval = -EBUSY;
+ if (old_type != PORT_UNKNOWN) {
+ retval = uport->ops->request_port(uport);
+ /*
+ * If we failed to restore the old settings,
+ * we fail like this.
+ */
+ if (retval)
+ uport->type = PORT_UNKNOWN;
+
+ /*
+ * We failed anyway.
+ */
+ retval = -EBUSY;
+ }
+
/* Added to return the correct error -Ram Gupta */
goto exit;
}
@@ -1900,6 +1907,9 @@ static void uart_change_pm(struct uart_s
{
struct uart_port *port = state->uart_port;
+ if (port->type == PORT_UNKNOWN)
+ return;
+
if (state->pm_state != pm_state) {
if (port->ops->pm)
port->ops->pm(port, pm_state, state->pm_state);
On Wed, 9 Apr 2014 13:22:14 +0200
Thomas Pfaff <[email protected]> wrote:
> From: "Thomas Pfaff" <[email protected]>
>
> While porting a RS485 driver from 2.6.29 to 3.14, i noticed that the serial tty
> driver could break it by using uart ports that it does not own :
Yes thats one spectacularly ugly driver that doesn't want integrating
into 8250.c - although you do need to pull some things the other way for
modern ports as it'll blow up spectacularly if 8250.c has set it up for
DMA etc!
You ought however to be able to re-implement it as a line discipline at
least for fixed ports (it's never going to work on USB ones).
The low level driver calls the ldisc->dcd_change method on a DCD, and you
can issue set_termios requests in order to change the other pins. Adding
a method for other bits used that way is trivial and better than the
existing staging horror.
> 1. uart_change_pm ist called during uart_open and calls the uart pm function
> without checking for PORT_UNKNOWN.
Removing this breaks other parts of the code assume that the port will be
powered up (notably setserial paths). So it makes sense that
uart_change_pm for a "none" port is a no-op but needs logic in the
setserial path to power up a port when we move none->known and power it
down on known->none
> 2. uart_shutdown is called from uart_set_info and does not check it either.
I don't see why this one matters. We would have done
uart_startup
uart_port_startup
uport->type == PORT_UNKNOWN
return 1;
ASYNCB_INITIALIZED is not set
uart_shutdown
ASYNCB_INITIALISED is not set
Skip call to uart_port_shutdown
So that code looks correct to me.
> 3. The return code from the uart request_port call in uart_set_info is not
> handled properly, leading to the situation that the serial driver also
> thinks it owns the uart ports.
> This can triggered by doing following actions :
>
> setserial /dev/ttyS0 uart none # release the uart ports
> modprobe lirc-serial # or any other device that uses the uart
This bit looks correct.
On Wed, 9 Apr 2014, One Thousand Gnomes wrote:
> On Wed, 9 Apr 2014 13:22:14 +0200
> Thomas Pfaff <[email protected]> wrote:
>
> > 1. uart_change_pm ist called during uart_open and calls the uart pm function
> > without checking for PORT_UNKNOWN.
>
> Removing this breaks other parts of the code assume that the port will be
> powered up (notably setserial paths). So it makes sense that
> uart_change_pm for a "none" port is a no-op but needs logic in the
> setserial path to power up a port when we move none->known and power it
> down on known->none
>
Then why not move uart_change_pm into uart_port_startup, where it will be called
when needed ?
A reworked patch is below.
> > 2. uart_shutdown is called from uart_set_info and does not check it either.
>
> I don't see why this one matters. We would have done
>
> uart_startup
> uart_port_startup
> uport->type == PORT_UNKNOWN
> return 1;
> ASYNCB_INITIALIZED is not set
>
> uart_shutdown
> ASYNCB_INITIALISED is not set
> Skip call to uart_port_shutdown
>
> So that code looks correct to me.
I agree, i have overlooked this.
Signed-off-by: Thomas Pfaff <[email protected]>
---
diff -urp linux-3.14.org/drivers/tty/serial/serial_core.c linux-3.14/drivers/tty/serial/serial_core.c
--- linux-3.14.org/drivers/tty/serial/serial_core.c 2014-03-31 05:40:15.000000000 +0200
+++ linux-3.14/drivers/tty/serial/serial_core.c 2014-04-09 16:41:03.940046814 +0200
@@ -138,6 +138,11 @@ static int uart_port_startup(struct tty_
return 1;
/*
+ * Make sure the device is in D0 state.
+ */
+ uart_change_pm(state, UART_PM_STATE_ON);
+
+ /*
* Initialise and allocate the transmit and temporary
* buffer.
*/
@@ -826,25 +831,29 @@ static int uart_set_info(struct tty_stru
* If we fail to request resources for the
* new port, try to restore the old settings.
*/
- if (retval && old_type != PORT_UNKNOWN) {
+ if (retval) {
uport->iobase = old_iobase;
uport->type = old_type;
uport->hub6 = old_hub6;
uport->iotype = old_iotype;
uport->regshift = old_shift;
uport->mapbase = old_mapbase;
- retval = uport->ops->request_port(uport);
- /*
- * If we failed to restore the old settings,
- * we fail like this.
- */
- if (retval)
- uport->type = PORT_UNKNOWN;
- /*
- * We failed anyway.
- */
- retval = -EBUSY;
+ if (old_type != PORT_UNKNOWN) {
+ retval = uport->ops->request_port(uport);
+ /*
+ * If we failed to restore the old settings,
+ * we fail like this.
+ */
+ if (retval)
+ uport->type = PORT_UNKNOWN;
+
+ /*
+ * We failed anyway.
+ */
+ retval = -EBUSY;
+ }
+
/* Added to return the correct error -Ram Gupta */
goto exit;
}
@@ -1570,12 +1579,6 @@ static int uart_open(struct tty_struct *
}
/*
- * Make sure the device is in D0 state.
- */
- if (port->count == 1)
- uart_change_pm(state, UART_PM_STATE_ON);
-
- /*
* Start up the serial port.
*/
retval = uart_startup(tty, state, 0);
On Wed, Apr 09, 2014 at 05:06:24PM +0200, Thomas Pfaff wrote:
>
> On Wed, 9 Apr 2014, One Thousand Gnomes wrote:
>
> > On Wed, 9 Apr 2014 13:22:14 +0200
> > Thomas Pfaff <[email protected]> wrote:
> >
> > > 1. uart_change_pm ist called during uart_open and calls the uart pm function
> > > without checking for PORT_UNKNOWN.
> >
> > Removing this breaks other parts of the code assume that the port will be
> > powered up (notably setserial paths). So it makes sense that
> > uart_change_pm for a "none" port is a no-op but needs logic in the
> > setserial path to power up a port when we move none->known and power it
> > down on known->none
> >
>
> Then why not move uart_change_pm into uart_port_startup, where it will be called
> when needed ?
> A reworked patch is below.
>
> > > 2. uart_shutdown is called from uart_set_info and does not check it either.
> >
> > I don't see why this one matters. We would have done
> >
> > uart_startup
> > uart_port_startup
> > uport->type == PORT_UNKNOWN
> > return 1;
> > ASYNCB_INITIALIZED is not set
> >
> > uart_shutdown
> > ASYNCB_INITIALISED is not set
> > Skip call to uart_port_shutdown
> >
> > So that code looks correct to me.
>
> I agree, i have overlooked this.
>
> Signed-off-by: Thomas Pfaff <[email protected]>
Can you resend this in a format that I can apply this in?
thanks,
greg k-h