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

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

[Takashi Sakamoto]

Hi,

This series is take 2 of the previous one[1].

The current implementation invokes both packet and transaction callbacks
from a workqueue when they complete normally[2][3]. 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.

Changes from v1:
* Use prefixed local variable in the issued macro

[1] [PATCH 0/5] firewire: core/ohci: defer cancellation callbacks to workqueue
https://lore.kernel.org/lkml/20261005112544.240552-1-o-takashi@sakamocchi.jp/
[2] [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/
[3] [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 | 101 ++++++++++++++--------------
 drivers/firewire/ohci.c             |  57 ++++++++++++----
 2 files changed, 93 insertions(+), 65 deletions(-)


base-commit: a6e7c3836b81235df08814bd409f7a23efcfb34d
-- 
2.53.0

[PATCH v2 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.

In the find_and_pop_transaction_entry() macro, a local variable provides
the evaluation result, however transmit_complete_callback() uses the same
local variable for another purpose. Add an underscore prefix to the one
in the macro to avoid the unexpected result.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index b4fccabb8fb2..37c61c4b7d20 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -87,36 +87,18 @@ void fw_cancel_pending_transactions(struct fw_card *card)
 // card->transactions.lock must be acquired in advance.
 #define find_and_pop_transaction_entry(card, condition)			\
 ({									\
-	struct fw_transaction *iter, *t = NULL;				\
+	struct fw_transaction *iter, *__t = NULL;			\
 	list_for_each_entry(iter, &card->transactions.list, link) {	\
 		if (condition) {					\
-			t = iter;					\
+			__t = iter;					\
 			break;						\
 		}							\
 	}								\
-	if (t && try_cancel_split_timeout(t))				\
-		remove_transaction_entry(card, t);			\
-	t;								\
+	if (__t && try_cancel_split_timeout(__t))			\
+		remove_transaction_entry(card, __t);			\
+	__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

[PATCH v2 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 37c61c4b7d20..cfd7d682c595 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 v2 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 v2 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 cfd7d682c595..05c54887b4a9 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 v2 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 05c54887b4a9..a0379fc3e60e 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

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

[Takashi Sakamoto]

On Wed, Oct 07, 2026 at 06:32:30PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This series is take 2 of the previous one[1].
> 
> The current implementation invokes both packet and transaction callbacks
> from a workqueue when they complete normally[2][3]. 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.
> 
> Changes from v1:
> * Use prefixed local variable in the issued macro
> 
> [1] [PATCH 0/5] firewire: core/ohci: defer cancellation callbacks to workqueue
> https://lore.kernel.org/lkml/20261005112544.240552-1-o-takashi@sakamocchi.jp/
> [2] [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/
> [3] [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 | 101 ++++++++++++++--------------
>  drivers/firewire/ohci.c             |  57 ++++++++++++----
>  2 files changed, 93 insertions(+), 65 deletions(-)

Applied fo for-next branch.


Regards

Takashi Sakamoto