Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1222205 > unrolled thread

[PATCH v2 0/4] serial: samsung: Fix UART status handling and other fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] serial: samsung: Fix UART status handling and other  fixes Robert Baldyga <r.baldyga@samsung.com> - 2015-09-10 15:50 +0200
    [PATCH v2 1/4] serial: samsung: remove unused 'irq' parameter Robert Baldyga <r.baldyga@samsung.com> - 2015-09-10 15:50 +0200
    [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode Robert Baldyga <r.baldyga@samsung.com> - 2015-09-10 15:50 +0200
      Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA  mode Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-11 08:10 +0200
        Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA  mode Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2015-09-11 08:40 +0200
          Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA  mode Robert Baldyga <r.baldyga@samsung.com> - 2015-09-11 08:50 +0200
        Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA  mode Robert Baldyga <r.baldyga@samsung.com> - 2015-09-11 08:40 +0200
    [PATCH v2 3/4] serial: samsung: introduce  s3c24xx_serial_rx_drain_fifo() function Robert Baldyga <r.baldyga@samsung.com> - 2015-09-10 15:50 +0200
      Re: [PATCH v2 3/4] serial: samsung: introduce  s3c24xx_serial_rx_drain_fifo() function Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 14:50 +0200

#1222205 — [PATCH v2 0/4] serial: samsung: Fix UART status handling and other fixes

Hello,

This patch set contains four patches: two minor fixes and two patches
fixing quite importatn bug which was missing UART status handling in
DMA mode. It fixes, among others, 'break' contition handling, which is
necessary if we want to use Magic SysRq. So this patch fixes Magic SysRq
handling for serial consoles using UART in DMA mode,

Best regards,
Robert Baldyga

Changelog:

v2:
- address comments from Krzysztof Kozlowski:
  - add comment in patch removing 'ignore_char' label
  - split patch "Fix UART status handling in DMA mode"
    into two patches for better code readability

v1: https://lkml.org/lkml/2015/9/8/190

Robert Baldyga (4):
  serial: samsung: remove unused 'irq' parameter
  serial: samsung: remove unneded 'ignore_char' label
  serial: samsung: introduce s3c24xx_serial_rx_drain_fifo() function
  serial: samsung: Fix UART status handling in DMA mode

 drivers/tty/serial/samsung.c | 66 ++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 45 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1222207 — [PATCH v2 1/4] serial: samsung: remove unused 'irq' parameter

This parameter is not used anywhere, so we can get rid of it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/tty/serial/samsung.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 856686d..fee764c 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -573,7 +573,7 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 	ourport->rx_mode = S3C24XX_RX_PIO;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_dma(int irq, void *dev_id)
+static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 {
 	unsigned int utrstat, ufstat, received;
 	struct s3c24xx_uart_port *ourport = dev_id;
@@ -621,7 +621,7 @@ finish:
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_pio(int irq, void *dev_id)
+static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 {
 	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
@@ -718,8 +718,8 @@ static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
 	struct s3c24xx_uart_port *ourport = dev_id;
 
 	if (ourport->dma && ourport->dma->rx_chan)
-		return s3c24xx_serial_rx_chars_dma(irq, dev_id);
-	return s3c24xx_serial_rx_chars_pio(irq, dev_id);
+		return s3c24xx_serial_rx_chars_dma(dev_id);
+	return s3c24xx_serial_rx_chars_pio(dev_id);
 }
 
 static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222209 — [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode

This patch fixes UART status handling in DMA mode. For this purpose we
use s3c24xx_serial_rx_drain_fifo() instead of uart_rx_drain_fifo(), which
does the same thing plus checks for special conditions (such as 'break').

Thanks to this we have, for example, Magic SysRq handling, which was
missing in DMA mode so far. Since we can use UART in DMA mode as serial
console, this is a quite important improvement.

This change additionally simplifies RX handling code, as we no longer
need uart_rx_drain_fifo() function, so we can remove it.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/tty/serial/samsung.c | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 1d7dd86..d72cd73 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -385,32 +385,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
 	}
 }
 
-static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
-				     unsigned long ufstat);
-
-static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
-{
-	struct uart_port *port = &ourport->port;
-	struct tty_port *tty = &port->state->port;
-	unsigned int ch, ufstat;
-	unsigned int count;
-
-	ufstat = rd_regl(port, S3C2410_UFSTAT);
-	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
-
-	if (!count)
-		return;
-
-	while (count-- > 0) {
-		ch = rd_regb(port, S3C2410_URXH);
-
-		ourport->port.icount.rx++;
-		tty_insert_flip_char(tty, ch, TTY_NORMAL);
-	}
-
-	tty_flip_buffer_push(tty);
-}
-
 static void s3c24xx_serial_stop_rx(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -573,6 +547,8 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 	ourport->rx_mode = S3C24XX_RX_PIO;
 }
 
