2024-04-03 16:39:09

by Allen Pais

[permalink] [raw]
Subject: [PATCH] tty: Convert from tasklet to BH workqueue

The only generic interface to execute asynchronously in the BH context is
tasklet; however, it's marked deprecated and has some design flaws. To
replace tasklets, BH workqueue support was recently added. A BH workqueue
behaves similarly to regular workqueues except that the queued work items
are executed in the BH context.

This patch converts drivers/tty/* from tasklet to BH workqueue.

Based on the work done by Tejun Heo <[email protected]>
Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10

Signed-off-by: Allen Pais <[email protected]>
---
drivers/tty/ipwireless/hardware.c | 15 +++---
drivers/tty/serial/atmel_serial.c | 79 ++++++++++++++++---------------
drivers/tty/serial/timbuart.c | 19 ++++----
drivers/tty/vt/keyboard.c | 34 ++++++-------
4 files changed, 76 insertions(+), 71 deletions(-)

diff --git a/drivers/tty/ipwireless/hardware.c b/drivers/tty/ipwireless/hardware.c
index 001ec318a918..a8bf3bd8f23a 100644
--- a/drivers/tty/ipwireless/hardware.c
+++ b/drivers/tty/ipwireless/hardware.c
@@ -22,6 +22,7 @@
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/slab.h>
+#include <linux/workqueue.h>

#include "hardware.h"
#include "setup_protocol.h"
@@ -275,7 +276,7 @@ struct ipw_hardware {
unsigned int control_lines[NL_NUM_OF_ADDRESSES];
struct ipw_rx_packet *packet_assembler[NL_NUM_OF_ADDRESSES];

- struct tasklet_struct tasklet;
+ struct work_struct work;

/* The handle for the network layer, for the sending of events to it. */
struct ipw_network *network;
@@ -1006,9 +1007,9 @@ static int send_pending_packet(struct ipw_hardware *hw, int priority_limit)
/*
* Send and receive all queued packets.
*/
-static void ipwireless_do_tasklet(struct tasklet_struct *t)
+static void ipwireless_do_work(struct work_struct *t)
{
- struct ipw_hardware *hw = from_tasklet(hw, t, tasklet);
+ struct ipw_hardware *hw = from_work(hw, t, work);
unsigned long flags;

spin_lock_irqsave(&hw->lock, flags);
@@ -1084,7 +1085,7 @@ static irqreturn_t ipwireless_handle_v1_interrupt(int irq,
}
if (ack != 0) {
outw(ack, hw->base_port + IOIR);
- tasklet_schedule(&hw->tasklet);
+ queue_work(system_bh_wq, &hw->work);
}
return IRQ_HANDLED;
}
@@ -1189,7 +1190,7 @@ static irqreturn_t ipwireless_handle_v2_v3_interrupt(int irq,
acknowledge_pcmcia_interrupt(hw);

if (tx || rx)
- tasklet_schedule(&hw->tasklet);
+ queue_work(system_bh_wq, &hw->work);
else if (!rx_repeat) {
if (hw->memreg_tx == &hw->memory_info_regs->memreg_tx_new) {
if (hw->serial_number_detected)
@@ -1635,7 +1636,7 @@ struct ipw_hardware *ipwireless_hardware_create(void)
INIT_LIST_HEAD(&hw->rx_queue);
INIT_LIST_HEAD(&hw->rx_pool);
spin_lock_init(&hw->lock);
- tasklet_setup(&hw->tasklet, ipwireless_do_tasklet);
+ INIT_WORK(&hw->work, ipwireless_do_work);
INIT_WORK(&hw->work_rx, ipw_receive_data_work);
timer_setup(&hw->setup_timer, ipwireless_setup_timer, 0);

@@ -1703,7 +1704,7 @@ static void ipwireless_setup_timer(struct timer_list *t)
hw->to_setup = 1;
hw->tx_ready = 1;
spin_unlock_irqrestore(&hw->lock, flags);
- tasklet_schedule(&hw->tasklet);
+ queue_work(system_bh_wq, &hw->work);
}

mod_timer(&hw->setup_timer,
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 85667f709515..09e5fa1c56b6 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -50,6 +50,7 @@
#define ATMEL_RTS_LOW_OFFSET 20

#include <linux/serial_core.h>
+#include <linux/workqueue.h>

#include "serial_mctrl_gpio.h"
#include "atmel_serial.h"
@@ -134,8 +135,8 @@ struct atmel_uart_port {
dma_cookie_t cookie_rx;
struct scatterlist sg_tx;
struct scatterlist sg_rx;
- struct tasklet_struct tasklet_rx;
- struct tasklet_struct tasklet_tx;
+ struct work_struct work_rx;
+ struct work_struct work_tx;
atomic_t tasklet_shutdown;
unsigned int irq_status_prev;
unsigned int tx_len;
@@ -282,11 +283,11 @@ static bool atmel_use_fifo(struct uart_port *port)
return atmel_port->fifo_size;
}

-static void atmel_tasklet_schedule(struct atmel_uart_port *atmel_port,
- struct tasklet_struct *t)
+static void atmel_queue_work(struct atmel_uart_port *atmel_port,
+ struct work_struct *t)
{
- if (!atomic_read(&atmel_port->tasklet_shutdown))
- tasklet_schedule(t);
+ if (!atomic_read(&atmel_port->work_shutdown))
+ queue_work(system_bh_wq, t);
}

/* Enable or disable the rs485 support */
@@ -821,11 +822,11 @@ static void atmel_rx_chars(struct uart_port *port)
status = atmel_uart_readl(port, ATMEL_US_CSR);
}

- atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_rx);
+ atmel_queue_work(atmel_port, &atmel_port->work_rx);
}

