[PATCH] tty: serial: msm_serial: Fix XON/XOFF

[PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Jorge Ramirez-Ortiz]

When the tty layer requests the uart to throttle, the current code
executing in msm_serial will trigger "Bad mode in Error Handler" and
generate an invalid stack frame in pstore before rebooting (that is if
pstore is indeed configured: otherwise the user shall just notice a
reboot with no further information dumped to the console).

This patch replaces the PIO byte accessor with the word accessor
already used in PIO mode.

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/tty/serial/msm_serial.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
index 109096033bb1..23833ad952ba 100644
--- a/drivers/tty/serial/msm_serial.c
+++ b/drivers/tty/serial/msm_serial.c
@@ -860,6 +860,7 @@ static void msm_handle_tx(struct uart_port *port)
 	struct circ_buf *xmit = &msm_port->uart.state->xmit;
 	struct msm_dma *dma = &msm_port->tx_dma;
 	unsigned int pio_count, dma_count, dma_min;
+	char buf[4] = { 0 };
 	void __iomem *tf;
 	int err = 0;
 
@@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
 		else
 			tf = port->membase + UART_TF;
 
+		buf[0] = port->x_char;
+
 		if (msm_port->is_uartdm)
 			msm_reset_dm_count(port, 1);
 
-		iowrite8_rep(tf, &port->x_char, 1);
+		iowrite32_rep(tf, buf, 1);
 		port->icount.tx++;
 		port->x_char = 0;
 		return;
-- 
2.21.0

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Stephen Boyd]

Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> When the tty layer requests the uart to throttle, the current code
> executing in msm_serial will trigger "Bad mode in Error Handler" and
> generate an invalid stack frame in pstore before rebooting (that is if
> pstore is indeed configured: otherwise the user shall just notice a
> reboot with no further information dumped to the console).
> 
> This patch replaces the PIO byte accessor with the word accessor
> already used in PIO mode.

Because the hardware only accepts word based accessors and fails
otherwise? I can believe that.

I wonder if the earlier UART hardware this driver used to support (i.e.
pre-DM) would accept byte access to the registers. It's possible, but we
don't really care because those boards aren't supported.

> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

>  drivers/tty/serial/msm_serial.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> index 109096033bb1..23833ad952ba 100644
> --- a/drivers/tty/serial/msm_serial.c
> +++ b/drivers/tty/serial/msm_serial.c
> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>                 else
>                         tf = port->membase + UART_TF;
>  
> +               buf[0] = port->x_char;
> +
>                 if (msm_port->is_uartdm)
>                         msm_reset_dm_count(port, 1);
>  
> -               iowrite8_rep(tf, &port->x_char, 1);
> +               iowrite32_rep(tf, buf, 1);

I suppose it's OK to write some extra zeroes here?

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Jorge Ramirez]

On 5/20/19 16:51, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>> When the tty layer requests the uart to throttle, the current code
>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>> generate an invalid stack frame in pstore before rebooting (that is if
>> pstore is indeed configured: otherwise the user shall just notice a
>> reboot with no further information dumped to the console).
>>
>> This patch replaces the PIO byte accessor with the word accessor
>> already used in PIO mode.
> 
> Because the hardware only accepts word based accessors and fails
> otherwise? I can believe that.
> 
> I wonder if the earlier UART hardware this driver used to support (i.e.
> pre-DM) would accept byte access to the registers. It's possible, but we
> don't really care because those boards aren't supported.

ok.

also the PIO path uses iowrite32_rep to write a number of bytes (from 1
to 4) so I think it is also appropriate to use it for XON/XOFF.

> 
>>
>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>> ---
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
>>  drivers/tty/serial/msm_serial.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>> index 109096033bb1..23833ad952ba 100644
>> --- a/drivers/tty/serial/msm_serial.c
>> +++ b/drivers/tty/serial/msm_serial.c
>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>>                 else
>>                         tf = port->membase + UART_TF;
>>  
>> +               buf[0] = port->x_char;
>> +
>>                 if (msm_port->is_uartdm)
>>                         msm_reset_dm_count(port, 1);
>>  
>> -               iowrite8_rep(tf, &port->x_char, 1);
>> +               iowrite32_rep(tf, buf, 1);
> 
> I suppose it's OK to write some extra zeroes here?
> 
> 