+static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
+
 static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 {
 	unsigned int utrstat, ufstat, received;
@@ -606,7 +582,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 		enable_rx_pio(ourport);
 	}
 
-	uart_rx_drain_fifo(ourport);
+	s3c24xx_serial_rx_drain_fifo(ourport);
 
 	if (tty) {
 		tty_flip_buffer_push(t);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222547 — Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode

On 10.09.2015 22:41, Robert Baldyga wrote:
> This patch fixes UART status handling in DMA mode.

I don't see any changes here. You did not respond to my comment neither.

Code looks itself good... except a locking issue but I don't know what's
the cause. It may be not related to the patchset and maybe just not all
of issues are fixed yet. Anyway I'll describe it in 3/4.

Best regards,
Krzysztof

> For this purpose we
> use s3c24xx_serial_rx_drain_fifo() instead of uart_rx_drain_fifo(), which
> does the same thing plus checks for special conditions (such as 'break').
> 
> Thanks to this we have, for example, Magic SysRq handling, which was
> missing in DMA mode so far. Since we can use UART in DMA mode as serial
> console, this is a quite important improvement.
> 
> This change additionally simplifies RX handling code, as we no longer
> need uart_rx_drain_fifo() function, so we can remove it.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/tty/serial/samsung.c | 30 +++---------------------------
>  1 file changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 1d7dd86..d72cd73 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -385,32 +385,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>  	}
>  }
>  
> -static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
> -				     unsigned long ufstat);
> -
> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> -{
> -	struct uart_port *port = &ourport->port;
> -	struct tty_port *tty = &port->state->port;
> -	unsigned int ch, ufstat;
> -	unsigned int count;
> -
> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
> -
> -	if (!count)
> -		return;
> -
> -	while (count-- > 0) {
> -		ch = rd_regb(port, S3C2410_URXH);
> -
> -		ourport->port.icount.rx++;
> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
> -	}
> -
> -	tty_flip_buffer_push(tty);
> -}
> -
>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>  {
>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
> @@ -573,6 +547,8 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>  	ourport->rx_mode = S3C24XX_RX_PIO;
>  }
>  
> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
> +
>  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>  {
>  	unsigned int utrstat, ufstat, received;
> @@ -606,7 +582,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>  		enable_rx_pio(ourport);
>  	}
>  
> -	uart_rx_drain_fifo(ourport);
> +	s3c24xx_serial_rx_drain_fifo(ourport);
>  
>  	if (tty) {
>  		tty_flip_buffer_push(t);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222554 — Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode

On 11.09.2015 15:32, Robert Baldyga wrote:
> On 09/11/2015 08:07 AM, Krzysztof Kozlowski wrote:
>> On 10.09.2015 22:41, Robert Baldyga wrote:
>>> This patch fixes UART status handling in DMA mode.
>>
>> I don't see any changes here. You did not respond to my comment neither.
>>
>> Code looks itself good... except a locking issue but I don't know what's
>> the cause. It may be not related to the patchset and maybe just not all
>> of issues are fixed yet. Anyway I'll describe it in 3/4.
>>
>> Best regards,
>> Krzysztof
>>
>>> For this purpose we
>>> use s3c24xx_serial_rx_drain_fifo() instead of uart_rx_drain_fifo(), which
>>> does the same thing plus checks for special conditions (such as 'break').
>>>
>>> Thanks to this we have, for example, Magic SysRq handling, which was
>>> missing in DMA mode so far. Since we can use UART in DMA mode as serial
>>> console, this is a quite important improvement.
>>>
>>> This change additionally simplifies RX handling code, as we no longer
>>> need uart_rx_drain_fifo() function, so we can remove it.
>>>
>>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>> ---
>>>  drivers/tty/serial/samsung.c | 30 +++---------------------------
>>>  1 file changed, 3 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>>> index 1d7dd86..d72cd73 100644
>>> --- a/drivers/tty/serial/samsung.c
>>> +++ b/drivers/tty/serial/samsung.c
>>> @@ -385,32 +385,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>>>  	}
>>>  }
>>>  
>>> -static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
>>> -				     unsigned long ufstat);
>>> -
>>> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>>> -{
>>> -	struct uart_port *port = &ourport->port;
>>> -	struct tty_port *tty = &port->state->port;
>>> -	unsigned int ch, ufstat;
>>> -	unsigned int count;
>>> -
>>> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
>>> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
>>> -
>>> -	if (!count)
>>> -		return;
>>> -
>>> -	while (count-- > 0) {
>>> -		ch = rd_regb(port, S3C2410_URXH);
>>> -
>>> -		ourport->port.icount.rx++;
>>> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
>>> -	}
>>> -
>>> -	tty_flip_buffer_push(tty);
>>> -}
>>> -
>>>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>>>  {
>>>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>>> @@ -573,6 +547,8 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>>>  	ourport->rx_mode = S3C24XX_RX_PIO;
>>>  }
>>>  
>>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
>>> +
>>>  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>>  {
>>>  	unsigned int utrstat, ufstat, received;
>>> @@ -606,7 +582,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>>  		enable_rx_pio(ourport);
>>>  	}
>>>  
> 
> The essence of change is here. We use another method for draining FIFO.
> Instead of just putting them into tty buffer, we additionally check for
> special conditions, and that's the improvement.

