serial: 8250_omap: Fix gpio check for auto RTS and CTS
diff mbox series

Message ID 20191006023254.23841-1-aford173@gmail.com
State New
Headers show
Series
  • serial: 8250_omap: Fix gpio check for auto RTS and CTS
Related show

Commit Message

Adam Ford Oct. 6, 2019, 2:32 a.m. UTC
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>

Comments

Yegor Yefremov Oct. 6, 2019, 11:21 a.m. UTC | #1
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
Adam Ford Oct. 6, 2019, 11:38 a.m. UTC | #2
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
Yegor Yefremov Oct. 6, 2019, 12:34 p.m. UTC | #3
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);
Adam Ford Oct. 6, 2019, 3:49 p.m. UTC | #4
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
Yegor Yefremov Oct. 6, 2019, 4:03 p.m. UTC | #5
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

Patch
diff mbox series

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;