yeah, semantically confusing msm_reset_dm_count is what really matters:
it tells the hardware to only take n bytes (in this case only one) so
the others will be ignored

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Jorge Ramirez]

On 5/20/19 16:56, Jorge Ramirez wrote:
> On 5/20/19 16:51, Stephen Boyd wrote:
>> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>>> When the tty layer requests the uart to throttle, the current code
>>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>>> generate an invalid stack frame in pstore before rebooting (that is if
>>> pstore is indeed configured: otherwise the user shall just notice a
>>> reboot with no further information dumped to the console).
>>>
>>> This patch replaces the PIO byte accessor with the word accessor
>>> already used in PIO mode.
>>
>> Because the hardware only accepts word based accessors and fails
>> otherwise? I can believe that.
>>
>> I wonder if the earlier UART hardware this driver used to support (i.e.
>> pre-DM) would accept byte access to the registers. It's possible, but we
>> don't really care because those boards aren't supported.
> 
> ok.
> 
> also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> to 4) so I think it is also appropriate to use it for XON/XOFF.
> 
>>
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>> ---
>>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>
>>>  drivers/tty/serial/msm_serial.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>> index 109096033bb1..23833ad952ba 100644
>>> --- a/drivers/tty/serial/msm_serial.c
>>> +++ b/drivers/tty/serial/msm_serial.c
>>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>>>                 else
>>>                         tf = port->membase + UART_TF;
>>>  
>>> +               buf[0] = port->x_char;
>>> +
>>>                 if (msm_port->is_uartdm)
>>>                         msm_reset_dm_count(port, 1);
>>>  
>>> -               iowrite8_rep(tf, &port->x_char, 1);
>>> +               iowrite32_rep(tf, buf, 1);
>>
>> I suppose it's OK to write some extra zeroes here?
>>
>>
> 
> yeah, semantically confusing msm_reset_dm_count is what really matters:
> it tells the hardware to only take n bytes (in this case only one) so
> the others will be ignored

um after I said this, maybe iowrite32_rep should only be applied to
uartdm ... what do you think?

> 
> 

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Stephen Boyd]

Quoting Jorge Ramirez (2019-05-20 07:58:54)
> On 5/20/19 16:56, Jorge Ramirez wrote:
> > 
> > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > it tells the hardware to only take n bytes (in this case only one) so
> > the others will be ignored
> 
> um after I said this, maybe iowrite32_rep should only be applied to
> uartdm ... what do you think?
> 

Probably. The uartdm hardware typically required words everywhere while
the pre-dm hardware didn't. It's an if condition so it should be OK.

It may be time to remove non-uartdm support from this driver
all-together. From what I recall the only devices that are upstream are
the uartdm ones, so it may be easier to just remove the legacy stuff
that nobody has tested in many years.

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Jorge Ramirez]

On 5/20/19 17:03, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2019-05-20 07:58:54)
>> On 5/20/19 16:56, Jorge Ramirez wrote:
>>>
>>> yeah, semantically confusing msm_reset_dm_count is what really matters:
>>> it tells the hardware to only take n bytes (in this case only one) so
>>> the others will be ignored
>>
>> um after I said this, maybe iowrite32_rep should only be applied to
>> uartdm ... what do you think?
>>
> 
> Probably. The uartdm hardware typically required words everywhere while
> the pre-dm hardware didn't. It's an if condition so it should be OK.
> 
> It may be time to remove non-uartdm support from this driver
> all-together. From what I recall the only devices that are upstream are
> the uartdm ones, so it may be easier to just remove the legacy stuff
> that nobody has tested in many years.
> 
> 

should this be merged before removing the non-uartdm support or after?

I also have some other changes - in particular with respect to the usage
of fifosize which according to the documentation is in words not in bytes.

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Bjorn Andersson]

On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:

