diff mbox series

[RFT,v3,21/27] tty: serial: samsung_tty: IRQ rework

Message ID 20210304213902.83903-22-marcan@marcan.st (mailing list archive)
State New, archived
Headers show
Series Apple M1 SoC platform bring-up | expand

Commit Message

Hector Martin March 4, 2021, 9:38 p.m. UTC
* Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq,
  where only the latter acquires the port lock. This will be necessary
  on platforms which have edge-triggered IRQs, as we need to call
  s3c24xx_serial_tx_chars to kick off transmission from outside IRQ
  context, with the port lock held.

* Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for
  consistency with the above. All it does now is call two other
  functions anyway.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/tty/serial/samsung_tty.c | 34 +++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 14 deletions(-)

Comments

Krzysztof Kozlowski March 5, 2021, 10:51 a.m. UTC | #1
On 04/03/2021 22:38, Hector Martin wrote:
> * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq,
>   where only the latter acquires the port lock. This will be necessary
>   on platforms which have edge-triggered IRQs, as we need to call
>   s3c24xx_serial_tx_chars to kick off transmission from outside IRQ
>   context, with the port lock held.
> 
> * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for
>   consistency with the above. All it does now is call two other
>   functions anyway.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/tty/serial/samsung_tty.c | 34 +++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Best regards,
Krzysztof
Andy Shevchenko March 5, 2021, 3:17 p.m. UTC | #2
On Thu, Mar 4, 2021 at 11:41 PM Hector Martin <marcan@marcan.st> wrote:
>
> * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq,
>   where only the latter acquires the port lock. This will be necessary
>   on platforms which have edge-triggered IRQs, as we need to call
>   s3c24xx_serial_tx_chars to kick off transmission from outside IRQ
>   context, with the port lock held.
>
> * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for
>   consistency with the above. All it does now is call two other
>   functions anyway.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/tty/serial/samsung_tty.c | 34 +++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 39b2eb165bdc..7106eb238d8c 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -827,7 +827,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>         return IRQ_HANDLED;
>  }
>
> -static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
> +static irqreturn_t s3c24xx_serial_rx_irq(int irq, void *dev_id)
>  {
>         struct s3c24xx_uart_port *ourport = dev_id;
>
> @@ -836,16 +836,12 @@ static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
>         return s3c24xx_serial_rx_chars_pio(dev_id);
>  }
>
> -static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
> +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport)
>  {
> -       struct s3c24xx_uart_port *ourport = id;
>         struct uart_port *port = &ourport->port;
>         struct circ_buf *xmit = &port->state->xmit;
> -       unsigned long flags;
>         int count, dma_count = 0;
>
> -       spin_lock_irqsave(&port->lock, flags);
> -
>         count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
>
>         if (ourport->dma && ourport->dma->tx_chan &&
> @@ -862,7 +858,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>                 wr_reg(port, S3C2410_UTXH, port->x_char);
>                 port->icount.tx++;
>                 port->x_char = 0;
> -               goto out;
> +               return;
>         }
>
>         /* if there isn't anything more to transmit, or the uart is now
> @@ -871,7 +867,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>
>         if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
>                 s3c24xx_serial_stop_tx(port);
> -               goto out;
> +               return;
>         }
>
>         /* try and drain the buffer... */
> @@ -893,7 +889,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>
>         if (!count && dma_count) {
>                 s3c24xx_serial_start_tx_dma(ourport, dma_count);
> -               goto out;
> +               return;
>         }
>
>         if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
> @@ -904,8 +900,18 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>
>         if (uart_circ_empty(xmit))
>                 s3c24xx_serial_stop_tx(port);
> +}
> +
> +static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id)
> +{
> +       struct s3c24xx_uart_port *ourport = id;
> +       struct uart_port *port = &ourport->port;
> +       unsigned long flags;
> +


> +       spin_lock_irqsave(&port->lock, flags);
> +
> +       s3c24xx_serial_tx_chars(ourport);
>
> -out:
>         spin_unlock_irqrestore(&port->lock, flags);

Add a separate change that removes flags from the spin lock in the IRQ handler.

>         return IRQ_HANDLED;
>  }
> @@ -919,11 +925,11 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id)
>         irqreturn_t ret = IRQ_HANDLED;
>
>         if (pend & S3C64XX_UINTM_RXD_MSK) {
> -               ret = s3c24xx_serial_rx_chars(irq, id);
> +               ret = s3c24xx_serial_rx_irq(irq, id);
>                 wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_RXD_MSK);
>         }
>         if (pend & S3C64XX_UINTM_TXD_MSK) {
> -               ret = s3c24xx_serial_tx_chars(irq, id);
> +               ret = s3c24xx_serial_tx_irq(irq, id);
>                 wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_TXD_MSK);
>         }
>         return ret;
> @@ -1155,7 +1161,7 @@ static int s3c24xx_serial_startup(struct uart_port *port)
>
>         ourport->rx_enabled = 1;
>
> -       ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_chars, 0,
> +       ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_irq, 0,
>                           s3c24xx_serial_portname(port), ourport);
>
>         if (ret != 0) {
> @@ -1169,7 +1175,7 @@ static int s3c24xx_serial_startup(struct uart_port *port)
>
>         ourport->tx_enabled = 1;
>
> -       ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_chars, 0,
> +       ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_irq, 0,
>                           s3c24xx_serial_portname(port), ourport);
>
>         if (ret) {
> --
> 2.30.0
>
Hector Martin March 5, 2021, 4:16 p.m. UTC | #3
On 06/03/2021 00.17, Andy Shevchenko wrote:
> Add a separate change that removes flags from the spin lock in the IRQ handler.

This commit should have no functional changes; I am just splitting an 
existing function into two, where one takes the lock and the other does 
the work. Do you mean using a different locking function? I'm not 
entirely sure what you're suggesting.
Andy Shevchenko March 5, 2021, 4:20 p.m. UTC | #4
On Fri, Mar 5, 2021 at 6:16 PM Hector Martin <marcan@marcan.st> wrote:
>
> On 06/03/2021 00.17, Andy Shevchenko wrote:
> > Add a separate change that removes flags from the spin lock in the IRQ handler.
>
> This commit should have no functional changes;

Exactly my point why I'm suggesting to have _separate_ change!

> I am just splitting an
> existing function into two, where one takes the lock and the other does
> the work. Do you mean using a different locking function? I'm not
> entirely sure what you're suggesting.

Yes, as a prerequisite

spin_lock_irqsave -> spin_lock().
Hector Martin March 5, 2021, 4:29 p.m. UTC | #5
On 06/03/2021 01.20, Andy Shevchenko wrote:
>> I am just splitting an
>> existing function into two, where one takes the lock and the other does
>> the work. Do you mean using a different locking function? I'm not
>> entirely sure what you're suggesting.
> 
> Yes, as a prerequisite
> 
> spin_lock_irqsave -> spin_lock().

Krzysztof, is this something you want in this series? I was trying to 
avoid logic changes to the non-Apple paths.
Krzysztof Kozlowski March 7, 2021, 11:34 a.m. UTC | #6
On 05/03/2021 17:29, Hector Martin wrote:
> On 06/03/2021 01.20, Andy Shevchenko wrote:
>>> I am just splitting an
>>> existing function into two, where one takes the lock and the other does
>>> the work. Do you mean using a different locking function? I'm not
>>> entirely sure what you're suggesting.
>>
>> Yes, as a prerequisite
>>
>> spin_lock_irqsave -> spin_lock().
> 
> Krzysztof, is this something you want in this series? I was trying to 
> avoid logic changes to the non-Apple paths.

I don't quite get the need for such change (the code will be still
called in interrupt handler, right?), but assuming the "why?" is
properly documented, it can be a separate patch here.

Best regards,
Krzysztof
Arnd Bergmann March 7, 2021, 4:01 p.m. UTC | #7
On Sun, Mar 7, 2021 at 12:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
> On 05/03/2021 17:29, Hector Martin wrote:
> > On 06/03/2021 01.20, Andy Shevchenko wrote:
> >>> I am just splitting an
> >>> existing function into two, where one takes the lock and the other does
> >>> the work. Do you mean using a different locking function? I'm not
> >>> entirely sure what you're suggesting.
> >>
> >> Yes, as a prerequisite
> >>
> >> spin_lock_irqsave -> spin_lock().
> >
> > Krzysztof, is this something you want in this series? I was trying to
> > avoid logic changes to the non-Apple paths.
>
> I don't quite get the need for such change (the code will be still
> called in interrupt handler, right?), but assuming the "why?" is
> properly documented, it can be a separate patch here.

This is only for readability: the common rule is to not disable
interrupts when they are already disabled, so a reader might wonder
if this instance of the handler is special in some case that it might
be called with interrupts enabled.

There is also a small overhead in accessing the global irq mask
register on some architectures, but for a uart that does not make
any difference of course.

While I'm generally in favor of that kind of cleanup, I'd also
prefer to leave it out of this series -- once you get into details
like this the series gets harder to review.

        Arnd