/*
- * Transmit characters (called from tasklet with TXRDY interrupt
+ * Transmit characters (called from workqueue with TXRDY interrupt
* disabled)
*/
static void atmel_tx_chars(struct uart_port *port)
@@ -882,7 +883,7 @@ static void atmel_complete_tx_dma(void *arg)
* remaining data from the beginning of xmit->buf to xmit->head.
*/
if (!uart_circ_empty(xmit))
- atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
+ atmel_queue_work(atmel_port, &atmel_port->work_tx);
else if (atmel_uart_is_half_duplex(port)) {
/*
* DMA done, re-enable TXEMPTY and signal that we can stop
@@ -914,7 +915,7 @@ static void atmel_release_tx_dma(struct uart_port *port)
}

/*
- * Called from tasklet with TXRDY interrupt is disabled.
+ * Called from workqueue with TXRDY interrupt is disabled.
*/
static void atmel_tx_dma(struct uart_port *port)
{
@@ -1082,7 +1083,7 @@ static void atmel_complete_rx_dma(void *arg)
struct uart_port *port = arg;
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);

- atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_rx);
+ atmel_queue_work(atmel_port, &atmel_port->work_rx);
}

static void atmel_release_rx_dma(struct uart_port *port)
@@ -1118,11 +1119,11 @@ static void atmel_rx_from_dma(struct uart_port *port)
dmastat = dmaengine_tx_status(chan,
atmel_port->cookie_rx,
&state);
- /* Restart a new tasklet if DMA status is error */
+ /* Queue a new work if DMA status is error */
if (dmastat == DMA_ERROR) {
- dev_dbg(port->dev, "Get residue error, restart tasklet\n");
+ dev_dbg(port->dev, "Get residue error, restart work\n");
atmel_uart_writel(port, ATMEL_US_IER, ATMEL_US_TIMEOUT);
- atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_rx);
+ atmel_queue_work(atmel_port, &atmel_port->work_rx);
return;
}

@@ -1288,7 +1289,7 @@ static void atmel_uart_timer_callback(struct timer_list *t)
struct uart_port *port = &atmel_port->uart;

if (!atomic_read(&atmel_port->tasklet_shutdown)) {
- tasklet_schedule(&atmel_port->tasklet_rx);
+ queue_work(system_bh_wq, &atmel_port->tasklet_rx);
mod_timer(&atmel_port->uart_timer,
jiffies + uart_poll_timeout(port));
}
@@ -1304,7 +1305,7 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)