> On 5/20/19 16:56, Jorge Ramirez wrote:
> > On 5/20/19 16:51, Stephen Boyd wrote:
> >> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> >>> When the tty layer requests the uart to throttle, the current code
> >>> executing in msm_serial will trigger "Bad mode in Error Handler" and
> >>> generate an invalid stack frame in pstore before rebooting (that is if
> >>> pstore is indeed configured: otherwise the user shall just notice a
> >>> reboot with no further information dumped to the console).
> >>>
> >>> This patch replaces the PIO byte accessor with the word accessor
> >>> already used in PIO mode.
> >>
> >> Because the hardware only accepts word based accessors and fails
> >> otherwise? I can believe that.
> >>
> >> I wonder if the earlier UART hardware this driver used to support (i.e.
> >> pre-DM) would accept byte access to the registers. It's possible, but we
> >> don't really care because those boards aren't supported.
> > 
> > ok.
> > 
> > also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> > to 4) so I think it is also appropriate to use it for XON/XOFF.
> > 
> >>
> >>>
> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> >>> ---
> >>
> >> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> >>
> >>>  drivers/tty/serial/msm_serial.c | 5 ++++-
> >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> >>> index 109096033bb1..23833ad952ba 100644
> >>> --- a/drivers/tty/serial/msm_serial.c
> >>> +++ b/drivers/tty/serial/msm_serial.c
> >>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> >>>                 else
> >>>                         tf = port->membase + UART_TF;
> >>>  
> >>> +               buf[0] = port->x_char;
> >>> +
> >>>                 if (msm_port->is_uartdm)
> >>>                         msm_reset_dm_count(port, 1);
> >>>  
> >>> -               iowrite8_rep(tf, &port->x_char, 1);
> >>> +               iowrite32_rep(tf, buf, 1);
> >>
> >> I suppose it's OK to write some extra zeroes here?
> >>
> >>
> > 
> > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > it tells the hardware to only take n bytes (in this case only one) so
> > the others will be ignored
> 
> um after I said this, maybe iowrite32_rep should only be applied to
> uartdm ... what do you think?
> 

If I read the history correctly this write was a writel() up until
68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").

So I think you should just change this back to a iowrite32_rep() and add
a Fixes tag.

Regards,
Bjorn

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Bjorn Andersson]

On Mon 20 May 08:11 PDT 2019, Bjorn Andersson wrote:

> On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:
> 
> > On 5/20/19 16:56, Jorge Ramirez wrote:
> > > On 5/20/19 16:51, Stephen Boyd wrote:
> > >> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
> > >>> When the tty layer requests the uart to throttle, the current code
> > >>> executing in msm_serial will trigger "Bad mode in Error Handler" and
> > >>> generate an invalid stack frame in pstore before rebooting (that is if
> > >>> pstore is indeed configured: otherwise the user shall just notice a
> > >>> reboot with no further information dumped to the console).
> > >>>
> > >>> This patch replaces the PIO byte accessor with the word accessor
> > >>> already used in PIO mode.
> > >>
> > >> Because the hardware only accepts word based accessors and fails
> > >> otherwise? I can believe that.
> > >>
> > >> I wonder if the earlier UART hardware this driver used to support (i.e.
> > >> pre-DM) would accept byte access to the registers. It's possible, but we
> > >> don't really care because those boards aren't supported.
> > > 
> > > ok.
> > > 
> > > also the PIO path uses iowrite32_rep to write a number of bytes (from 1
> > > to 4) so I think it is also appropriate to use it for XON/XOFF.
> > > 
> > >>
> > >>>
> > >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> > >>> ---
> > >>
> > >> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > >>
> > >>>  drivers/tty/serial/msm_serial.c | 5 ++++-
> > >>>  1 file changed, 4 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
> > >>> index 109096033bb1..23833ad952ba 100644
> > >>> --- a/drivers/tty/serial/msm_serial.c
> > >>> +++ b/drivers/tty/serial/msm_serial.c
> > >>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
> > >>>                 else
> > >>>                         tf = port->membase + UART_TF;
> > >>>  
> > >>> +               buf[0] = port->x_char;
> > >>> +
> > >>>                 if (msm_port->is_uartdm)
> > >>>                         msm_reset_dm_count(port, 1);
> > >>>  
> > >>> -               iowrite8_rep(tf, &port->x_char, 1);
> > >>> +               iowrite32_rep(tf, buf, 1);
> > >>
> > >> I suppose it's OK to write some extra zeroes here?
> > >>
> > >>
> > > 
> > > yeah, semantically confusing msm_reset_dm_count is what really matters:
> > > it tells the hardware to only take n bytes (in this case only one) so
> > > the others will be ignored
> > 
> > um after I said this, maybe iowrite32_rep should only be applied to
> > uartdm ... what do you think?
> > 
> 
> If I read the history correctly this write was a writel() up until
> 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").
> 
> So I think you should just change this back to a iowrite32_rep() and add
> a Fixes tag.
> 

