[PATCH 0/5] firewire: core/ohci: defer cancellation callbacks to workqueue

[PATCH 0/5] firewire: core/ohci: defer cancellation callbacks to workqueue

[Takashi Sakamoto]

Hi,

The current implementation invokes both packet and transaction callbacks
from a workqueue when they complete normally[1][2]. In contrast, the
cancellation paths invoke the callbacks synchronously while cancelling
them.

Although the cancellation paths are guaranteed to run in process context,
invoking the callbacks synchronously is undesirable because it makes
callback execution depends on the context in which cancellation is
executed.

This series defers the callbacks to a workqueue when cancelling the
packets and transactions, so that callbacks are invoked from a workqueue
in both normal completion and cancellation paths.

This also allows local AT packets to be cancelled. Local packets currently
use a separate path and cannot be cancelled once queued. Make them use
the same cancellation mechanism as remote packets.

[1] [PATCH 0/5] firewire: ohci: invoke all AT packet callbacks from a workqueue
https://lore.kernel.org/lkml/20260919115017.859169-1-o-takashi@sakamocchi.jp/
[2] [PATCH 0/3] firewire: core: invoke transaction callbacks in process context
https://lore.kernel.org/lkml/20260921031931.287429-1-o-takashi@sakamocchi.jp/

Takashi Sakamoto (5):
  firewire: core: expand close_transaction() into its callers and remove
    it
  firewire: ohci: schedule callback on workqueue when cancelling AT
    request packet
  firewire: ohci: allow AT local packet cancellation
  firewire: core: schedule callback when cancelling pending transactions
  firewire: core: fulfill kerneldoc for fw_cancel_transaction()

 drivers/firewire/core-transaction.c | 91 ++++++++++++++---------------
 drivers/firewire/ohci.c             | 57 +++++++++++++-----
 2 files changed, 88 insertions(+), 60 deletions(-)


base-commit: a6e7c3836b81235df08814bd409f7a23efcfb34d
-- 
2.53.0

[PATCH 1/5] firewire: core: expand close_transaction() into its callers and remove it

[Takashi Sakamoto]

The return value from close_transaction() is useful only in
fw_cancel_transaction(). The other caller,
transmit_complete_callback(), simply discards it.

Additionally, the pending transaction queue should be enumerated as soon
as possible to prevent the issued transaction from completing.

Expand close_transaction() into its callers, and remove it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 66 ++++++++++++++---------------
 1 file changed, 31 insertions(+), 35 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index b4fccabb8fb2..35f3a11ab2bb 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -99,24 +99,6 @@ void fw_cancel_pending_transactions(struct fw_card *card)
 	t;								\
 })
 
-static int close_transaction(struct fw_transaction *transaction, struct fw_card *card, int rcode,
-			     u32 response_tstamp)
-{
-	struct fw_transaction *t;
-
-	// NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
-	// local destination never runs in any type of IRQ context.
-	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
-		t = find_and_pop_transaction_entry(card, iter == transaction);
-		if (!t)
-			return -ENOENT;
-	}
-
-	invoke_callback(t, rcode, response_tstamp, NULL, 0);
-
-	return 0;
-}
-
 /*
  * Only valid for transactions that are potentially pending (ie have
  * been sent).
@@ -135,17 +117,25 @@ int fw_cancel_transaction(struct fw_card *card,
 	if (card->driver->cancel_packet(card, &transaction->packet) == 0)
 		return 0;
 
+	// If the request packet has already been sent, we need to see if the transaction is still
+	// pending and remove it in that case (e.g. the split transaction).
+	//
+	// NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+	// local destination never runs in any type of IRQ context.
+	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
+		if (!find_and_pop_transaction_entry(card, iter == transaction))
+			return -ENOENT;
+	}
+
 	u32 curr_cycle_time = 0;
 
 	// Timestamping on behalf of hardware.
 	(void)fw_card_read_cycle_time(card, &curr_cycle_time);
 	tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time);
 
-	/*
-	 * If the request packet has already been sent, we need to see
-	 * if the transaction is still pending and remove it in that case.
-	 */
-	return close_transaction(transaction, card, RCODE_CANCELLED, tstamp);
+	invoke_callback(transaction, RCODE_CANCELLED, tstamp, NULL, 0);
+
+	return 0;
 }
 EXPORT_SYMBOL(fw_cancel_transaction);
 
