Re: [PATCH v2 1/3] gpio: mpsse: use rcu to ensure worker is torn down

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Tue, Sep 23, 2025 at 09:33:02AM -0400, Mary Strodl wrote:
> @@ -261,9 +273,8 @@ static int gpio_mpsse_direction_input(struct gpio_chip *chip,
>  
>  	guard(mutex)(&priv->io_mutex);
>  	priv->gpio_dir[bank] &= ~BIT(bank_offset);
> -	gpio_mpsse_set_bank(priv, bank);
>  
> -	return 0;
> +	return gpio_mpsse_set_bank(priv, bank);

The change looks irrelevant to the patch.

> +static void gpio_mpsse_poll(struct work_struct *my_work)
>  {
[...]
> +	/*
> +	 * We only want one worker. Workers race to acquire irq_race and tear
> +	 * down all other workers. This is a cond guard so that we don't deadlock
> +	 * trying to cancel a worker.
> +	 */
> +	scoped_cond_guard(mutex_try, ;, &priv->irq_race) {
> +		scoped_guard(rcu) {
> +			list_for_each_entry_rcu(worker, &priv->workers, list) {

I'm not sure: doesn't it need to use list_for_each_entry_safe() (or variants)
as elements may be removed in the loop?

> +				/* Don't stop ourselves */
> +				if (worker == my_worker)
> +					continue;
> +
> +				scoped_guard(raw_spinlock_irqsave, &priv->irq_spin)
> +					list_del_rcu(&worker->list);

If RCU is using, does it still need to acquire the spinlock?  Alternatively,
could it use the spinlock to protect the list so that it doesn't need RCU at
all?

>  static void gpio_mpsse_irq_disable(struct irq_data *irqd)
>  {
> +	struct mpsse_worker *worker;
>  	struct mpsse_priv *priv = irq_data_get_irq_chip_data(irqd);
>  
>  	atomic_and(~BIT(irqd->hwirq), &priv->irq_enabled);
>  	gpiochip_disable_irq(&priv->gpio, irqd->hwirq);
> +
> +	/* Can't actually do teardown in IRQ context (blocks...) */
> +	scoped_guard(rcu)
> +		list_for_each_entry_rcu(worker, &priv->workers, list)
> +			atomic_set(&worker->cancelled, 1);
>  }
>  
>  static void gpio_mpsse_irq_enable(struct irq_data *irqd)
>  {
> +	struct mpsse_worker *worker;
>  	struct mpsse_priv *priv = irq_data_get_irq_chip_data(irqd);
>  
>  	gpiochip_enable_irq(&priv->gpio, irqd->hwirq);
>  	/* If no-one else was using the IRQ, enable it */
>  	if (!atomic_fetch_or(BIT(irqd->hwirq), &priv->irq_enabled)) {
> -		INIT_WORK(&priv->irq_work, gpio_mpsse_poll);
> -		schedule_work(&priv->irq_work);
> +		/*
> +		 * Can't be devm because it uses a non-raw spinlock (illegal in
> +		 * this context, where a raw spinlock is held by our caller)
> +		 */
> +		worker = kzalloc(sizeof(*worker), GFP_KERNEL);

I'm not sure: however it seems this function may be in IRQ context too (as
gpio_mpsse_irq_disable() does).  GFP_KERNEL can sleep.

> +		if (!worker)
> +			return;
> +
> +		worker->priv = priv;
> +		INIT_LIST_HEAD(&worker->list);
> +		INIT_WORK(&worker->work, gpio_mpsse_poll);
> +		schedule_work(&worker->work);
> +
> +		scoped_guard(raw_spinlock_irqsave, &priv->irq_spin)
> +			list_add_rcu(&worker->list, &priv->workers);

Doesn't it need a synchronize_rcu()?

>  static void gpio_mpsse_disconnect(struct usb_interface *intf)
>  {
> +	struct mpsse_worker *worker;
>  	struct mpsse_priv *priv = usb_get_intfdata(intf);
> +	struct list_head destructors = LIST_HEAD_INIT(destructors);
> +
> +	/*
> +	 * Lock prevents double-free of worker from here and the teardown
> +	 * step at the beginning of gpio_mpsse_poll
> +	 */
> +	scoped_guard(mutex, &priv->irq_race) {
> +		scoped_guard(rcu) {
> +			list_for_each_entry_rcu(worker, &priv->workers, list) {
> +				scoped_guard(raw_spinlock_irqsave, &priv->irq_spin)
> +					list_del_rcu(&worker->list);
> +
> +				/* Give worker a chance to terminate itself */
> +				atomic_set(&worker->cancelled, 1);
> +				/* Keep track of stuff to cancel */
> +				INIT_LIST_HEAD(&worker->destroy);
> +				list_add(&worker->destroy, &destructors);
> +			}
> +		}
> +		/* Make sure list consumers are finished before we tear down */
> +		synchronize_rcu();
> +		list_for_each_entry(worker, &destructors, destroy)
> +			gpio_mpsse_stop(worker);
> +	}

The code block is very similar to block in gpio_mpsse_poll() above.  Could
consider to use a function to prevent duplicate code.