* [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
@ 2019-10-06 2:32 Adam Ford
2019-10-06 11:21 ` Yegor Yefremov
0 siblings, 1 reply; 6+ messages in thread
From: Adam Ford @ 2019-10-06 2:32 UTC (permalink / raw)
To: linux-serial
Cc: adam.ford, linux-omap, Adam Ford, Greg Kroah-Hartman, Jiri Slaby,
Vignesh R, Andy Shevchenko, Yegor Yefremov, Tony Lindgren,
Douglas Anderson, linux-kernel
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
2019-10-06 2:32 [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS Adam Ford
@ 2019-10-06 11:21 ` Yegor Yefremov
2019-10-06 11:38 ` Adam Ford
0 siblings, 1 reply; 6+ messages in thread
From: Yegor Yefremov @ 2019-10-06 11:21 UTC (permalink / raw)
To: Adam Ford
Cc: linux-serial, adam.ford, Linux-OMAP, Greg Kroah-Hartman,
Jiri Slaby, Vignesh R, Andy Shevchenko, Tony Lindgren,
Douglas Anderson, kernel list, Nuno Gonçalves,
Giulio Benetti, Stefan Roese
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
2019-10-06 11:21 ` Yegor Yefremov
@ 2019-10-06 11:38 ` Adam Ford
2019-10-06 12:34 ` Yegor Yefremov
0 siblings, 1 reply; 6+ messages in thread
From: Adam Ford @ 2019-10-06 11:38 UTC (permalink / raw)
To: Yegor Yefremov
Cc: linux-serial, Adam Ford, Linux-OMAP, Greg Kroah-Hartman,
Jiri Slaby, Vignesh R, Andy Shevchenko, Tony Lindgren,
Douglas Anderson, kernel list, Nuno Gonçalves,
Giulio Benetti, Stefan Roese
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
2019-10-06 11:38 ` Adam Ford
@ 2019-10-06 12:34 ` Yegor Yefremov
2019-10-06 15:49 ` Adam Ford
0 siblings, 1 reply; 6+ messages in thread
From: Yegor Yefremov @ 2019-10-06 12:34 UTC (permalink / raw)
To: Adam Ford
Cc: linux-serial, Adam Ford, Linux-OMAP, Greg Kroah-Hartman,
Jiri Slaby, Vignesh R, Andy Shevchenko, Tony Lindgren,
Douglas Anderson, kernel list, Nuno Gonçalves,
Giulio Benetti, Stefan Roese
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
2019-10-06 12:34 ` Yegor Yefremov
@ 2019-10-06 15:49 ` Adam Ford
2019-10-06 16:03 ` Yegor Yefremov
0 siblings, 1 reply; 6+ messages in thread
From: Adam Ford @ 2019-10-06 15:49 UTC (permalink / raw)
To: Yegor Yefremov
Cc: linux-serial, Adam Ford, Linux-OMAP, Greg Kroah-Hartman,
Jiri Slaby, Vignesh R, Andy Shevchenko, Tony Lindgren,
Douglas Anderson, kernel list, Nuno Gonçalves,
Giulio Benetti, Stefan Roese
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS
2019-10-06 15:49 ` Adam Ford
@ 2019-10-06 16:03 ` Yegor Yefremov
0 siblings, 0 replies; 6+ messages in thread
From: Yegor Yefremov @ 2019-10-06 16:03 UTC (permalink / raw)
To: Adam Ford
Cc: linux-serial, Adam Ford, Linux-OMAP, Greg Kroah-Hartman,
Jiri Slaby, Vignesh R, Andy Shevchenko, Tony Lindgren,
Douglas Anderson, kernel list, Nuno Gonçalves,
Giulio Benetti, Stefan Roese
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-06 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-06 2:32 [PATCH] serial: 8250_omap: Fix gpio check for auto RTS and CTS Adam Ford
2019-10-06 11:21 ` Yegor Yefremov
2019-10-06 11:38 ` Adam Ford
2019-10-06 12:34 ` Yegor Yefremov
2019-10-06 15:49 ` Adam Ford
2019-10-06 16:03 ` Yegor Yefremov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).