[PATCH 0/5] firewire: ohci: refactor local AT request/response handling

[PATCH 0/5] firewire: ohci: refactor local AT request/response handling

[Takashi Sakamoto]

This series refactors the handling of asynchronous transactions to the local
node in the OHCI driver.

The current implementation has several special cases for transactions to
the two CSR address ranges on the local node. The code for detecting these
cases and generating the corresponding response packets is spread across
conditional and switch statements, making the handling difficult to follow.

This series first factors out the detection of the CSR address ranges and
local AT request/response packets. It then simplifies the response handling
and separates the request and response paths into different helper functions.

No functional changes are intended.


Takashi Sakamoto (5):
  firewire: ohci: use in_range() macro to detect CSR address ranges
  firewire: ohci: add helper function to detect local AT
    request/response packets
  firewire: ohci: refactor multiple calls to fw_fill_response()
  firewire: ohci: refactor local response packet handling
  firewire: ohci: refactor handling of local AT request/response packets

 drivers/firewire/ohci.c | 137 +++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 59 deletions(-)


base-commit: b5da1bc987ce48b61cd623c7821f1e7172b0e597
-- 
2.53.0

[PATCH 1/5] firewire: ohci: use in_range() macro to detect CSR address ranges

[Takashi Sakamoto]

The current implementation handles asynchronous packets sent to the two
CSR address ranges on the local node specially. To decide whether a packet
is destined for either range, the destination offset needs to be checked
according to the IEEE 1394 and 1394 OHCI specifications.

The current implementation uses both conditional and switch statements to
check the offset, which makes the range checks difficult to reuse.

Add two helper functions for the range checks using the in_range() macro.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 7d797755bbd6..c8cb7bdcb4ed 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1534,35 +1534,38 @@ static void handle_local_lock(struct fw_ohci *ohci,
 	fw_core_handle_response(&ohci->card, &response);
 }
 
