| Message ID | 20191006023254.23841-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: 8250_omap: Fix gpio check for auto RTS and CTS | expand |
Hi Adam, On Sun, Oct 6, 2019 at 4:33 AM Adam Ford <aford173@gmail.com> wrote: > > There are two checks to see if the manual gpio is configured, but > these the check is seeing if the structure is NULL instead it > should check to see if there are CTS and/or RTS pins defined. > > This patch uses checks for those individual pins instead of > checking for the structure itself. > > Signed-off-by: Adam Ford <aford173@gmail.com> > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > index c68e2b3a1634..836e736ae188 100644 > --- a/drivers/tty/serial/8250/8250_omap.c > +++ b/drivers/tty/serial/8250/8250_omap.c > @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) > > serial8250_do_set_mctrl(port, mctrl); > > - if (!up->gpios) { > + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { > /* > * Turn off autoRTS if RTS is lowered and restore autoRTS > * setting if RTS is raised > @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, > up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); > > if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && > - !up->gpios) { > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { > /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ > up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; > priv->efr |= UART_EFR_CTS; Looks good to me but !up->gpios must remain as otherwise, we will get NULL pointer dereference. What do you think? Also adding some more people who can be interested in testing this approach. Cheers, Yegor
On Sun, Oct 6, 2019 at 6:21 AM Yegor Yefremov <yegorslists@googlemail.com> wrote: > > Hi Adam, > > On Sun, Oct 6, 2019 at 4:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > There are two checks to see if the manual gpio is configured, but > > these the check is seeing if the structure is NULL instead it > > should check to see if there are CTS and/or RTS pins defined. > > > > This patch uses checks for those individual pins instead of > > checking for the structure itself. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > > index c68e2b3a1634..836e736ae188 100644 > > --- a/drivers/tty/serial/8250/8250_omap.c > > +++ b/drivers/tty/serial/8250/8250_omap.c > > @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > > serial8250_do_set_mctrl(port, mctrl); > > > > - if (!up->gpios) { > > + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { > > /* > > * Turn off autoRTS if RTS is lowered and restore autoRTS > > * setting if RTS is raised > > @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, > > up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); > > > > if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && > > - !up->gpios) { > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { > > /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ > > up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; > > priv->efr |= UART_EFR_CTS; > > Looks good to me but !up->gpios must remain as otherwise, we will get > NULL pointer dereference. What do you think? I was not seeing up->gpios ever NULL so the contents inside the check never was executed. When I removed the check, the performance came back. I looked at examples on how other devices checked for RTS and CTS, and I noticed that the Atmel serial driver did something like the above. > > Also adding some more people who can be interested in testing this approach. I am open for ideas. If something is better, but something is either incorrectly setting up->gpios to non-NULL or the check for non-NULL is wrong. adam > > Cheers, > Yegor
On Sun, Oct 6, 2019 at 1:38 PM Adam Ford <aford173@gmail.com> wrote: > > On Sun, Oct 6, 2019 at 6:21 AM Yegor Yefremov > <yegorslists@googlemail.com> wrote: > > > > Hi Adam, > > > > On Sun, Oct 6, 2019 at 4:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > There are two checks to see if the manual gpio is configured, but > > > these the check is seeing if the structure is NULL instead it > > > should check to see if there are CTS and/or RTS pins defined. > > > > > > This patch uses checks for those individual pins instead of > > > checking for the structure itself. > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index e682390..d5fdb71 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct uart_8250_port *up) } else { uart->gpios = gpios; } + } else { + uart->gpios = NULL; } serial8250_set_defaults(uart); > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > > > index c68e2b3a1634..836e736ae188 100644 > > > --- a/drivers/tty/serial/8250/8250_omap.c > > > +++ b/drivers/tty/serial/8250/8250_omap.c > > > @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > > > > serial8250_do_set_mctrl(port, mctrl); > > > > > > - if (!up->gpios) { > > > + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { > > > /* > > > * Turn off autoRTS if RTS is lowered and restore autoRTS > > > * setting if RTS is raised > > > @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, > > > up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); > > > > > > if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && > > > - !up->gpios) { > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { > > > /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ > > > up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; > > > priv->efr |= UART_EFR_CTS; > > > > Looks good to me but !up->gpios must remain as otherwise, we will get > > NULL pointer dereference. What do you think? > > I was not seeing up->gpios ever NULL so the contents inside the check > never was executed. When I removed the check, the performance came > back. I looked at examples on how other devices checked for RTS and > CTS, and I noticed that the Atmel serial driver did something like the > above. > > > > > Also adding some more people who can be interested in testing this approach. > > I am open for ideas. If something is better, but something is either > incorrectly setting up->gpios to non-NULL or the check for non-NULL is > wrong. I wonder whether we forgot to add this assignment for the ACPI systems: diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index e682390..d5fdb71 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct uart_8250_port *up) } else { uart->gpios = gpios; } + } else { + uart->gpios = NULL; } serial8250_set_defaults(uart);
On Sun, Oct 6, 2019 at 7:34 AM Yegor Yefremov <yegorslists@googlemail.com> wrote: > > On Sun, Oct 6, 2019 at 1:38 PM Adam Ford <aford173@gmail.com> wrote: > > > > On Sun, Oct 6, 2019 at 6:21 AM Yegor Yefremov > > <yegorslists@googlemail.com> wrote: > > > > > > Hi Adam, > > > > > > On Sun, Oct 6, 2019 at 4:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > There are two checks to see if the manual gpio is configured, but > > > > these the check is seeing if the structure is NULL instead it > > > > should check to see if there are CTS and/or RTS pins defined. > > > > > > > > This patch uses checks for those individual pins instead of > > > > checking for the structure itself. > > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c > index e682390..d5fdb71 100644 > --- a/drivers/tty/serial/8250/8250_core.c > +++ b/drivers/tty/serial/8250/8250_core.c > @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct > uart_8250_port *up) > } else { > uart->gpios = gpios; > } > + } else { > + uart->gpios = NULL; > } > > serial8250_set_defaults(uart); > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > > > > index c68e2b3a1634..836e736ae188 100644 > > > > --- a/drivers/tty/serial/8250/8250_omap.c > > > > +++ b/drivers/tty/serial/8250/8250_omap.c > > > > @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > > > > > > serial8250_do_set_mctrl(port, mctrl); > > > > > > > > - if (!up->gpios) { > > > > + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { > > > > /* > > > > * Turn off autoRTS if RTS is lowered and restore autoRTS > > > > * setting if RTS is raised > > > > @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, > > > > up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); > > > > > > > > if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && > > > > - !up->gpios) { > > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && > > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { > > > > /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ > > > > up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; > > > > priv->efr |= UART_EFR_CTS; > > > > > > Looks good to me but !up->gpios must remain as otherwise, we will get > > > NULL pointer dereference. What do you think? What we if add a check to make sure we deference it? I was thinking something like: diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c index d9074303c88e..fb4781292d40 100644 --- a/drivers/tty/serial/serial_mctrl_gpio.c +++ b/drivers/tty/serial/serial_mctrl_gpio.c @@ -66,6 +66,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set); struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, enum mctrl_gpio_idx gidx) { + if (gpios == NULL) + return NULL; + return gpios->gpio[gidx]; } EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod); This should make my previous patch tolerate situations where gpios is NULL. It also explictly looks for conditions where the gpios are manually configured for RTS and/or CTS. > > > > I was not seeing up->gpios ever NULL so the contents inside the check > > never was executed. When I removed the check, the performance came > > back. I looked at examples on how other devices checked for RTS and > > CTS, and I noticed that the Atmel serial driver did something like the > > above. > > > > > > > > Also adding some more people who can be interested in testing this approach. > > > > I am open for ideas. If something is better, but something is either > > incorrectly setting up->gpios to non-NULL or the check for non-NULL is > > wrong. > > I wonder whether we forgot to add this assignment for the ACPI systems: > I unwound my patch and applied your patch instead, but it is back to having corrupt frames again. > diff --git a/drivers/tty/serial/8250/8250_core.c > b/drivers/tty/serial/8250/8250_core.c > index e682390..d5fdb71 100644 > --- a/drivers/tty/serial/8250/8250_core.c > +++ b/drivers/tty/serial/8250/8250_core.c > @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct > uart_8250_port *up) > } else { > uart->gpios = gpios; > } > + } else { > + uart->gpios = NULL; > } > I did not check the value of uart->gpios after this patch, but since the flow control code isn't executing, I am guessing it's still not NULL. > serial8250_set_defaults(uart); adam
On Sun, Oct 6, 2019 at 5:49 PM Adam Ford <aford173@gmail.com> wrote: > > On Sun, Oct 6, 2019 at 7:34 AM Yegor Yefremov > <yegorslists@googlemail.com> wrote: > > > > On Sun, Oct 6, 2019 at 1:38 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > On Sun, Oct 6, 2019 at 6:21 AM Yegor Yefremov > > > <yegorslists@googlemail.com> wrote: > > > > > > > > Hi Adam, > > > > > > > > On Sun, Oct 6, 2019 at 4:33 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > There are two checks to see if the manual gpio is configured, but > > > > > these the check is seeing if the structure is NULL instead it > > > > > should check to see if there are CTS and/or RTS pins defined. > > > > > > > > > > This patch uses checks for those individual pins instead of > > > > > checking for the structure itself. > > > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c > > index e682390..d5fdb71 100644 > > --- a/drivers/tty/serial/8250/8250_core.c > > +++ b/drivers/tty/serial/8250/8250_core.c > > @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct > > uart_8250_port *up) > > } else { > > uart->gpios = gpios; > > } > > + } else { > > + uart->gpios = NULL; > > } > > > > serial8250_set_defaults(uart); > > > > > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > > > > > diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c > > > > > index c68e2b3a1634..836e736ae188 100644 > > > > > --- a/drivers/tty/serial/8250/8250_omap.c > > > > > +++ b/drivers/tty/serial/8250/8250_omap.c > > > > > @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > > > > > > > > serial8250_do_set_mctrl(port, mctrl); > > > > > > > > > > - if (!up->gpios) { > > > > > + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { > > > > > /* > > > > > * Turn off autoRTS if RTS is lowered and restore autoRTS > > > > > * setting if RTS is raised > > > > > @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, > > > > > up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); > > > > > > > > > > if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && > > > > > - !up->gpios) { > > > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && > > > > > + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { > > > > > /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ > > > > > up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; > > > > > priv->efr |= UART_EFR_CTS; > > > > > > > > Looks good to me but !up->gpios must remain as otherwise, we will get > > > > NULL pointer dereference. What do you think? > > What we if add a check to make sure we deference it? I was thinking > something like: > > diff --git a/drivers/tty/serial/serial_mctrl_gpio.c > b/drivers/tty/serial/serial_mctrl_gpio.c > index d9074303c88e..fb4781292d40 100644 > --- a/drivers/tty/serial/serial_mctrl_gpio.c > +++ b/drivers/tty/serial/serial_mctrl_gpio.c > @@ -66,6 +66,9 @@ EXPORT_SYMBOL_GPL(mctrl_gpio_set); > struct gpio_desc *mctrl_gpio_to_gpiod(struct mctrl_gpios *gpios, > enum mctrl_gpio_idx gidx) > { > + if (gpios == NULL) > + return NULL; > + > return gpios->gpio[gidx]; > } > EXPORT_SYMBOL_GPL(mctrl_gpio_to_gpiod); > > This should make my previous patch tolerate situations where gpios is > NULL. It also explictly looks for conditions where the gpios are > manually configured for RTS and/or CTS. I like this idea. This would solve my concern. Yegor > > > > > > I was not seeing up->gpios ever NULL so the contents inside the check > > > never was executed. When I removed the check, the performance came > > > back. I looked at examples on how other devices checked for RTS and > > > CTS, and I noticed that the Atmel serial driver did something like the > > > above. > > > > > > > > > > > Also adding some more people who can be interested in testing this approach. > > > > > > I am open for ideas. If something is better, but something is either > > > incorrectly setting up->gpios to non-NULL or the check for non-NULL is > > > wrong. > > > > I wonder whether we forgot to add this assignment for the ACPI systems: > > > > I unwound my patch and applied your patch instead, but it is back to > having corrupt frames again. > > > diff --git a/drivers/tty/serial/8250/8250_core.c > > b/drivers/tty/serial/8250/8250_core.c > > index e682390..d5fdb71 100644 > > --- a/drivers/tty/serial/8250/8250_core.c > > +++ b/drivers/tty/serial/8250/8250_core.c > > @@ -1031,6 +1031,8 @@ int serial8250_register_8250_port(struct > > uart_8250_port *up) > > } else { > > uart->gpios = gpios; > > } > > + } else { > > + uart->gpios = NULL; > > } > > > I did not check the value of uart->gpios after this patch, but since > the flow control code isn't executing, I am guessing it's still not > NULL. > > serial8250_set_defaults(uart); > > adam
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index c68e2b3a1634..836e736ae188 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -141,7 +141,7 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl) serial8250_do_set_mctrl(port, mctrl); - if (!up->gpios) { + if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) { /* * Turn off autoRTS if RTS is lowered and restore autoRTS * setting if RTS is raised @@ -456,7 +456,8 @@ static void omap_8250_set_termios(struct uart_port *port, up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF); if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW && - !up->gpios) { + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS) && + !mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_CTS)) { /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */ up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS; priv->efr |= UART_EFR_CTS;
There are two checks to see if the manual gpio is configured, but these the check is seeing if the structure is NULL instead it should check to see if there are CTS and/or RTS pins defined. This patch uses checks for those individual pins instead of checking for the structure itself. Signed-off-by: Adam Ford <aford173@gmail.com>