Hm? I was referring to my comment - I did not see any changes around
"fixes" in commit message.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222564 — Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode

On 09/11/2015 08:34 AM, Krzysztof Kozlowski wrote:
> On 11.09.2015 15:32, Robert Baldyga wrote:
>> On 09/11/2015 08:07 AM, Krzysztof Kozlowski wrote:
>>> On 10.09.2015 22:41, Robert Baldyga wrote:
>>>> This patch fixes UART status handling in DMA mode.
>>>
>>> I don't see any changes here. You did not respond to my comment neither.
>>>
>>> Code looks itself good... except a locking issue but I don't know what's
>>> the cause. It may be not related to the patchset and maybe just not all
>>> of issues are fixed yet. Anyway I'll describe it in 3/4.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>>> For this purpose we
>>>> use s3c24xx_serial_rx_drain_fifo() instead of uart_rx_drain_fifo(), which
>>>> does the same thing plus checks for special conditions (such as 'break').
>>>>
>>>> Thanks to this we have, for example, Magic SysRq handling, which was
>>>> missing in DMA mode so far. Since we can use UART in DMA mode as serial
>>>> console, this is a quite important improvement.
>>>>
>>>> This change additionally simplifies RX handling code, as we no longer
>>>> need uart_rx_drain_fifo() function, so we can remove it.
>>>>
>>>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>>> ---
>>>>  drivers/tty/serial/samsung.c | 30 +++---------------------------
>>>>  1 file changed, 3 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>>>> index 1d7dd86..d72cd73 100644
>>>> --- a/drivers/tty/serial/samsung.c
>>>> +++ b/drivers/tty/serial/samsung.c
>>>> @@ -385,32 +385,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>>>>  	}
>>>>  }
>>>>  
>>>> -static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
>>>> -				     unsigned long ufstat);
>>>> -
>>>> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>>>> -{
>>>> -	struct uart_port *port = &ourport->port;
>>>> -	struct tty_port *tty = &port->state->port;
>>>> -	unsigned int ch, ufstat;
>>>> -	unsigned int count;
>>>> -
>>>> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
>>>> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
>>>> -
>>>> -	if (!count)
>>>> -		return;
>>>> -
>>>> -	while (count-- > 0) {
>>>> -		ch = rd_regb(port, S3C2410_URXH);
>>>> -
>>>> -		ourport->port.icount.rx++;
>>>> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
>>>> -	}
>>>> -
>>>> -	tty_flip_buffer_push(tty);
>>>> -}
>>>> -
>>>>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>>>>  {
>>>>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>>>> @@ -573,6 +547,8 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>>>>  	ourport->rx_mode = S3C24XX_RX_PIO;
>>>>  }
>>>>  
>>>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
>>>> +
>>>>  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>>>  {
>>>>  	unsigned int utrstat, ufstat, received;
>>>> @@ -606,7 +582,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>>>  		enable_rx_pio(ourport);
>>>>  	}
>>>>  
>>
>> The essence of change is here. We use another method for draining FIFO.
>> Instead of just putting them into tty buffer, we additionally check for
>> special conditions, and that's the improvement.
> 
> Hm? I was referring to my comment - I did not see any changes around
> "fixes" in commit message.

Ohh, I see :p I will describe it better ;)

Thanks,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222559 — Re: [PATCH v2 4/4] serial: samsung: Fix UART status handling in DMA mode

On 09/11/2015 08:07 AM, Krzysztof Kozlowski wrote:
> On 10.09.2015 22:41, Robert Baldyga wrote:
>> This patch fixes UART status handling in DMA mode.
> 
> I don't see any changes here. You did not respond to my comment neither.
> 
> Code looks itself good... except a locking issue but I don't know what's
> the cause. It may be not related to the patchset and maybe just not all
> of issues are fixed yet. Anyway I'll describe it in 3/4.
> 
> Best regards,
> Krzysztof
> 
>> For this purpose we
>> use s3c24xx_serial_rx_drain_fifo() instead of uart_rx_drain_fifo(), which
>> does the same thing plus checks for special conditions (such as 'break').
>>
>> Thanks to this we have, for example, Magic SysRq handling, which was
>> missing in DMA mode so far. Since we can use UART in DMA mode as serial
>> console, this is a quite important improvement.
>>
>> This change additionally simplifies RX handling code, as we no longer
>> need uart_rx_drain_fifo() function, so we can remove it.
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/tty/serial/samsung.c | 30 +++---------------------------
>>  1 file changed, 3 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index 1d7dd86..d72cd73 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -385,32 +385,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>>  	}
>>  }
>>  
>> -static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
>> -				     unsigned long ufstat);
>> -
>> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>> -{
>> -	struct uart_port *port = &ourport->port;
>> -	struct tty_port *tty = &port->state->port;
>> -	unsigned int ch, ufstat;
>> -	unsigned int count;
>> -
>> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
>> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
>> -
>> -	if (!count)
>> -		return;
>> -
>> -	while (count-- > 0) {
>> -		ch = rd_regb(port, S3C2410_URXH);
>> -
>> -		ourport->port.icount.rx++;
>> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
>> -	}
>> -
>> -	tty_flip_buffer_push(tty);
>> -}
>> -
>>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>>  {
>>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>> @@ -573,6 +547,8 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>>  	ourport->rx_mode = S3C24XX_RX_PIO;
>>  }
>>  
>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport);
>> +
>>  static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>  {
>>  	unsigned int utrstat, ufstat, received;
>> @@ -606,7 +582,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>>  		enable_rx_pio(ourport);
>>  	}
>>  