if (atmel_use_pdc_rx(port)) {
/*
- * PDC receive. Just schedule the tasklet and let it
+ * PDC receive. Just schedule the task and let it
* figure out the details.
*
* TODO: We're not handling error flags correctly at
@@ -1313,8 +1314,8 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
if (pending & (ATMEL_US_ENDRX | ATMEL_US_TIMEOUT)) {
atmel_uart_writel(port, ATMEL_US_IDR,
(ATMEL_US_ENDRX | ATMEL_US_TIMEOUT));
- atmel_tasklet_schedule(atmel_port,
- &atmel_port->tasklet_rx);
+ atmel_queue_work(atmel_port,
+ &atmel_port->work_rx);
}

if (pending & (ATMEL_US_RXBRK | ATMEL_US_OVRE |
@@ -1326,8 +1327,8 @@ atmel_handle_receive(struct uart_port *port, unsigned int pending)
if (pending & ATMEL_US_TIMEOUT) {
atmel_uart_writel(port, ATMEL_US_IDR,
ATMEL_US_TIMEOUT);
- atmel_tasklet_schedule(atmel_port,
- &atmel_port->tasklet_rx);
+ atmel_queue_work(atmel_port,
+ &atmel_port->work_rx);
}
}

@@ -1367,7 +1368,7 @@ atmel_handle_transmit(struct uart_port *port, unsigned int pending)
atmel_start_rx(port);
}

- atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
+ atmel_queue_work(atmel_port, &atmel_port->work_tx);
}
}

@@ -1454,7 +1455,7 @@ static void atmel_release_tx_pdc(struct uart_port *port)
}

/*
- * Called from tasklet with ENDTX and TXBUFE interrupts disabled.
+ * Called from workqueue with ENDTX and TXBUFE interrupts disabled.
*/
static void atmel_tx_pdc(struct uart_port *port)
{
@@ -1710,12 +1711,12 @@ static int atmel_prepare_rx_pdc(struct uart_port *port)
}

/*
- * tasklet handling tty stuff outside the interrupt handler.
+ * task handling tty stuff outside the interrupt handler.
*/
-static void atmel_tasklet_rx_func(struct tasklet_struct *t)
+static void atmel_work_rx_func(struct work_struct *t)
{
- struct atmel_uart_port *atmel_port = from_tasklet(atmel_port, t,
- tasklet_rx);
+ struct atmel_uart_port *atmel_port = from_work(atmel_port, t,
+ work_rx);
struct uart_port *port = &atmel_port->uart;

/* The interrupt handler does not take the lock */
@@ -1724,10 +1725,10 @@ static void atmel_tasklet_rx_func(struct tasklet_struct *t)
uart_port_unlock(port);
}

-static void atmel_tasklet_tx_func(struct tasklet_struct *t)
+static void atmel_work_tx_func(struct work_struct *t)
{
- struct atmel_uart_port *atmel_port = from_tasklet(atmel_port, t,
- tasklet_tx);
+ struct atmel_uart_port *atmel_port = from_work(atmel_port, t,
+ work_tx);
struct uart_port *port = &atmel_port->uart;

/* The interrupt handler does not take the lock */
@@ -1906,8 +1907,8 @@ static int atmel_startup(struct uart_port *port)
}

atomic_set(&atmel_port->tasklet_shutdown, 0);
- tasklet_setup(&atmel_port->tasklet_rx, atmel_tasklet_rx_func);
- tasklet_setup(&atmel_port->tasklet_tx, atmel_tasklet_tx_func);
+ INIT_WORK(&atmel_port->tasklet_rx, atmel_work_rx_func);
+ INIT_WORK(&atmel_port->tasklet_tx, atmel_work_tx_func);

/*
* Initialize DMA (if necessary)
@@ -1953,7 +1954,7 @@ static int atmel_startup(struct uart_port *port)
atmel_uart_writel(port, ATMEL_US_FMR, fmr);
}

- /* Save current CSR for comparison in atmel_tasklet_func() */
+ /* Save current CSR for comparison in atmel_work_func() */
atmel_port->irq_status_prev = atmel_uart_readl(port, ATMEL_US_CSR);

/*
@@ -2036,11 +2037,11 @@ static void atmel_shutdown(struct uart_port *port)
/* Disable interrupts at device level */
atmel_uart_writel(port, ATMEL_US_IDR, -1);

- /* Prevent spurious interrupts from scheduling the tasklet */
+ /* Prevent spurious interrupts from scheduling the task */
atomic_inc(&atmel_port->tasklet_shutdown);

/*
- * Prevent any tasklets being scheduled during
+ * Prevent any tasks being scheduled during
* cleanup
*/
del_timer_sync(&atmel_port->uart_timer);
@@ -2049,11 +2050,11 @@ static void atmel_shutdown(struct uart_port *port)
synchronize_irq(port->irq);

/*
- * Clear out any scheduled tasklets before
+ * Clear out any scheduled tasks before
* we destroy the buffers
*/
- tasklet_kill(&atmel_port->tasklet_rx);
- tasklet_kill(&atmel_port->tasklet_tx);
+ cancel_work_sync(&atmel_port->work_rx);
+ cancel_work_sync(&atmel_port->work_tx);

/*
* Ensure everything is stopped and
@@ -3014,8 +3015,8 @@ static void atmel_serial_remove(struct platform_device *pdev)
struct uart_port *port = platform_get_drvdata(pdev);
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);

- tasklet_kill(&atmel_port->tasklet_rx);
- tasklet_kill(&atmel_port->tasklet_tx);
+ cancel_work_sync(&atmel_port->tasklet_rx);
+ cancel_work_sync(&atmel_port->tasklet_tx);

device_init_wakeup(&pdev->dev, 0);

diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
index 4bc89a9b380a..7ac55e35e061 100644
--- a/drivers/tty/serial/timbuart.c
+++ b/drivers/tty/serial/timbuart.c
@@ -18,12 +18,13 @@
#include <linux/ioport.h>
#include <linux/slab.h>
#include <linux/module.h>
+#include <linux/workqueue.h>

#include "timbuart.h"

struct timbuart_port {
struct uart_port port;
- struct tasklet_struct tasklet;
+ struct work_struct work;
int usedma;
u32 last_ier;
struct platform_device *dev;
@@ -55,8 +56,8 @@ static void timbuart_start_tx(struct uart_port *port)
struct timbuart_port *uart =
container_of(port, struct timbuart_port, port);

- /* do not transfer anything here -> fire off the tasklet */
- tasklet_schedule(&uart->tasklet);
+ /* do not transfer anything here -> fire off the task */
+ queue_work(system_bh_wq, &uart->work);
}

static unsigned int timbuart_tx_empty(struct uart_port *port)
@@ -169,9 +170,9 @@ static void timbuart_handle_rx_port(struct uart_port *port, u32 isr, u32 *ier)
dev_dbg(port->dev, "%s - leaving\n", __func__);
}

-static void timbuart_tasklet(struct tasklet_struct *t)
+static void timbuart_work(struct work_struct *t)
{
- struct timbuart_port *uart = from_tasklet(uart, t, tasklet);
+ struct timbuart_port *uart = from_work(uart, t, work);
u32 isr, ier = 0;

uart_port_lock(&uart->port);
@@ -350,11 +351,11 @@ static irqreturn_t timbuart_handleinterrupt(int irq, void *devid)
if (ioread8(uart->port.membase + TIMBUART_IPR)) {
uart->last_ier = ioread32(uart->port.membase + TIMBUART_IER);

- /* disable interrupts, the tasklet enables them again */
+ /* disable interrupts, the task enables them again */
iowrite32(0, uart->port.membase + TIMBUART_IER);

/* fire off bottom half */
- tasklet_schedule(&uart->tasklet);
+ queue_work(system_bh_wq, &uart->work);

return IRQ_HANDLED;
} else
@@ -448,7 +449,7 @@ static int timbuart_probe(struct platform_device *dev)
}
uart->port.irq = irq;

- tasklet_setup(&uart->tasklet, timbuart_tasklet);
+ INIT_WORK(&uart->work, timbuart_work);

