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

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

[Takashi Sakamoto]

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@sakamocchi.jp>
---
 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);
+}
+
 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;
 
 	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);

base-commit: 82d59eb7f5431771cfc6fb074b74f466cd4e3353
-- 
2.53.0

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@sakamocchi.jp>
> ---
>  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