+static bool in_config_rom_csr_registers(u64 offset)
+{
+	return in_range(offset, CSR_CONFIG_ROM, CONFIG_ROM_SIZE);
+}
+
+// 5.5.1 Bus Management CSR Registers.
+static bool in_bus_management_csr_registers(u64 offset)
+{
+	// 0x22c = CSR_MAINT_CONTROL following to CSR_CHANNELS_AVAILABLE_LO.
+	return in_range(offset, CSR_BUS_MANAGER_ID, 0x22c - CSR_BUS_MANAGER_ID);
+}
+
 static void handle_local_request(struct at_context *ctx, struct fw_packet *packet)
 {
 	struct fw_ohci *ohci = ctx->context.ohci;
-	u64 offset, csr;
 
 	if (ctx == &ohci->at_request_ctx) {
 		packet->ack = ACK_PENDING;
 		packet->callback(packet, &ohci->card, packet->ack);
 	}
 
-	offset = async_header_get_offset(packet->header);
-	csr = offset - CSR_REGISTER_BASE;
+	u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE;
 
-	/* Handle config rom reads. */
-	if (csr >= CSR_CONFIG_ROM && csr < CSR_CONFIG_ROM_END)
-		handle_local_rom(ohci, packet, csr);
-	else switch (csr) {
-	case CSR_BUS_MANAGER_ID:
-	case CSR_BANDWIDTH_AVAILABLE:
-	case CSR_CHANNELS_AVAILABLE_HI:
-	case CSR_CHANNELS_AVAILABLE_LO:
-		handle_local_lock(ohci, packet, csr);
-		break;
-	default:
+	if (in_config_rom_csr_registers(csr_offset)) {
+		handle_local_rom(ohci, packet, csr_offset);
+	} else if (in_bus_management_csr_registers(csr_offset)) {
+		handle_local_lock(ohci, packet, csr_offset);
+	} else {
 		if (ctx == &ohci->at_request_ctx)
 			fw_core_handle_request(&ohci->card, packet);
 		else
 			fw_core_handle_response(&ohci->card, packet);
-		break;
 	}
 
 	if (ctx == &ohci->at_response_ctx) {
-- 
2.53.0

[PATCH 2/5] firewire: ohci: add helper function to detect local AT request/response packets

[Takashi Sakamoto]

The current implementation handles asynchronous packets sent to the local
node specially. To decide whether a packet is destined for the local node,
the local node ID and bus generation need to be checked while holding the
spinlock, since both can change.

Add a helper function annotated to require the spinlock to be held.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index c8cb7bdcb4ed..290bac5112b7 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1574,6 +1574,15 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe
 	}
 }
 
+static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci)
+__must_hold(&ohci->lock)
+{
+	lockdep_assert_held(&ohci->lock);
+
+	return (async_header_get_destination(packet->header) == ohci->node_id &&
+		ohci->generation == packet->generation);
+}
+
 static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet)
 {
 	struct fw_ohci *ohci = ctx->context.ohci;
@@ -1582,8 +1591,7 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 
 	spin_lock_irqsave(&ohci->lock, flags);
 
-	if (async_header_get_destination(packet->header) == ohci->node_id &&
-	    ohci->generation == packet->generation) {
+	if (destination_is_local(packet, ohci)) {
 		spin_unlock_irqrestore(&ohci->lock, flags);
 
 		// Timestamping on behalf of the hardware.
-- 
2.53.0

[PATCH 3/5] firewire: ohci: refactor multiple calls to fw_fill_response()

[Takashi Sakamoto]

The handle_local_rom() function calls fw_fill_response() in each
conditional branch. Using local variables for the function parameters
allows the function to be called from a single place.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 290bac5112b7..ec069657ea8b 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1459,7 +1459,8 @@ static void handle_local_rom(struct fw_ohci *ohci,
 			     struct fw_packet *packet, u32 csr)
 {
 	struct fw_packet response;
-	int tcode, length, i;
+	int tcode, rcode, length, i;
+	void *payload = NULL;
 
 	tcode = async_header_get_tcode(packet->header);
 	if (tcode_is_block_packet(tcode))
@@ -1469,16 +1470,18 @@ static void handle_local_rom(struct fw_ohci *ohci,
 
 	i = csr - CSR_CONFIG_ROM;
 	if (i + length > CONFIG_ROM_SIZE) {
-		fw_fill_response(&response, packet->header,
-				 RCODE_ADDRESS_ERROR, NULL, 0);
+		rcode = RCODE_ADDRESS_ERROR;
+		length = 0;
 	} else if (!tcode_is_read_request(tcode)) {
-		fw_fill_response(&response, packet->header,
-				 RCODE_TYPE_ERROR, NULL, 0);
+		rcode = RCODE_TYPE_ERROR;
+		length = 0;
 	} else {
-		fw_fill_response(&response, packet->header, RCODE_COMPLETE,
-				 (void *) ohci->config_rom + i, length);
+		rcode = RCODE_COMPLETE;
+		payload = (u8 *)ohci->config_rom + i;
 	}
 
+	fw_fill_response(&response, packet->header, rcode, payload, length);
+
 	// Timestamping on behalf of the hardware.
 	response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
 	fw_core_handle_response(&ohci->card, &response);
-- 
2.53.0

[PATCH 4/5] firewire: ohci: refactor local response packet handling

[Takashi Sakamoto]

For an asynchronous transaction to one of the two CSR address ranges on
the local node, the current implementation generates the packet data for
the response subaction internally while handling the request subaction.
The handling code is duplicated between the two ranges.

Reduce the duplication by moving response handling to the callers.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index ec069657ea8b..8a0aedf50a92 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1455,10 +1455,9 @@ static int handle_at_packet(struct context *context,
 
 static u32 get_cycle_time(struct fw_ohci *ohci);
 
-static void handle_local_rom(struct fw_ohci *ohci,
-			     struct fw_packet *packet, u32 csr)
+static void handle_local_rom(struct fw_ohci *ohci, struct fw_packet *packet, u32 csr,
+			     struct fw_packet *response)
 {
-	struct fw_packet response;
 	int tcode, rcode, length, i;
 	void *payload = NULL;
 
@@ -1480,17 +1479,12 @@ static void handle_local_rom(struct fw_ohci *ohci,
 		payload = (u8 *)ohci->config_rom + i;
 	}
 
-	fw_fill_response(&response, packet->header, rcode, payload, length);
-
-	// Timestamping on behalf of the hardware.
-	response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
-	fw_core_handle_response(&ohci->card, &response);
+	fw_fill_response(response, packet->header, rcode, payload, length);
 }
 
-static void handle_local_lock(struct fw_ohci *ohci,
-			      struct fw_packet *packet, u32 csr)
+static void handle_local_lock(struct fw_ohci *ohci, struct fw_packet *packet, u32 csr,
+			      struct fw_packet *response)
 {
-	struct fw_packet response;
 	int tcode, length, ext_tcode, sel, try;
 	__be32 *payload, lock_old;
 	u32 lock_arg, lock_data;
@@ -1508,9 +1502,8 @@ static void handle_local_lock(struct fw_ohci *ohci,
 		lock_arg = 0;
 		lock_data = 0;
 	} else {
-		fw_fill_response(&response, packet->header,
-				 RCODE_TYPE_ERROR, NULL, 0);
-		goto out;
+		fw_fill_response(response, packet->header, RCODE_TYPE_ERROR, NULL, 0);
+		return;
 	}
 
 	sel = (csr - CSR_BUS_MANAGER_ID) / 4;
@@ -1522,19 +1515,13 @@ static void handle_local_lock(struct fw_ohci *ohci,
 		if (reg_read(ohci, OHCI1394_CSRControl) & 0x80000000) {
 			lock_old = cpu_to_be32(reg_read(ohci,
 							OHCI1394_CSRData));
-			fw_fill_response(&response, packet->header,
-					 RCODE_COMPLETE,
-					 &lock_old, sizeof(lock_old));
-			goto out;
+			fw_fill_response(response, packet->header, RCODE_COMPLETE, &lock_old,
+					 sizeof(lock_old));
+			return;
 		}
 
 	ohci_err(ohci, "swap not done (CSR lock timeout)\n");
-	fw_fill_response(&response, packet->header, RCODE_BUSY, NULL, 0);
-
- out:
-	// Timestamping on behalf of the hardware.
-	response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
-	fw_core_handle_response(&ohci->card, &response);
+	fw_fill_response(response, packet->header, RCODE_BUSY, NULL, 0);
 }
 
 static bool in_config_rom_csr_registers(u64 offset)
@@ -1560,10 +1547,21 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe
 
 	u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE;
 
-	if (in_config_rom_csr_registers(csr_offset)) {
-		handle_local_rom(ohci, packet, csr_offset);
-	} else if (in_bus_management_csr_registers(csr_offset)) {
-		handle_local_lock(ohci, packet, csr_offset);
+	if (in_config_rom_csr_registers(csr_offset) || in_bus_management_csr_registers(csr_offset)) {
+		struct fw_packet response;
+
+		memset(&response, 0, sizeof(response));
+
+		if (in_bus_management_csr_registers(csr_offset))
+			handle_local_lock(ohci, packet, csr_offset, &response);
+		else
+			handle_local_rom(ohci, packet, csr_offset, &response);
+
+		// Timestamping on behalf of the hardware.
+		response.timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
+
+		// Finish the transaction immediately.
+		fw_core_handle_response(&ohci->card, &response);
 	} else {
 		if (ctx == &ohci->at_request_ctx)
 			fw_core_handle_request(&ohci->card, packet);
-- 
2.53.0

[PATCH 5/5] firewire: ohci: refactor handling of local AT request/response packets

[Takashi Sakamoto]

For an asynchronous transaction to one of the two CSR address ranges on
the local node, the current implementation generates the packet data for
the response subaction internally while handling the request subaction.
Unlike transactions to other addresses, there is no need to submit the new
packet for the response subaction.

The current implementation handles both subactions in a single helper
function with conditional statements. This makes it difficult to see why
the two subactions should be handled differently.

Split the helper function into two helper functions, one for the request
subaction and the other for the response subaction.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 8a0aedf50a92..bd3e01b2f450 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1536,14 +1536,11 @@ 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_request(struct at_context *ctx, struct fw_packet *packet)
+static void handle_local_at_request_packet(struct fw_ohci *ohci, struct fw_packet *packet)
 {
-	struct fw_ohci *ohci = ctx->context.ohci;
-
-	if (ctx == &ohci->at_request_ctx) {
-		packet->ack = ACK_PENDING;
-		packet->callback(packet, &ohci->card, packet->ack);
-	}
+	// Emulate split transaction.
+	packet->ack = ACK_PENDING;
+	packet->callback(packet, &ohci->card, packet->ack);
 
 	u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE;
 
@@ -1563,16 +1560,23 @@ static void handle_local_request(struct at_context *ctx, struct fw_packet *packe
 		// Finish the transaction immediately.
 		fw_core_handle_response(&ohci->card, &response);
 	} else {
-		if (ctx == &ohci->at_request_ctx)
-			fw_core_handle_request(&ohci->card, packet);
-		else
-			fw_core_handle_response(&ohci->card, packet);
+		fw_core_handle_request(&ohci->card, packet);
 	}
+}
 
-	if (ctx == &ohci->at_response_ctx) {
-		packet->ack = ACK_COMPLETE;
-		packet->callback(packet, &ohci->card, packet->ack);
-	}
+static void handle_local_at_response_packet(struct fw_ohci *ohci, struct fw_packet *packet)
+{
+	u64 csr_offset = async_header_get_offset(packet->header) - CSR_REGISTER_BASE;
+
+	// The transaction is already finished by handle_local_at_request_packet().
+	if (WARN_ON(in_config_rom_csr_registers(csr_offset)) ||
+	    WARN_ON(in_bus_management_csr_registers(csr_offset)))
+		return;
+
+	fw_core_handle_response(&ohci->card, packet);
+
+	packet->ack = ACK_COMPLETE;
+	packet->callback(packet, &ohci->card, packet->ack);
 }
 
 static bool destination_is_local(const struct fw_packet *packet, const struct fw_ohci *ohci)
@@ -1598,7 +1602,10 @@ static void at_context_transmit(struct at_context *ctx, struct fw_packet *packet
 		// Timestamping on behalf of the hardware.
 		packet->timestamp = cycle_time_to_ohci_tstamp(get_cycle_time(ohci));
 
-		handle_local_request(ctx, packet);
+		if (ctx == &ohci->at_request_ctx)
+			handle_local_at_request_packet(ohci, packet);
+		else
+			handle_local_at_response_packet(ohci, packet);
 		return;
 	}
 
-- 
2.53.0

Re: [PATCH 0/5] firewire: ohci: refactor local AT request/response handling

[Takashi Sakamoto]

On Sun, Sep 13, 2026 at 07:13:51PM +0900, Takashi Sakamoto wrote:
> This series refactors the handling of asynchronous transactions to the local
> node in the OHCI driver.
> 
> The current implementation has several special cases for transactions to
> the two CSR address ranges on the local node. The code for detecting these
> cases and generating the corresponding response packets is spread across
> conditional and switch statements, making the handling difficult to follow.
> 
> This series first factors out the detection of the CSR address ranges and
> local AT request/response packets. It then simplifies the response handling
> and separates the request and response paths into different helper functions.
> 
> No functional changes are intended.
> 
> 
> Takashi Sakamoto (5):
>   firewire: ohci: use in_range() macro to detect CSR address ranges
>   firewire: ohci: add helper function to detect local AT
>     request/response packets
>   firewire: ohci: refactor multiple calls to fw_fill_response()
>   firewire: ohci: refactor local response packet handling
>   firewire: ohci: refactor handling of local AT request/response packets
> 
>  drivers/firewire/ohci.c | 137 +++++++++++++++++++++++-----------------
>  1 file changed, 78 insertions(+), 59 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto