On 09/10/2015 02:47 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 18:38, Robert Baldyga wrote:
>> This patch fixes UART status handling in DMA mode.
>
> Please describe the issue - what kind of issues are in UART status
> handling? I think this fixes a specific bug which can be observed by the
> user, so it should be mentioned here. You mention this a little bit
> later but I would miss it if I were not aware earlier of the issue.
>
>> For this purpose we
>> introduce s3c24xx_serial_rx_drain_fifo() function, which contains common
>> RX handling code for both PIO mode and DMA mode (in case of interrupt).
>>
>> Thanks to this we have, among others, 'break' condition handling needed
>> for Magic SysRq, which was missing in DMA mode so far. Since we can use
>> UART in DMA mode as serial console, it's 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.
>
> Maybe it's because I don't know the driver but I have difficulties to
> spot the exact fix. There is significant code movement... To spot the
> difference I even applied the patch, it helped...
>
> I think it could be split for easier understanding. First introduce
> common code (s3c24xx_serial_rx_drain_fifo()) and then fix the issue.
> Yeah, I am grumpy but really the fix should be made visible (easier to
> spot).
>
Well, what I did is quite simple, but indeed the result diff looks ugly.
The change is: instead of doing simple FIFO drain when interrupt occurs
(which can be caused by simple timeout or by special condition like
'break') I'm calling function which drains FIFO *and checks the special
conditions*. So now we have special condition handling in DMA mode which
was missing. To avoid code duplication I have separated this function
from PIO handling code.
I can try to split this change into some steps. Maybe it will produce
some more readable diff ;)
Thanks,
Robert
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
>> ---
>> drivers/tty/serial/samsung.c | 142 +++++++++++++++++++------------------------
>> 1 file changed, 61 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index 2b05194..007bc5f 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -388,29 +388,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,64 +550,12 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>> ourport->rx_mode = S3C24XX_RX_PIO;
>> }
>>
>> -static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>> {
>> - unsigned int utrstat, ufstat, received;
>> - struct s3c24xx_uart_port *ourport = dev_id;
>> - struct uart_port *port = &ourport->port;
>> - struct s3c24xx_uart_dma *dma = ourport->dma;
>> - struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
>> - struct tty_port *t = &port->state->port;
>> - unsigned long flags;
>> - struct dma_tx_state state;
>> -
>> - utrstat = rd_regl(port, S3C2410_UTRSTAT);
>> - ufstat = rd_regl(port, S3C2410_UFSTAT);
>> -
>> - spin_lock_irqsave(&port->lock, flags);
>> -
>> - if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
>> - s3c64xx_start_rx_dma(ourport);
>> - if (ourport->rx_mode == S3C24XX_RX_PIO)
>> - enable_rx_dma(ourport);
>> - goto finish;
>> - }
>> -
>> - if (ourport->rx_mode == S3C24XX_RX_DMA) {
>> - dmaengine_pause(dma->rx_chan);
>> - dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
>> - dmaengine_terminate_all(dma->rx_chan);
>> - received = dma->rx_bytes_requested - state.residue;
>> - s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
>> -
>> - enable_rx_pio(ourport);
>> - }
>> -
>> - uart_rx_drain_fifo(ourport);
>> -
>> - if (tty) {
>> - tty_flip_buffer_push(t);
>> - tty_kref_put(tty);
>> - }
>> -
>> - wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
>> -
>> -finish:
>> - spin_unlock_irqrestore(&port->lock, flags);
>> -
>> - return IRQ_HANDLED;
>> -}
>> -
>> -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 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 +579,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 +625,67 @@ 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_dma(void *dev_id)
>> +{
>> + unsigned int utrstat, ufstat, received;
>> + struct s3c24xx_uart_port *ourport = dev_id;
>> + struct uart_port *port = &ourport->port;
>> + struct s3c24xx_uart_dma *dma = ourport->dma;
>> + struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
>> + struct tty_port *t = &port->state->port;
>> + unsigned long flags;
>> + struct dma_tx_state state;
>> +
>> + utrstat = rd_regl(port, S3C2410_UTRSTAT);
>> + ufstat = rd_regl(port, S3C2410_UFSTAT);
>> +
>> + spin_lock_irqsave(&port->lock, flags);
>> +
>> + if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
>> + s3c64xx_start_rx_dma(ourport);
>> + if (ourport->rx_mode == S3C24XX_RX_PIO)
>> + enable_rx_dma(ourport);
>> + goto finish;
>> + }
>> +
>> + if (ourport->rx_mode == S3C24XX_RX_DMA) {
>> + dmaengine_pause(dma->rx_chan);
>> + dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
>> + dmaengine_terminate_all(dma->rx_chan);
>> + received = dma->rx_bytes_requested - state.residue;
>> + s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
>> +
>> + enable_rx_pio(ourport);
>> + }
>> +
>> + s3c24xx_serial_rx_drain_fifo(ourport);
>> +
>> + if (tty) {
>> + tty_flip_buffer_push(t);
>> + tty_kref_put(tty);
>> + }
>> +
>> + wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
>> +
>> +finish:
>> + spin_unlock_irqrestore(&port->lock, flags);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
