[PATCH 0/5] firewire: ohci: invoke all AT packet callbacks from a workqueue

[PATCH 0/5] firewire: ohci: invoke all AT packet callbacks from a workqueue

[Takashi Sakamoto]

Hi,

For local-to-local asynchronous transactions, a shortcut is implemented to
handle the relevant packets in the request initiator's context instead of
queueing them to the hardware. This may seem beneficial since the
transaction completes immediately within the software stack.

However, this has the drawback that the context in which the callback
handler is invoked is unspecified, as it runs in the initiator's context.
This makes it difficult to implement the callback handlers, especially
address handlers.

This patchset addresses this issue by using a workqueue. All packet
callbacks are now invoked in process context.

Takashi Sakamoto (5):
  firewire: ohci: use workqueue to handle error case of AT
    request/response packet queueing
  firewire: ohci: use workqueue to handle local AT request/response
    packets
  firewire: ohci: refactor branches in at_context_transmit()
  firewire: core: use spinlock without irqsave for card split_timeout
    member
  firewire: core: use spinlock without irqsave for card topology_map
    member

 drivers/firewire/core-card.c        |   7 +-
 drivers/firewire/core-transaction.c |  22 ++---
 drivers/firewire/ohci.c             | 119 ++++++++++++++++++++++------
 include/linux/firewire.h            |   5 +-
 4 files changed, 107 insertions(+), 46 deletions(-)


base-commit: a20545fbf9226591017fc371bdfb1bfd490b0e0b
-- 
2.53.0

[PATCH 1/5] firewire: ohci: use workqueue to handle error case of AT request/response packet queueing

[Takashi Sakamoto]

When a packet cannot be queued to an AT context, the packet callback is
invoked to report the error to the caller. Since the callback runs in the
caller's context, its execution context depends on where the packet was
submitted. This makes the callback context inconsistent between
successful and failed subactions.

Use a workqueue to handle the error cases.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c |  7 +--
 drivers/firewire/ohci.c      | 90 +++++++++++++++++++++++++++++++++---
 include/linux/firewire.h     |  3 ++
 3 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 23749434d900..be6f18d67ece 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -643,11 +643,12 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
 	//  * == WQ_FREEZABLE		The target device would not be available when being freezed.
 	//  * == WQ_HIGHPRI		High priority to process semi-realtime timestamped data.
 	//  * == WQ_SYSFS		Parameters are available via sysfs.
-	//  * max_active == 4		A hardIRQ could notify events for a pair of requests and
-	//				response AR/AT contexts.
+	//  * max_active == 4 + 2	A hardIRQ could notify events for a pair of requests and
+	//				response AR/AT contexts. Additional 2 capacity are for the
+	//				internal handling of local AT request and response packets.
 	async_wq = alloc_workqueue("firewire-async-card%u",
 				   WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS,
-				   4, card->index);
+				   6, card->index);
 	if (!async_wq)
 		return -ENOMEM;
 
diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index bd3e01b2f450..45f03095a196 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -164,6 +164,13 @@ struct at_context {
 	struct work_struct work;
 };
 
+// The local-to-local transaction is handled by the work item in the following structure.
+struct at_local {
+	struct list_head list;
+	spinlock_t lock;
+	struct work_struct work;
+};
+
 struct iso_context {
 	struct fw_iso_context base;
 	struct context context;
@@ -216,6 +223,9 @@ struct fw_ohci {
 	struct at_context at_request_ctx;
 	struct at_context at_response_ctx;
 
+	struct at_local at_request_local;
+	struct at_local at_response_local;
+
 	u32 it_context_support;
 	u32 it_context_mask;     /* unoccupied IT contexts */
 	struct iso_context *it_context_list;
@@ -1579,6 +1589,51 @@ static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_pack
 	packet->callback(packet, &ohci->card, packet->ack);
 }
 
+static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci)
+{
+	struct fw_packet *packet;
+
+	spin_lock(&local->lock);
+
+	while ((packet = list_first_entry_or_null(&local->list, typeof(*packet), link_for_local))) {
+		list_del(&packet->link_for_local);
+		spin_unlock(&local->lock);
+
+		if (unlikely(packet->ack != 0)) {
+			// This case is active when the call of at_context_queue_packet() returns
+			// error in at_context_transmit().
+			packet->callback(packet, &ohci->card, packet->ack);
+		}
+
+		spin_lock(&local->lock);
+	}
+
+	spin_unlock(&local->lock);
+}
+
+static void at_request_local_work(struct work_struct *work)
+{
+	struct at_local *local = from_work(local, work, work);
+	struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_request_local);
+
+	handle_at_local_packets(local, ohci);
+}
+
+static void at_response_local_work(struct work_struct *work)
+{
+	struct at_local *local = from_work(local, work, work);
+	struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_response_local);
+
+	handle_at_local_packets(local, ohci);
+}
+
+static void at_local_init(struct at_local *local, work_func_t func)
+{
+	spin_lock_init(&local->lock);
+	INIT_LIST_HEAD(&local->list);
+	INIT_WORK(&local->work, func);
+}
+
 static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci)
 __must_hold(&ohci->lock)
 {
@@ -1588,6 +1643,24 @@ __must_hold(&ohci->lock)
 		ohci->generation == packet->generation);
 }
 