err = uart_register_driver(&timbuart_driver);
if (err)
@@ -477,7 +478,7 @@ static void timbuart_remove(struct platform_device *dev)
{
struct timbuart_port *uart = platform_get_drvdata(dev);

- tasklet_kill(&uart->tasklet);
+ cancel_work_sync(&uart->work);
uart_remove_one_port(&timbuart_driver, &uart->port);
uart_unregister_driver(&timbuart_driver);
kfree(uart);
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index a2116e135a82..81943bed2063 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -46,6 +46,7 @@
#include <linux/tty.h>
#include <linux/uaccess.h>
#include <linux/vt_kern.h>
+#include <linux/workqueue.h>

#include <asm/irq_regs.h>

@@ -131,8 +132,8 @@ static const unsigned char max_vals[] = {

static const int NR_TYPES = ARRAY_SIZE(max_vals);

-static void kbd_bh(struct tasklet_struct *unused);
-static DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh);
+static void kbd_bh(struct work_struct *unused);
+static DECLARE_WORK(keyboard_work, kbd_bh);

static struct input_handler kbd_handler;
static DEFINE_SPINLOCK(kbd_event_lock);
@@ -379,7 +380,7 @@ static void to_utf8(struct vc_data *vc, uint c)
/* FIXME: review locking for vt.c callers */
static void set_leds(void)
{
- tasklet_schedule(&keyboard_tasklet);
+ queue_work(system_bh_wq, &keyboard_work);
}

/*
@@ -1031,12 +1032,12 @@ static int kbd_led_trigger_activate(struct led_classdev *cdev)
struct kbd_led_trigger *trigger =
container_of(cdev->trigger, struct kbd_led_trigger, trigger);

- tasklet_disable(&keyboard_tasklet);
+ disable_work_sync(&keyboard_work);
if (ledstate != -1U)
led_trigger_event(&trigger->trigger,
ledstate & trigger->mask ?
LED_FULL : LED_OFF);
- tasklet_enable(&keyboard_tasklet);
+ enable_and_queue_work(system_bh_wq, &keyboard_work);

return 0;
}
@@ -1247,12 +1248,13 @@ void vt_kbd_con_stop(unsigned int console)
}

/*
- * This is the tasklet that updates LED state of LEDs using standard
- * keyboard triggers. The reason we use tasklet is that we need to
- * handle the scenario when keyboard handler is not registered yet
- * but we already getting updates from the VT to update led state.
+ * This is the task that updates LED state of LEDs using standard
+ * keyboard triggers. The reason we use workqueue is that we need
+ * to handle the scenario when keyboard handler is not registered
+ * yet but we already getting updates from the VT to update led
+ * state.
*/
-static void kbd_bh(struct tasklet_struct *unused)
+static void kbd_bh(struct work_struct *unused)
{
unsigned int leds;
unsigned long flags;
@@ -1544,7 +1546,7 @@ static void kbd_event(struct input_handle *handle, unsigned int event_type,

spin_unlock(&kbd_event_lock);

- tasklet_schedule(&keyboard_tasklet);
+ queue_work(system_bh_wq, &keyboard_work);
do_poke_blanked_console = 1;
schedule_console_callback();
}
@@ -1616,12 +1618,12 @@ static void kbd_disconnect(struct input_handle *handle)
*/
static void kbd_start(struct input_handle *handle)
{
- tasklet_disable(&keyboard_tasklet);
+ disable_work_sync(&keyboard_work);

if (ledstate != -1U)
kbd_update_leds_helper(handle, &ledstate);

- tasklet_enable(&keyboard_tasklet);
+ enable_and_queue_work(system_bh_wq, &keyboard_work);
}

static const struct input_device_id kbd_ids[] = {
@@ -1671,8 +1673,8 @@ int __init kbd_init(void)
if (error)
return error;

- tasklet_enable(&keyboard_tasklet);
- tasklet_schedule(&keyboard_tasklet);
+ enable_and_queue_work(system_bh_wq, &keyboard_work);
+ queue_work(system_bh_wq, &keyboard_work);

return 0;
}
@@ -2233,7 +2235,7 @@ void vt_reset_keyboard(unsigned int console)
kb->ledmode = LED_SHOW_FLAGS;
kb->ledflagstate = kb->default_ledflagstate;
spin_unlock(&led_lock);
- /* do not do set_leds here because this causes an endless tasklet loop
+ /* do not do set_leds here because this causes an endless loop
when the keyboard hasn't been initialized yet */
spin_unlock_irqrestore(&kbd_event_lock, flags);
}
--
2.17.1



2024-04-04 06:14:03

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] tty: Convert from tasklet to BH workqueue

On 03. 04. 24, 18:15, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/tty/* from tasklet to BH workqueue.
>
> Based on the work done by Tejun Heo <[email protected]>
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais <[email protected]>
> ---
> drivers/tty/ipwireless/hardware.c | 15 +++---
> drivers/tty/serial/atmel_serial.c | 79 ++++++++++++++++---------------
> drivers/tty/serial/timbuart.c | 19 ++++----
> drivers/tty/vt/keyboard.c | 34 ++++++-------

Could you CC maintainers of the drivers?

--
js
suse labs


2024-04-04 06:18:35

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] tty: Convert from tasklet to BH workqueue

On 03. 04. 24, 18:15, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/tty/* from tasklet to BH workqueue.

Quickly looking into the changes, could you also elaborate why not to
convert most (all?) of them to (non-BH) wq? Or threaded IRQs. Much of
the code comes from the pre-WQ era.

thanks,
--
js
suse labs


2024-04-09 16:31:10

by Allen

[permalink] [raw]
Subject: Re: [PATCH] tty: Convert from tasklet to BH workqueue

> > The only generic interface to execute asynchronously in the BH context is
> > tasklet; however, it's marked deprecated and has some design flaws. To
> > replace tasklets, BH workqueue support was recently added. A BH workqueue
> > behaves similarly to regular workqueues except that the queued work items
> > are executed in the BH context.
> >
> > This patch converts drivers/tty/* from tasklet to BH workqueue.
>
> Quickly looking into the changes, could you also elaborate why not to
> convert most (all?) of them to (non-BH) wq? Or threaded IRQs. Much of
> the code comes from the pre-WQ era.
>

You are right. The idea I had was to convert every instance of tasklet to
bh workqueues in the kernel. I will send out a v2, which will use
threaded IRQ's.

Thanks.