The essence of change is here. We use another method for draining FIFO.
Instead of just putting them into tty buffer, we additionally check for
special conditions, and that's the improvement.

>> -	uart_rx_drain_fifo(ourport);
>> +	s3c24xx_serial_rx_drain_fifo(ourport);
>>  
>>  	if (tty) {
>>  		tty_flip_buffer_push(t);
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222211 — [PATCH v2 3/4] serial: samsung: introduce s3c24xx_serial_rx_drain_fifo() function

This patch introduces s3c24xx_serial_rx_drain_fifo() which reads data
from RX FIFO and writes it to tty buffer. It also checks for special
conditions (such as 'break') and handles it. This function has been
separated from s3c24xx_serial_rx_chars_pio() as it contains code which
can be used also in DMA mode.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/tty/serial/samsung.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index dc4be54..1d7dd86 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -621,16 +621,12 @@ finish:
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
+static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 {
-	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
 	unsigned int ufcon, ch, flag, ufstat, uerstat;
-	unsigned long flags;
 	int max_count = port->fifosize;
 
-	spin_lock_irqsave(&port->lock, flags);
-
 	while (max_count-- > 0) {
 		ufcon = rd_regl(port, S3C2410_UFCON);
 		ufstat = rd_regl(port, S3C2410_UFSTAT);
@@ -654,9 +650,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 					ufcon |= S3C2410_UFCON_RESETRX;
 					wr_regl(port, S3C2410_UFCON, ufcon);
 					rx_enabled(port) = 1;
-					spin_unlock_irqrestore(&port->lock,
-							flags);
-					goto out;
+					return;
 				}
 				continue;
 			}
@@ -702,10 +696,19 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 				 ch, flag);
 	}
 
-	spin_unlock_irqrestore(&port->lock, flags);
 	tty_flip_buffer_push(&port->state->port);