+static void queue_work_for_at_local_packet(struct at_context *ctx, struct fw_packet *packet,
+					   struct fw_ohci *ohci)
+{
+	struct at_local *local;
+
+	if (ctx == &ohci->at_request_ctx)
+		local = &ohci->at_request_local;
+	else
+		local = &ohci->at_response_local;
+
+	// Timestamping on behalf of the hardware.
+	packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
+
+	scoped_guard(spinlock_irqsave, &local->lock)
+		list_add_tail(&packet->link_for_local, &local->list);
+	queue_work(ohci->card.async_wq, &local->work);
+}
+
 static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet)
 {
 	struct fw_ohci *ohci = ctx->context.ohci;
@@ -1612,12 +1685,8 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 	ret = at_context_queue_packet(ctx, packet);
 	spin_unlock_irqrestore(&ohci->lock, flags);
 
-	if (ret < 0) {
-		// Timestamping on behalf of the hardware.
-		packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
-
-		packet->callback(packet, &ohci->card, packet->ack);
-	}
+	if (ret < 0)
+		queue_work_for_at_local_packet(ctx, packet, ohci);
 }
 
 static void detect_dead_context(struct fw_ohci *ohci,
@@ -2474,6 +2543,9 @@ static void ohci_disable(struct fw_card *card)
 	flush_work(&ohci->at_request_ctx.work);
 	flush_work(&ohci->at_response_ctx.work);
 
+	flush_work(&ohci->at_request_local.work);
+	flush_work(&ohci->at_response_local.work);
+
 	for (i = 0; i < ohci->n_ir; ++i) {
 		if (!(ohci->ir_context_mask & BIT(i)))
 			flush_work(&ohci->ir_context_list[i].base.work);
@@ -2485,6 +2557,9 @@ static void ohci_disable(struct fw_card *card)
 
 	at_context_flush(&ohci->at_request_ctx);
 	at_context_flush(&ohci->at_response_ctx);
+
+	at_request_local_work(&ohci->at_request_local.work);
+	at_response_local_work(&ohci->at_response_local.work);
 }
 
 static int ohci_set_config_rom(struct fw_card *card,
@@ -3684,6 +3759,9 @@ static int pci_probe(struct pci_dev *dev,
 		return err;
 	INIT_WORK(&ohci->at_response_ctx.work, ohci_at_context_work);
 
+	at_local_init(&ohci->at_request_local, at_request_local_work);
+	at_local_init(&ohci->at_response_local, at_response_local_work);
+
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0);
 	ohci->ir_context_channels = ~0ULL;
 	ohci->ir_context_support = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet);
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index cbe59a18162e..1c71ff69c42f 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -329,6 +329,9 @@ struct fw_packet {
 	bool payload_mapped;
 	u32 timestamp;
 
+	// Used to handle the local-to-local packets in the AT request/response contexts.
+	struct list_head link_for_local;
+
 	/*
 	 * This callback is called when the packet transmission has completed.
 	 * For successful transmission, the status code is the ack received
-- 
2.53.0

[PATCH 2/5] firewire: ohci: use workqueue to handle local AT request/response packets

[Takashi Sakamoto]

Local-to-local asynchronous transactions are currently handled in the
initiator's context. This requires the request handlers to support any
context in which the initiator may run.

Use workqueue to handle the AT request and response packets targeting
local address offsets, so that the request handler always runs in process
context.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/ohci.c  | 21 +++++++++------------
 include/linux/firewire.h |  2 +-
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 45f03095a196..dddb08dbb45f 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1546,7 +1546,7 @@ static bool in_bus_management_csr_registers(u64 offset)
 	return in_range(offset, CSR_BUS_MANAGER_ID, 0x22c - CSR_BUS_MANAGER_ID);
 }
 
-static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packet *packet)
+static void handle_at_request_local_packet(struct fw_ohci *ohci, struct fw_packet *packet)
 {
 	// Emulate split transaction.
 	packet->ack = ACK_PENDING;
@@ -1574,7 +1574,7 @@ static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packe
 	}
 }
 
-static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_packet *packet)
+static void handle_at_response_local_packet(struct fw_ohci *ohci, struct fw_packet *packet)
 {
 	u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE;
 
@@ -1589,7 +1589,8 @@ static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_pack
 	packet->callback(packet, &ohci->card, packet->ack);
 }
 
-static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci)
+static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci,
+			void (*handle_at_local_packet)(struct fw_ohci *, struct fw_packet *))
 {
 	struct fw_packet *packet;
 
@@ -1603,6 +1604,8 @@ static void handle_at_local_packets(struct at_local *local, struct fw_ohci *ohci
 			// This case is active when the call of at_context_queue_packet() returns
 			// error in at_context_transmit().
 			packet->callback(packet, &ohci->card, packet->ack);
+		} else {
+			handle_at_local_packet(ohci, packet);
 		}
 
 		spin_lock(&local->lock);
@@ -1616,7 +1619,7 @@ static void at_request_local_work(struct work_struct *work)
 	struct at_local *local = from_work(local, work, work);
 	struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_request_local);
 
-	handle_at_local_packets(local, ohci);
+	handle_at_local_packets(local, ohci, handle_at_request_local_packet);
 }
 
 static void at_response_local_work(struct work_struct *work)
@@ -1624,7 +1627,7 @@ static void at_response_local_work(struct work_struct *work)
 	struct at_local *local = from_work(local, work, work);
 	struct fw_ohci *ohci = container_of(local, struct fw_ohci, at_response_local);
 
-	handle_at_local_packets(local, ohci);
+	handle_at_local_packets(local, ohci, handle_at_response_local_packet);
 }
 
 static void at_local_init(struct at_local *local, work_func_t func)
@@ -1672,13 +1675,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 	if (destination_is_local(packet, ohci)) {
 		spin_unlock_irqrestore(&ohci->lock, flags);
 
-		// Timestamping on behalf of the hardware.
-		packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
-
-		if (ctx == &ohci->at_request_ctx)
-			handle_local_at_request_packet(ohci, packet);
-		else
-			handle_local_at_response_packet(ohci, packet);
+		queue_work_for_at_local_packet(ctx, packet, ohci);
 		return;
 	}
 
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 1c71ff69c42f..2b065f03565d 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -337,7 +337,7 @@ struct fw_packet {
 	 * For successful transmission, the status code is the ack received
 	 * from the destination.  Otherwise it is one of the juju-specific
 	 * rcodes:  RCODE_SEND_ERROR, _CANCELLED, _BUSY, _GENERATION, _NO_ACK.
-	 * The callback can be called from workqueue and thus must never block.
+	 * The callback is called from a workqueue. It is not preferable to block it so long.
 	 */
 	fw_packet_callback_t callback;
 	int ack;
-- 
2.53.0

[PATCH 3/5] firewire: ohci: refactor branches in at_context_transmit()

[Takashi Sakamoto]

Refactor the branches to consolidate the handling of local requests and
responses, as well as hardware queueing errors.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index dddb08dbb45f..710640b759e4 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1667,22 +1667,16 @@ static void queue_work_for_at_local_packet(struct at_context *ctx, struct fw_pac
 static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet)
 {
 	struct fw_ohci *ohci = ctx->context.ohci;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&ohci->lock, flags);
+	bool use_work = true;
 
-	if (destination_is_local(packet, ohci)) {
-		spin_unlock_irqrestore(&ohci->lock, flags);
-
-		queue_work_for_at_local_packet(ctx, packet, ohci);
-		return;
+	scoped_guard(spinlock_irqsave, &ohci->lock) {
+		if (!destination_is_local(packet, ohci)) {
+			if (!at_context_queue_packet(ctx, packet))
+				use_work = false;
+		}
 	}
 
-	ret = at_context_queue_packet(ctx, packet);
-	spin_unlock_irqrestore(&ohci->lock, flags);
-
-	if (ret < 0)
+	if (use_work)
 		queue_work_for_at_local_packet(ctx, packet, ohci);
 }
 
-- 
2.53.0

[PATCH 4/5] firewire: core: use spinlock without irqsave for card split_timeout member

[Takashi Sakamoto]

Asynchronous packet callbacks are now always invoked in process context.
Therefore, the split_timeout member is not accessed from IRQ context.

Use spin_lock() for the split_timeout member since disabling local IRQs
is unnecessary.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 995c2001bee0..91ed85b7aa22 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -204,9 +204,7 @@ static void transmit_complete_callback(struct fw_packet *packet,
 	{
 		unsigned int delta;
 
-		// 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->split_timeout.lock) {
+		scoped_guard(spinlock, &card->split_timeout.lock) {
 			t->split_timeout_cycle =
 				compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff;
 			delta = card->split_timeout.jiffies;
@@ -900,9 +898,7 @@ static struct fw_request *allocate_request(struct fw_card *card,
 		return NULL;
 	kref_init(&request->kref);
 
-	// 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->split_timeout.lock)
+	scoped_guard(spinlock, &card->split_timeout.lock)
 		request->response.timestamp = compute_split_timeout_timestamp(card, p->timestamp);
 
 	request->response.speed = p->speed;
@@ -1340,10 +1336,7 @@ static void handle_registers(struct fw_card *card, struct fw_request *request,
 		if (tcode == TCODE_READ_QUADLET_REQUEST) {
 			*data = cpu_to_be32(card->split_timeout.hi);
 		} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
-			// 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->split_timeout.lock) {
+			scoped_guard(spinlock, &card->split_timeout.lock) {
 				card->split_timeout.hi = be32_to_cpu(*data) & 7;
 				update_split_timeout(card);
 			}
@@ -1356,10 +1349,7 @@ static void handle_registers(struct fw_card *card, struct fw_request *request,
 		if (tcode == TCODE_READ_QUADLET_REQUEST) {
 			*data = cpu_to_be32(card->split_timeout.lo);
 		} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
-			// 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->split_timeout.lock) {
+			scoped_guard(spinlock, &card->split_timeout.lock) {
 				card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000;
 				update_split_timeout(card);
 			}
-- 
2.53.0

[PATCH 5/5] firewire: core: use spinlock without irqsave for card topology_map member

[Takashi Sakamoto]

Asynchronous packet callbacks are now always invoked in process context.
Therefore, the topology_map member is not accessed from IRQ context.

Use spin_lock() for the topology_map member since disabling local IRQs is
unnecessary.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 91ed85b7aa22..a2a8d755ad0a 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -1256,9 +1256,7 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request
 
 	start = (offset - topology_map_region.start) / 4;
 
-	// 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->topology_map.lock)
+	scoped_guard(spinlock, &card->topology_map.lock)
 		memcpy(payload, &card->topology_map.buffer[start], length);
 
 	fw_send_response(card, request, RCODE_COMPLETE);
-- 
2.53.0

Re: [PATCH 0/5] firewire: ohci: invoke all AT packet callbacks from a workqueue

[Takashi Sakamoto]

On Sat, Sep 19, 2026 at 08:50:12PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> For local-to-local asynchronous transactions, a shortcut is implemented to
> handle the relevant packets in the request initiator's context instead of
> queueing them to the hardware. This may seem beneficial since the
> transaction completes immediately within the software stack.
> 
> However, this has the drawback that the context in which the callback
> handler is invoked is unspecified, as it runs in the initiator's context.
> This makes it difficult to implement the callback handlers, especially
> address handlers.
> 
> This patchset addresses this issue by using a workqueue. All packet
> callbacks are now invoked in process context.
> 
> Takashi Sakamoto (5):
>   firewire: ohci: use workqueue to handle error case of AT
>     request/response packet queueing
>   firewire: ohci: use workqueue to handle local AT request/response
>     packets
>   firewire: ohci: refactor branches in at_context_transmit()
>   firewire: core: use spinlock without irqsave for card split_timeout
>     member
>   firewire: core: use spinlock without irqsave for card topology_map
>     member
> 
>  drivers/firewire/core-card.c        |   7 +-
>  drivers/firewire/core-transaction.c |  22 ++---
>  drivers/firewire/ohci.c             | 119 ++++++++++++++++++++++------
>  include/linux/firewire.h            |   5 +-
>  4 files changed, 107 insertions(+), 46 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto