Re: [PATCH] firewire: core: invoke packet callback from workqueue when removing card instance

[Takashi Sakamoto]

I found that some issues are reported by Sashiko.dev[1].

On Fri, Oct 16, 2026 at 10:59:48PM +0900, Takashi Sakamoto wrote:
> Current implementation switches the underlying driver of a card instance
> to the dummy driver in the kernel stack when removing the card. The dummy
> driver simply invokes callbacks when sending request and response
> packets.
> 
> Recent changes for the 1394 OHCI driver and core transaction layer ensure
> that callbacks are invoked in process context from a workqueue. This
> guarantees a consistent execution context for callbacks. However, card
> removel is currently an exception.
> 
> Invoke packet callbacks from a workqueue during card removal.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> ---
>  drivers/firewire/core-card.c | 57 +++++++++++++++++++++++++++++++-----
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
> index be6f18d67ece..a26b5e380c6c 100644
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -688,6 +688,15 @@ EXPORT_SYMBOL(fw_card_add);
>   * .set_config_rom() is never called since the card is taken out of card_list
>   * before switching to the dummy driver.
>   */
> +struct fw_card_driver_closer {
> +	struct fw_card_driver driver;
> +
> +	// For closing operations.
> +	struct fw_card *card;
> +	struct list_head packet_list;
> +	spinlock_t packet_list_lock;
> +	struct work_struct work;
> +};
>  
>  static int dummy_read_phy_reg(struct fw_card *card, int address)
>  {
> @@ -700,14 +709,39 @@ static int dummy_update_phy_reg(struct fw_card *card, int address,
>  	return -ENODEV;
>  }
>  
> +static void dummy_packet_callback_work(struct work_struct *work)
> +{
> +	struct fw_card_driver_closer *closer = container_of(work, typeof(*closer), work);
> +	struct fw_packet *p;
> +
> +	spin_lock(&closer->packet_list_lock);
> +	while ((p = list_first_entry_or_null(&closer->packet_list, typeof(*p), link_for_local))) {
> +		list_del(&p->link_for_local);
> +		spin_unlock(&closer->packet_list_lock);
> +
> +		p->callback(p, closer->card, RCODE_CANCELLED);
> +
> +		spin_lock(&closer->packet_list_lock);
> +	}
> +	spin_unlock(&closer->packet_list_lock);
> +}

They should be 'spinlock_irqsave' since the packet_list_lock is acquired
by dummy_send_request(), which can run in interrupt context.

Additionally, an issue has been reported that the packet cancelling and
the transaction cancelling by fw_cancel_pending_transactions() would conflict, since
the fw_packet structure is the part of the fw_transaction structure.

>  static void dummy_send_request(struct fw_card *card, struct fw_packet *packet)
>  {
> -	packet->callback(packet, card, RCODE_CANCELLED);
> +	struct fw_card_driver_closer *closer = container_of(card->driver, typeof(*closer), driver);
> +
> +	scoped_guard(spinlock_irqsave, &closer->packet_list_lock)
> +		list_add_tail(&packet->link_for_local, &closer->packet_list);
> +	queue_work(card->async_wq, &closer->work);
>  }
>  
>  static void dummy_send_response(struct fw_card *card, struct fw_packet *packet)
>  {
> -	packet->callback(packet, card, RCODE_CANCELLED);
> +	struct fw_card_driver_closer *closer = container_of(card->driver, typeof(*closer), driver);
> +
> +	scoped_guard(spinlock_irqsave, &closer->packet_list_lock)
> +		list_add_tail(&packet->link_for_local, &closer->packet_list);
> +	queue_work(card->async_wq, &closer->work);
>  }
>  
>  static int dummy_cancel_packet(struct fw_card *card, struct fw_packet *packet)
> @@ -789,7 +823,7 @@ EXPORT_SYMBOL_GPL(fw_card_release);
>  
>  void fw_core_remove_card(struct fw_card *card)
>  {
> -	struct fw_card_driver dummy_driver = dummy_driver_template;
> +	struct fw_card_driver_closer closer;
>  
>  	might_sleep();
>  
> @@ -800,11 +834,17 @@ void fw_core_remove_card(struct fw_card *card)
>  	scoped_guard(mutex, &card_mutex)
>  		list_del_init(&card->link);
>  
> -	/* Switch off most of the card driver interface. */
> -	dummy_driver.free_iso_context	= card->driver->free_iso_context;
> -	dummy_driver.stop_iso		= card->driver->stop_iso;
> -	dummy_driver.disable		= card->driver->disable;
> -	card->driver = &dummy_driver;
> +	closer.card = card;
> +	INIT_LIST_HEAD(&closer.packet_list);
> +	spin_lock_init(&closer.packet_list_lock);
> +	INIT_WORK(&closer.work, dummy_packet_callback_work);
> +
> +	// Switch off most of the card driver interface.
> +	closer.driver = dummy_driver_template;
> +	closer.driver.free_iso_context	= card->driver->free_iso_context;
> +	closer.driver.stop_iso = card->driver->stop_iso;
> +	closer.driver.disable = card->driver->disable;
> +	card->driver = &closer.driver;

There is a concern about the reordering. When the assigned pointer
value of card->driver is visible in any processor core, there is no
guarantee that the content of fw_card_driver_closer structure is filled
as expected.

Here any memory barrier is required. It is a potential problem that
the original code has.

>  	drain_workqueue(card->isoc_wq);
>  	drain_workqueue(card->async_wq);
> @@ -821,6 +861,7 @@ void fw_core_remove_card(struct fw_card *card)
>  	destroy_workqueue(card->isoc_wq);
>  	destroy_workqueue(card->async_wq);
>  
> +	WARN_ON(!list_empty(&closer.packet_list));
>  	WARN_ON(!list_empty(&card->transactions.list));
>  }
>  EXPORT_SYMBOL(fw_core_remove_card);

Although we are nearing the end of the current release cycle, let me put
in some more work on them.


[1] https://sashiko.dev/#/patchset/20261016135948.85343-1-o-takashi%40sakamocchi.jp


Regards

Takashi Sakamoto