I mean...

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

Re: [PATCH] tty: serial: msm_serial: Fix XON/XOFF

[Jorge Ramirez]

On 5/20/19 17:12, Bjorn Andersson wrote:
> On Mon 20 May 08:11 PDT 2019, Bjorn Andersson wrote:
> 
>> On Mon 20 May 07:58 PDT 2019, Jorge Ramirez wrote:
>>
>>> On 5/20/19 16:56, Jorge Ramirez wrote:
>>>> On 5/20/19 16:51, Stephen Boyd wrote:
>>>>> Quoting Jorge Ramirez-Ortiz (2019-05-20 03:34:35)
>>>>>> When the tty layer requests the uart to throttle, the current code
>>>>>> executing in msm_serial will trigger "Bad mode in Error Handler" and
>>>>>> generate an invalid stack frame in pstore before rebooting (that is if
>>>>>> pstore is indeed configured: otherwise the user shall just notice a
>>>>>> reboot with no further information dumped to the console).
>>>>>>
>>>>>> This patch replaces the PIO byte accessor with the word accessor
>>>>>> already used in PIO mode.
>>>>>
>>>>> Because the hardware only accepts word based accessors and fails
>>>>> otherwise? I can believe that.
>>>>>
>>>>> I wonder if the earlier UART hardware this driver used to support (i.e.
>>>>> pre-DM) would accept byte access to the registers. It's possible, but we
>>>>> don't really care because those boards aren't supported.
>>>>
>>>> ok.
>>>>
>>>> also the PIO path uses iowrite32_rep to write a number of bytes (from 1
>>>> to 4) so I think it is also appropriate to use it for XON/XOFF.
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>>
>>>>>>  drivers/tty/serial/msm_serial.c | 5 ++++-
>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c
>>>>>> index 109096033bb1..23833ad952ba 100644
>>>>>> --- a/drivers/tty/serial/msm_serial.c
>>>>>> +++ b/drivers/tty/serial/msm_serial.c
>>>>>> @@ -869,10 +870,12 @@ static void msm_handle_tx(struct uart_port *port)
>>>>>>                 else
>>>>>>                         tf = port->membase + UART_TF;
>>>>>>  
>>>>>> +               buf[0] = port->x_char;
>>>>>> +
>>>>>>                 if (msm_port->is_uartdm)
>>>>>>                         msm_reset_dm_count(port, 1);
>>>>>>  
>>>>>> -               iowrite8_rep(tf, &port->x_char, 1);
>>>>>> +               iowrite32_rep(tf, buf, 1);
>>>>>
>>>>> I suppose it's OK to write some extra zeroes here?
>>>>>
>>>>>
>>>>
>>>> yeah, semantically confusing msm_reset_dm_count is what really matters:
>>>> it tells the hardware to only take n bytes (in this case only one) so
>>>> the others will be ignored
>>>
>>> um after I said this, maybe iowrite32_rep should only be applied to
>>> uartdm ... what do you think?
>>>
>>
>> If I read the history correctly this write was a writel() up until
>> 68252424a7c7 ("tty: serial: msm: Support big-endian CPUs").
>>
>> So I think you should just change this back to a iowrite32_rep() and add
>> a Fixes tag.
>>
> 
> I mean...

ok. cool

> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Regards,
> Bjorn
>