Re: [PATCH v3 10/15] serial: 8250_mxpcie: defer uart_write_wakeup() to workqueue

[Jiri Slaby <jirislaby@kernel.org>]

On 09. 07. 26, 7:33, Crescent Hsieh wrote:
> Avoid calling uart_write_wakeup() directly from the interrupt-driven TX
> path.
> 
> Defer the wakeup to a per-port work item and coalesce multiple TX events
> using a pending flag, so only one wakeup is scheduled while a previous
> one is still outstanding.

I am a bit worried of this added complexity. So what's the measured 
benefit of this actually?

What multiple TX events do you mean to coalesce? The work would run 
right after the ISR is completed, or what am I missing?

I am likely missing also the purpose of the MOXA_EVENT_TXLOW flag. The 
work won't be scheduled twice. And when it is scheduled once, the event 
is set, so no need to check it in the work. Or do you plan on more EVENTs?

> Signed-off-by: Crescent Hsieh <crescentcy.hsieh@moxa.com>
> ---
>   drivers/tty/serial/8250/8250_mxpcie.c | 34 +++++++++++++++++++++++----
>   1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
> index c7467fb94021..7b39ad19b331 100644
> --- a/drivers/tty/serial/8250/8250_mxpcie.c
> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
> @@ -15,6 +15,7 @@
>   #include <linux/pci.h>
>   #include <linux/tty_flip.h>
>   #include <linux/types.h>
> +#include <linux/workqueue.h>
>   
>   #include <linux/8250_pci.h>
>   #include <linux/serial_8250.h>
> @@ -104,7 +105,12 @@
>   #define MOXA_EVEN_RS_MASK	GENMASK(3, 0)
>   #define MOXA_ODD_RS_MASK	GENMASK(7, 4)
>   
> +#define MOXA_EVENT_TXLOW	BIT(0)
> +
>   struct mxpcie8250_port {
> +	struct uart_port *port;
> +	struct work_struct work;
> +	unsigned long event_flags;
>   	int line;
>   };
>   
> @@ -328,6 +334,8 @@ static void mxpcie8250_tx_chars(struct uart_8250_port *up)
>   {
>   	struct uart_port *port = &up->port;
>   	struct tty_port *tport = &port->state->port;
> +	struct device *dev = port->dev;
> +	struct mxpcie8250 *priv = dev_get_drvdata(dev);
>   	unsigned int count, txsize;
>   	unsigned char c;
>   
> @@ -348,9 +356,10 @@ static void mxpcie8250_tx_chars(struct uart_8250_port *up)
>   
>   		serial_out(up, MOXA_PUART_TX_FIFO_MEM + i, c);
>   	}
> -	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
> -		uart_write_wakeup(port);
> -
> +	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS) {
> +		if (!test_and_set_bit(MOXA_EVENT_TXLOW, &priv->port[port->port_id].event_flags))
> +			schedule_work(&priv->port[port->port_id].work);
> +	}
>   	if (kfifo_is_empty(&tport->xmit_fifo) && !(up->capabilities & UART_CAP_RPM))
>   		port->ops->stop_tx(port);
>   }
> @@ -377,6 +386,14 @@ static int mxpcie8250_handle_irq(struct uart_port *port)
>   	return 1;
>   }
>   
> +static void mxpcie8250_work_handler(struct work_struct *work)
> +{
> +	struct mxpcie8250_port *priv_port = container_of(work, struct mxpcie8250_port, work);
> +
> +	if (test_and_clear_bit(MOXA_EVENT_TXLOW, &priv_port->event_flags))
> +		uart_write_wakeup(priv_port->port);
> +}
> +
>   static void mxpcie8250_init_board(struct pci_dev *pdev, struct mxpcie8250 *priv)
>   {
>   	void __iomem *bar2_base = priv->bar2_base;
> @@ -432,7 +449,7 @@ static void mxpcie8250_setup_port(struct pci_dev *pdev,
>   static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
>   	struct device *dev = &pdev->dev;
> -	struct uart_8250_port up = {};
> +	struct uart_8250_port up = {}, *new_port;
>   	struct mxpcie8250 *priv;
>   	unsigned short device = pdev->device;
>   	unsigned int num_ports;
> @@ -492,6 +509,11 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>   				up.port.iotype, priv->port[i].line);
>   			break;
>   		}
> +		new_port = serial8250_get_port(priv->port[i].line);
> +
> +		priv->port[i].port = &new_port->port;
> +
> +		INIT_WORK(&priv->port[i].work, mxpcie8250_work_handler);
>   	}
>   	pci_set_drvdata(pdev, priv);
>   
> @@ -502,8 +524,10 @@ static void mxpcie8250_remove(struct pci_dev *pdev)
>   {
>   	struct mxpcie8250 *priv = pci_get_drvdata(pdev);
>   
> -	for (unsigned int i = 0; i < priv->num_ports; i++)
> +	for (unsigned int i = 0; i < priv->num_ports; i++) {
> +		cancel_work_sync(&priv->port[i].work);
>   		serial8250_unregister_port(priv->port[i].line);
> +	}
>   }
>   
>   static const struct pci_device_id mxpcie8250_pci_ids[] = {


-- 
js
suse labs