Krzysztof Kozlowski March 7, 2021, 7:51 p.m. UTC | #8
On 07/03/2021 17:01, Arnd Bergmann wrote:
> On Sun, Mar 7, 2021 at 12:34 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>> On 05/03/2021 17:29, Hector Martin wrote:
>>> On 06/03/2021 01.20, Andy Shevchenko wrote:
>>>>> I am just splitting an
>>>>> existing function into two, where one takes the lock and the other does
>>>>> the work. Do you mean using a different locking function? I'm not
>>>>> entirely sure what you're suggesting.
>>>>
>>>> Yes, as a prerequisite
>>>>
>>>> spin_lock_irqsave -> spin_lock().
>>>
>>> Krzysztof, is this something you want in this series? I was trying to
>>> avoid logic changes to the non-Apple paths.
>>
>> I don't quite get the need for such change (the code will be still
>> called in interrupt handler, right?), but assuming the "why?" is
>> properly documented, it can be a separate patch here.
> 
> This is only for readability: the common rule is to not disable
> interrupts when they are already disabled, so a reader might wonder
> if this instance of the handler is special in some case that it might
> be called with interrupts enabled.
> 
> There is also a small overhead in accessing the global irq mask
> register on some architectures, but for a uart that does not make
> any difference of course.
> 
> While I'm generally in favor of that kind of cleanup, I'd also
> prefer to leave it out of this series -- once you get into details
> like this the series gets harder to review.

So it's only about the spinlock in the IRQ handler (which does not need
to disable the IRQs). Makes sense but not related at all to the topic of
bringing up Apple M1, therefore should not stop the review/merging.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 39b2eb165bdc..7106eb238d8c 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -827,7 +827,7 @@  static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
+static irqreturn_t s3c24xx_serial_rx_irq(int irq, void *dev_id)
 {
 	struct s3c24xx_uart_port *ourport = dev_id;
 
@@ -836,16 +836,12 @@  static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
 	return s3c24xx_serial_rx_chars_pio(dev_id);
 }
 
-static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
+static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport)
 {
-	struct s3c24xx_uart_port *ourport = id;
 	struct uart_port *port = &ourport->port;
 	struct circ_buf *xmit = &port->state->xmit;
-	unsigned long flags;
 	int count, dma_count = 0;
 
-	spin_lock_irqsave(&port->lock, flags);
-
 	count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
 
 	if (ourport->dma && ourport->dma->tx_chan &&
@@ -862,7 +858,7 @@  static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
-		goto out;
+		return;
 	}
 
 	/* if there isn't anything more to transmit, or the uart is now
@@ -871,7 +867,7 @@  static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 
 	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
 		s3c24xx_serial_stop_tx(port);
-		goto out;
+		return;
 	}
 
 	/* try and drain the buffer... */
@@ -893,7 +889,7 @@  static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 
 	if (!count && dma_count) {
 		s3c24xx_serial_start_tx_dma(ourport, dma_count);
-		goto out;
+		return;
 	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
@@ -904,8 +900,18 @@  static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 
 	if (uart_circ_empty(xmit))
 		s3c24xx_serial_stop_tx(port);
+}
+
+static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id)
+{
+	struct s3c24xx_uart_port *ourport = id;
+	struct uart_port *port = &ourport->port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	s3c24xx_serial_tx_chars(ourport);
 
-out:
 	spin_unlock_irqrestore(&port->lock, flags);
 	return IRQ_HANDLED;
 }
@@ -919,11 +925,11 @@  static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id)
 	irqreturn_t ret = IRQ_HANDLED;
 
 	if (pend & S3C64XX_UINTM_RXD_MSK) {
-		ret = s3c24xx_serial_rx_chars(irq, id);
+		ret = s3c24xx_serial_rx_irq(irq, id);
 		wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_RXD_MSK);
 	}
 	if (pend & S3C64XX_UINTM_TXD_MSK) {
-		ret = s3c24xx_serial_tx_chars(irq, id);
+		ret = s3c24xx_serial_tx_irq(irq, id);
 		wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_TXD_MSK);
 	}
 	return ret;
@@ -1155,7 +1161,7 @@  static int s3c24xx_serial_startup(struct uart_port *port)
 
 	ourport->rx_enabled = 1;
 
-	ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_chars, 0,
+	ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_irq, 0,
 			  s3c24xx_serial_portname(port), ourport);
 
 	if (ret != 0) {
@@ -1169,7 +1175,7 @@  static int s3c24xx_serial_startup(struct uart_port *port)
 
 	ourport->tx_enabled = 1;
 
-	ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_chars, 0,
+	ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_irq, 0,
 			  s3c24xx_serial_portname(port), ourport);
 
 	if (ret) {