@@ -201,9 +191,6 @@ static void transmit_complete_callback(struct fw_packet *packet,
 					      packet->speed, status, packet->timestamp);
 
 	switch (status) {
-	case ACK_COMPLETE:
-		close_transaction(t, card, RCODE_COMPLETE, packet->timestamp);
-		break;
 	case ACK_PENDING:
 	{
 		unsigned int delta;
@@ -218,27 +205,36 @@ static void transmit_complete_callback(struct fw_packet *packet,
 		// local destination never runs in any type of IRQ context.
 		scoped_guard(spinlock_irqsave, &card->transactions.lock)
 			start_split_transaction_timeout(t, delta);
-		break;
+		return;
 	}
+	case ACK_COMPLETE:
+		status = RCODE_COMPLETE;
+		break;
 	case ACK_BUSY_X:
 	case ACK_BUSY_A:
 	case ACK_BUSY_B:
-		close_transaction(t, card, RCODE_BUSY, packet->timestamp);
+		status = RCODE_BUSY;
 		break;
 	case ACK_DATA_ERROR:
-		close_transaction(t, card, RCODE_DATA_ERROR, packet->timestamp);
+		status = RCODE_DATA_ERROR;
 		break;
 	case ACK_TYPE_ERROR:
-		close_transaction(t, card, RCODE_TYPE_ERROR, packet->timestamp);
+		status = RCODE_TYPE_ERROR;
 		break;
 	default:
-		/*
-		 * In this case the ack is really a juju specific
-		 * rcode, so just forward that to the callback.
-		 */
-		close_transaction(t, card, status, packet->timestamp);
+		// In this case the ack is really a juju specific rcode, so just forward that to
+		// the callback.
 		break;
 	}
+
+	// NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+	// local destination never runs in any type of IRQ context.
+	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
+		if (!find_and_pop_transaction_entry(card, iter == t))
+			return;
+	}
+
+	invoke_callback(t, status, packet->timestamp, NULL, 0);
 }
 
 static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel,
-- 
2.53.0

Re: [PATCH 1/5] firewire: core: expand close_transaction() into its callers and remove it

[Takashi Sakamoto]

On Mon, Oct 05, 2026 at 08:25:40PM +0900, Takashi Sakamoto wrote:
> The return value from close_transaction() is useful only in
> fw_cancel_transaction(). The other caller,
> transmit_complete_callback(), simply discards it.
> 
> Additionally, the pending transaction queue should be enumerated as soon
> as possible to prevent the issued transaction from completing.
> 
> Expand close_transaction() into its callers, and remove it.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  drivers/firewire/core-transaction.c | 66 ++++++++++++++---------------
>  1 file changed, 31 insertions(+), 35 deletions(-)
> ... 
> @@ -201,9 +191,6 @@ static void transmit_complete_callback(struct fw_packet *packet,
>  					      packet->speed, status, packet->timestamp);
>  
>  	switch (status) {
> -	case ACK_COMPLETE:
> -		close_transaction(t, card, RCODE_COMPLETE, packet->timestamp);
> -		break;
>  	case ACK_PENDING:
>  	{
>  		unsigned int delta;
> @@ -218,27 +205,36 @@ static void transmit_complete_callback(struct fw_packet *packet,
>  		// local destination never runs in any type of IRQ context.
>  		scoped_guard(spinlock_irqsave, &card->transactions.lock)
>  			start_split_transaction_timeout(t, delta);
> -		break;
> +		return;
>  	}
> +	case ACK_COMPLETE:
> +		status = RCODE_COMPLETE;
> +		break;
>  	case ACK_BUSY_X:
>  	case ACK_BUSY_A:
>  	case ACK_BUSY_B:
> -		close_transaction(t, card, RCODE_BUSY, packet->timestamp);
> +		status = RCODE_BUSY;
>  		break;
>  	case ACK_DATA_ERROR:
> -		close_transaction(t, card, RCODE_DATA_ERROR, packet->timestamp);
> +		status = RCODE_DATA_ERROR;
>  		break;
>  	case ACK_TYPE_ERROR:
> -		close_transaction(t, card, RCODE_TYPE_ERROR, packet->timestamp);
> +		status = RCODE_TYPE_ERROR;
>  		break;
>  	default:
> -		/*
> -		 * In this case the ack is really a juju specific
> -		 * rcode, so just forward that to the callback.
> -		 */
> -		close_transaction(t, card, status, packet->timestamp);
> +		// In this case the ack is really a juju specific rcode, so just forward that to
> +		// the callback.
>  		break;
>  	}
> +
> +	// NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
> +	// local destination never runs in any type of IRQ context.
> +	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
> +		if (!find_and_pop_transaction_entry(card, iter == t))
> +			return;
> +	}

I realized that this expansion has a bug to suppress the following
callback, since the condition 'iter == t' includes the local variable 't' 
while 'find_and_pop_transaction_entry()' macro initiates the 't' with NULL
for its internal use. When receiving one of the above error acknowledge
codes (e.g. RCODE_GENERATION at bus reset),
'find_and_pop_transaction_entry()' is evaluated as NULL, then the above
return statement is executed always.

> +	invoke_callback(t, status, packet->timestamp, NULL, 0);
>  }
>  
>  static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel,

I'll post v2 patch after enough testing.


Regards

Takashi Sakamoto

[PATCH 2/5] firewire: ohci: schedule callback on workqueue when cancelling AT request packet

[Takashi Sakamoto]

The current implementation invokes the packet callback in the context
where the packet cancellation is requested. This is inconvenient because
the context can affect the packet callback.

Invoke the packet callback from a workqueue instead. Use the workqueue
for local AT packets.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c |  8 ++----
 drivers/firewire/ohci.c             | 43 ++++++++++++++++++++---------
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 35f3a11ab2bb..8c30a476f9fd 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -108,12 +108,8 @@ int fw_cancel_transaction(struct fw_card *card,
 {
 	u32 tstamp;
 
-	/*
-	 * Cancel the packet transmission if it's still queued.  That
-	 * will call the packet transmission callback which cancels
-	 * the transaction.
-	 */
-
+	// Cancel the packet transmission if it's still queued. That will schedule the packet
+	// transmission callback which cancels the transaction.
 	if (card->driver->cancel_packet(card, &transaction->packet) == 0)
 		return 0;
 
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index df53efec0b0a..896d0cb9a895 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1669,6 +1669,9 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 	struct fw_ohci *ohci = ctx->context.ohci;
 	bool use_work = true;
 
+	// These members are used to cancel the packet.
+	packet->driver_data = NULL;
+
 	scoped_guard(spinlock_irqsave, &ohci->lock) {
 		if (!destination_is_local(packet, ohci)) {
 			if (!at_context_queue_packet(ctx, packet))
@@ -2652,7 +2655,7 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
 {
 	struct fw_ohci *ohci = fw_ohci(card);
 	struct at_context *ctx = &ohci->at_request_ctx;
-	struct driver_data *driver_data = packet->driver_data;
+	struct at_local *local = &ohci->at_request_local;
 	int ret = -ENOENT;
 
 	// Avoid dead lock due to programming mistake.
@@ -2660,24 +2663,38 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
 		return 0;
 	disable_work_sync(&ctx->work);
 
-	if (packet->ack != 0)
-		goto out;
+	bool use_local_work = false;
 
-	if (packet->payload_mapped)
-		dma_unmap_single(ohci->card.device, packet->payload_bus,
-				 packet->payload_length, DMA_TO_DEVICE);
+	if (!packet->ack) {
+		// NOTE: This function relies on this value, thus it is not thread-safe.
+		struct driver_data *driver_data = packet->driver_data;
 
-	driver_data->packet = NULL;
-	packet->ack = RCODE_CANCELLED;
+		if (driver_data) {
+			if (packet->payload_mapped) {
+				dma_unmap_single(ohci->card.device, packet->payload_bus,
+						 packet->payload_length, DMA_TO_DEVICE);
+			}
 
-	// Timestamping on behalf of the hardware.
-	packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
+			// Invalidate the packet queued in AT request context.
+			driver_data->packet = NULL;
+
+			scoped_guard(spinlock_irqsave, &local->lock)
+				list_add_tail(&packet->link_for_local, &local->list);
+			use_local_work = true;
+		}
+	}
 
-	packet->callback(packet, &ohci->card, packet->ack);
-	ret = 0;
- out:
 	enable_and_queue_work(card->async_wq, &ctx->work);
 
+	if (use_local_work) {
+		// Timestamping on behalf of the hardware.
+		packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
+		packet->ack = RCODE_CANCELLED;
+		queue_work(card->async_wq, &local->work);
+
+		ret = 0;
+	}
+
 	return ret;
 }
 
-- 
2.53.0

[PATCH 3/5] firewire: ohci: allow AT local packet cancellation

[Takashi Sakamoto]

The current implementation uses a different path for local packets than
for remote packets, and local packets cannot be cancelled once they have
been queued.

Allow local packets to be cancelled.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 896d0cb9a895..c06e8d87239c 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1597,7 +1597,7 @@ static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci
 	spin_lock(&local->lock);
 
 	while ((packet = list_first_entry_or_null(&local->list, typeof(*packet), link_for_local))) {
-		list_del(&packet->link_for_local);
+		list_del_init(&packet->link_for_local);
 		spin_unlock(&local->lock);
 
 		if (unlikely(packet->ack != 0)) {
@@ -1671,6 +1671,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 
 	// These members are used to cancel the packet.
 	packet->driver_data = NULL;
+	INIT_LIST_HEAD(&packet->link_for_local);
 
 	scoped_guard(spinlock_irqsave, &ohci->lock) {
 		if (!destination_is_local(packet, ohci)) {
@@ -2658,6 +2659,10 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
 	struct at_local *local = &ohci->at_request_local;
 	int ret = -ENOENT;
 
+	// handle_at_request_local_packet() can reach here by calling fw_core_handle_response().
+	if (current_work() != &local->work)
+		disable_work_sync(&local->work);
+
 	// Avoid dead lock due to programming mistake.
 	if (WARN_ON_ONCE(current_work() == &ctx->work))
 		return 0;
@@ -2681,6 +2686,11 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
 			scoped_guard(spinlock_irqsave, &local->lock)
 				list_add_tail(&packet->link_for_local, &local->list);
 			use_local_work = true;
+		} else {
+			scoped_guard(spinlock_irqsave, &local->lock) {
+				if (!list_empty(&packet->link_for_local))
+					use_local_work = true;
+			}
 		}
 	}
 
@@ -2690,11 +2700,13 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet)
 		// Timestamping on behalf of the hardware.
 		packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
 		packet->ack = RCODE_CANCELLED;
-		queue_work(card->async_wq, &local->work);
 
 		ret = 0;
 	}
 
+	if (current_work() != &local->work)
+		enable_and_queue_work(card->async_wq, &local->work);
+
 	return ret;
 }
 
-- 
2.53.0

[PATCH 4/5] firewire: core: schedule callback when cancelling pending transactions

[Takashi Sakamoto]

The current implementation invokes the transaction callback in the
context where transaction cancellation is requested. This is
inconvenient because the context can affect the transaction callback.

Invoke the transaction callback from a workqueue instead. Use the
workqueue for error handling.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 8c30a476f9fd..90419760d8b4 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -106,8 +106,6 @@ void fw_cancel_pending_transactions(struct fw_card *card)
 int fw_cancel_transaction(struct fw_card *card,
 			  struct fw_transaction *transaction)
 {
-	u32 tstamp;
-
 	// Cancel the packet transmission if it's still queued. That will schedule the packet
 	// transmission callback which cancels the transaction.
 	if (card->driver->cancel_packet(card, &transaction->packet) == 0)
@@ -127,9 +125,10 @@ int fw_cancel_transaction(struct fw_card *card,
 
 	// Timestamping on behalf of hardware.
 	(void)fw_card_read_cycle_time(card, &curr_cycle_time);
-	tstamp = cycle_time_to_ohci_tstamp(curr_cycle_time);
+	transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time);
+	transaction->rcode = RCODE_CANCELLED;
 
-	invoke_callback(transaction, RCODE_CANCELLED, tstamp, NULL, 0);
+	enable_and_queue_work(card->async_wq, &transaction->error_work);
 
 	return 0;
 }
-- 
2.53.0

[PATCH 5/5] firewire: core: fulfill kerneldoc for fw_cancel_transaction()

[Takashi Sakamoto]

fw_cancel_transaction() has no kerneldoc even though it is exposed.

Add kerneldoc for it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 90419760d8b4..18a731eecae3 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -99,12 +99,20 @@ void fw_cancel_pending_transactions(struct fw_card *card)
 	t;								\
 })
 
-/*
- * Only valid for transactions that are potentially pending (ie have
- * been sent).
+/**
+ * fw_cancel_transaction: cancel transaction.
+ * @card:		interface where the transaction has been requested
+ * @transaction:	transaction instance to be cancelled
+ *
+ * Terminate the transaction by either canceling the request subaction or the response subaction
+ * of the split transaction, then schedule the transaction callback to notify the cancellation.
+ * The transaction instance should be kept until the callback will be invoked.
+ *
+ * Context: Process context.
+ * Returns: 0 on success, -ENOENT if the transaction instance is not found in the queue for the
+ * pending transaction.
  */
-int fw_cancel_transaction(struct fw_card *card,
-			  struct fw_transaction *transaction)
+int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction)
 {
 	// Cancel the packet transmission if it's still queued. That will schedule the packet
 	// transmission callback which cancels the transaction.
-- 
2.53.0