+}
+
+static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
+{
+	struct s3c24xx_uart_port *ourport = dev_id;
+	struct uart_port *port = &ourport->port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	s3c24xx_serial_rx_drain_fifo(ourport);
+	spin_unlock_irqrestore(&port->lock, flags);
 
-out:
 	return IRQ_HANDLED;
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225048 — Re: [PATCH v2 3/4] serial: samsung: introduce s3c24xx_serial_rx_drain_fifo() function

On 09/11/2015 08:15 AM, Krzysztof Kozlowski wrote:
> On 10.09.2015 22:41, Robert Baldyga wrote:
>> This patch introduces s3c24xx_serial_rx_drain_fifo() which reads data
>> from RX FIFO and writes it to tty buffer. It also checks for special
>> conditions (such as 'break') and handles it. This function has been
>> separated from s3c24xx_serial_rx_chars_pio() as it contains code which
>> can be used also in DMA mode.
> 
> Much better, thanks! Now it is also easier to spot the difference (see
> below).
> 
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/tty/serial/samsung.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index dc4be54..1d7dd86 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -621,16 +621,12 @@ finish:
>>  	return IRQ_HANDLED;
>>  }
>>  
>> -static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>>  {
>> -	struct s3c24xx_uart_port *ourport = dev_id;
>>  	struct uart_port *port = &ourport->port;
>>  	unsigned int ufcon, ch, flag, ufstat, uerstat;
>> -	unsigned long flags;
>>  	int max_count = port->fifosize;
>>  
>> -	spin_lock_irqsave(&port->lock, flags);
>> -
>>  	while (max_count-- > 0) {
>>  		ufcon = rd_regl(port, S3C2410_UFCON);
>>  		ufstat = rd_regl(port, S3C2410_UFSTAT);
>> @@ -654,9 +650,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>>  					ufcon |= S3C2410_UFCON_RESETRX;
>>  					wr_regl(port, S3C2410_UFCON, ufcon);
>>  					rx_enabled(port) = 1;
>> -					spin_unlock_irqrestore(&port->lock,
>> -							flags);
>> -					goto out;
>> +					return;
>>  				}
>>  				continue;
>>  			}
>> @@ -702,10 +696,19 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>>  				 ch, flag);
>>  	}
>>  
>> -	spin_unlock_irqrestore(&port->lock, flags);
>>  	tty_flip_buffer_push(&port->state->port);
> 
> Here is a difference - previously this was outside of spinlock. I think
> moving it inside spin lock is okay, just the interrupts won't be
> disabled before unlock and queue_work() from tty_flip_buffer_push().
> 
> However after testing this patchset (entire) on:
> next-20150910 + my dt-for-next branch (dma for serial) + this patchset
> you can see quite complicated lockdep warning:
> 
> [    3.568657] =========================================================
> [    3.575079] [ INFO: possible irq lock inversion dependency detected ]
> [    3.581506] 4.2.0-next-20150910-00009-g65fd5a9cff54 #218 Not tainted
> [    3.587838] ---------------------------------------------------------
> [    3.594263] swapper/0/0 just changed the state of lock:
> [    3.599470]  (&port_lock_key){..-...}, at: [<c02a8fac>]
> s3c24xx_serial_tx_dma_complete+0x8c/0xfc
> [    3.608237] but this lock took another, SOFTIRQ-unsafe lock in the past:
> [    3.614919]  (&(&ctx->lock)->rlock){+.+...}
> [    3.614919]
> [    3.614919] and interrupts could create inverse lock ordering between
> them.
> [    3.614919]
> [    3.625076]
> [    3.625076] other info that might help us debug this:
> [    3.631586]  Possible interrupt unsafe locking scenario:
> [    3.631586]
> [    3.638356]        CPU0                    CPU1
> [    3.642870]        ----                    ----
> [    3.647382]   lock(&(&ctx->lock)->rlock);
> [    3.651376]                                local_irq_disable();
> [    3.657278]                                lock(&port_lock_key);
> [    3.663267]                                lock(&(&ctx->lock)->rlock);
> [    3.669777]   <Interrupt>
> [    3.672381]     lock(&port_lock_key);
> 
> 
> Config: exynos, disabled MMC_CLKGATE, enabled usual testing stuff
> Board: Trats2
> 
> Didn't you notice it?
> 
> 
> Additionally the SysRq "Show backtrace of all active CPUs" on this
> linux-next (without additional patches, pure next) has significant delay
> (like 5 seconds) and a:
> [  169.221223] s3c-i2c 138d0000.i2c: timeout waiting for bus idle
> 
> That's weird. But as I said this occurs on pure next as well.
> 

I've noticed that commit [1] reveals the problem, but in result of git
bisect (at each step I was applying [1]) I got:

d71fc239b6915a8b750e9a447311029ff45b6580 is the first bad commit

That points to [2], which looks strange to me. However bug seems to be
unrelated to my patches, so I will send v3 soon.

[1] commit: 22374fbedb2ce03ef81323b3f7ceb3fa29344aa6
    "ARM: dts: Add DMA support for serial ports in exynos4"

[2] commit: d71fc239b6915a8b750e9a447311029ff45b6580
    "Merge tag 'armsoc-late' of
git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc"


Best regards,
Robert Baldyga

> 
>> +}
>> +
>> +static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>> +{
>> +	struct s3c24xx_uart_port *ourport = dev_id;
>> +	struct uart_port *port = &ourport->port;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&port->lock, flags);
>> +	s3c24xx_serial_rx_drain_fifo(ourport);
>> +	spin_unlock_irqrestore(&port->lock, flags);
>>  
>> -out:
>>  	return IRQ_HANDLED;
>>  }
>>  
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web