[PATCH 0/3] firewire: ohci: switch to threaded IRQ handler for SelfIDComplete event

[PATCH 0/3] firewire: ohci: switch to threaded IRQ handler for SelfIDComplete event

[Takashi Sakamoto]

Hi,

This patchset replaces the module-local workqueue with a threaded IRQ
handler for handling the SelfIDComplete event in the 1394 OHCI PCI driver.

The SelfIDComplete event is the first step in maintaining bus topology.
It occurs after a bus reset or when the topology changes, and must be
processed outside the hard IRQ context due to the latency involved in
enumerating the SelfID sequence. Historically, this was handled by a
module-local workqueue with the WQ_MEM_RECLAIM flag. A threaded IRQ
handler offers a cleaner and more reliable solution, leveraging the
kernel's common infrastructure and eliminating the need for maintaining
a custom workqueue.

Takashi Sakamoto (3):
  firewire: ohci: move self_id_complete tracepoint after validating
    register
  firewire: ohci: use threaded IRQ handler to handle SelfIDComplete
    event
  firewire: ohci: remove module-local workqueue

 drivers/firewire/ohci.c | 61 +++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)


base-commit: 8748368c3d92f7bdef67c90d3f62ab92083b3677
-- 
2.48.1

[PATCH 1/3] firewire: ohci: move self_id_complete tracepoint after validating register

[Takashi Sakamoto]

The value of OHCI1394_SelfIDCount register includes an error-indicating
bit. It is safer to place the tracepoint probe after validating the
register value.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index c8c5d598e3c8..b3a187e4cba7 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -1863,6 +1863,9 @@ static void bus_reset_work(struct work_struct *work)
 		ohci_notice(ohci, "self ID receive error\n");
 		return;
 	}
+
+	trace_self_id_complete(ohci->card.index, reg, ohci->self_id, has_be_header_quirk(ohci));
+
 	/*
 	 * The count in the SelfIDCount register is the number of
 	 * bytes in the self ID receive buffer.  Since we also receive
@@ -2024,15 +2027,8 @@ static irqreturn_t irq_handler(int irq, void *data)
 	if (event & OHCI1394_busReset)
 		reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset);
 
-	if (event & OHCI1394_selfIDComplete) {
-		if (trace_self_id_complete_enabled()) {
-			u32 reg = reg_read(ohci, OHCI1394_SelfIDCount);
-
-			trace_self_id_complete(ohci->card.index, reg, ohci->self_id,
-					       has_be_header_quirk(ohci));
-		}
+	if (event & OHCI1394_selfIDComplete)
 		queue_work(selfid_workqueue, &ohci->bus_reset_work);
-	}
 
 	if (event & OHCI1394_RQPkt)
 		queue_work(ohci->card.async_wq, &ohci->ar_request_ctx.work);
-- 
2.48.1

[PATCH 2/3] firewire: ohci: use threaded IRQ handler to handle SelfIDComplete event

[Takashi Sakamoto]

The first step  maintaining the bus topology is to handle SelfIDComplete
event. This event occurs after initiating bus reset when 1394 OHCI link
layer is enabled, or when the bus topology changes (e.g. when a device is
added). Because enumeration of the selfID sequence can take some time, it
should be processed in a bottom half.

Currently, this is done in a module-local workqueue with the
WQ_MEM_RECLAIM flag, to allow invocation during memory reclaim paths. A
threaded IRQ handler is a preferable alternative, as it eliminates the
need to manage workqueue attributes manually.

Although SelfIDComplete events are not so frequent in normal usage,
handling them correctly is critical for proper bus topology management.
This commit switches SelfIDComplete handling to a threaded IRQ handler.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index b3a187e4cba7..5b16286280e0 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -760,7 +760,7 @@ static __le32 *handle_ar_packet(struct ar_context *ctx, __le32 *buffer)
 	 *
 	 * Alas some chips sometimes emit bus reset packets with a
 	 * wrong generation.  We set the correct generation for these
-	 * at a slightly incorrect time (in bus_reset_work).
+	 * at a slightly incorrect time (in handle_selfid_complete_event).
 	 */
 	if (evt == OHCI1394_evt_bus_reset) {
 		if (!(ohci->quirks & QUIRK_RESET_PACKET))
@@ -1830,9 +1830,9 @@ static int find_and_insert_self_id(struct fw_ohci *ohci, int self_id_count)
 	return self_id_count;
 }
 
-static void bus_reset_work(struct work_struct *work)
+static irqreturn_t handle_selfid_complete_event(int irq, void *data)
 {
-	struct fw_ohci *ohci = from_work(ohci, work, bus_reset_work);
+	struct fw_ohci *ohci = data;
 	int self_id_count, generation, new_generation, i, j;
 	u32 reg, quadlet;
 	void *free_rom = NULL;
@@ -1843,11 +1843,11 @@ static void bus_reset_work(struct work_struct *work)
 	if (!(reg & OHCI1394_NodeID_idValid)) {
 		ohci_notice(ohci,
 			    "node ID not valid, new bus reset in progress\n");
-		return;
+		goto end;
 	}
 	if ((reg & OHCI1394_NodeID_nodeNumber) == 63) {
 		ohci_notice(ohci, "malconfigured bus\n");
-		return;
+		goto end;
 	}
 	ohci->node_id = reg & (OHCI1394_NodeID_busNumber |
 			       OHCI1394_NodeID_nodeNumber);
@@ -1861,7 +1861,7 @@ static void bus_reset_work(struct work_struct *work)
 	reg = reg_read(ohci, OHCI1394_SelfIDCount);
 	if (ohci1394_self_id_count_is_error(reg)) {
 		ohci_notice(ohci, "self ID receive error\n");
-		return;
+		goto end;
 	}
 
 	trace_self_id_complete(ohci->card.index, reg, ohci->self_id, has_be_header_quirk(ohci));
@@ -1876,7 +1876,7 @@ static void bus_reset_work(struct work_struct *work)
 
 	if (self_id_count > 252) {
 		ohci_notice(ohci, "bad selfIDSize (%08x)\n", reg);
-		return;
+		goto end;
 	}
 
 	quadlet = cond_le32_to_cpu(ohci->self_id[0], has_be_header_quirk(ohci));
@@ -1903,7 +1903,7 @@ static void bus_reset_work(struct work_struct *work)
 
 			ohci_notice(ohci, "bad self ID %d/%d (%08x != ~%08x)\n",
 				    j, self_id_count, id, id2);
-			return;
+			goto end;
 		}
 		ohci->self_id_buffer[j] = id;
 	}
@@ -1913,13 +1913,13 @@ static void bus_reset_work(struct work_struct *work)
 		if (self_id_count < 0) {
 			ohci_notice(ohci,
 				    "could not construct local self ID\n");
-			return;
+			goto end;
 		}
 	}
 
 	if (self_id_count == 0) {
 		ohci_notice(ohci, "no self IDs\n");
-		return;
+		goto end;
 	}
 	rmb();
 
@@ -1941,7 +1941,7 @@ static void bus_reset_work(struct work_struct *work)
 	new_generation = ohci1394_self_id_count_get_generation(reg);
 	if (new_generation != generation) {
 		ohci_notice(ohci, "new bus reset, discarding self ids\n");
-		return;
+		goto end;
 	}
 
 	// FIXME: Document how the locking works.
@@ -2002,6 +2002,8 @@ static void bus_reset_work(struct work_struct *work)
 				 self_id_count, ohci->self_id_buffer,
 				 ohci->csr_state_setclear_abdicate);
 	ohci->csr_state_setclear_abdicate = false;
+end:
+	return IRQ_HANDLED;
 }
 
 static irqreturn_t irq_handler(int irq, void *data)
@@ -2023,13 +2025,10 @@ static irqreturn_t irq_handler(int irq, void *data)
 		  event & ~(OHCI1394_busReset | OHCI1394_postedWriteErr));
 	trace_irqs(ohci->card.index, event);
 
-	// The flag is masked again at bus_reset_work() scheduled by selfID event.
+	// The flag is masked again at handle_selfid_complete_event() scheduled by selfID event.
 	if (event & OHCI1394_busReset)
 		reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset);
 
-	if (event & OHCI1394_selfIDComplete)
-		queue_work(selfid_workqueue, &ohci->bus_reset_work);
-
 	if (event & OHCI1394_RQPkt)
 		queue_work(ohci->card.async_wq, &ohci->ar_request_ctx.work);
 
@@ -2100,7 +2099,10 @@ static irqreturn_t irq_handler(int irq, void *data)
 	} else
 		flush_writes(ohci);
 
-	return IRQ_HANDLED;
+	if (event & OHCI1394_selfIDComplete)
+		return IRQ_WAKE_THREAD;
+	else
+		return IRQ_HANDLED;
 }
 
 static int software_reset(struct fw_ohci *ohci)
@@ -2413,7 +2415,7 @@ static int ohci_set_config_rom(struct fw_card *card,
 	 * then set up the real values for the two registers.
 	 *
 	 * We use ohci->lock to avoid racing with the code that sets
-	 * ohci->next_config_rom to NULL (see bus_reset_work).
+	 * ohci->next_config_rom to NULL (see handle_selfid_complete_event).
 	 */
 
 	next_config_rom = dmam_alloc_coherent(ohci->card.device, CONFIG_ROM_SIZE,
@@ -3620,7 +3622,9 @@ static int pci_probe(struct pci_dev *dev,
 		goto fail_msi;
 	}
 
-	err = request_threaded_irq(irq, irq_handler, NULL,
+	// IRQF_ONESHOT is not applied so that any events are handled in the hardIRQ handler during
+	// invoking the threaded IRQ handler for SelfIDComplete event.
+	err = request_threaded_irq(irq, irq_handler, handle_selfid_complete_event,
 				   pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED, ohci_driver_name,
 				   ohci);
 	if (err < 0) {
-- 
2.48.1

[PATCH 3/3] firewire: ohci: remove module-local workqueue

[Takashi Sakamoto]

Now module-local workqueue has been replaced by a threaded IRQ handler.

This commit removes the workqueue and the associated work item
accordingly.

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 5b16286280e0..40851b120615 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -228,13 +228,10 @@ struct fw_ohci {
 
 	__le32    *self_id;
 	dma_addr_t self_id_bus;
-	struct work_struct bus_reset_work;
 
 	u32 self_id_buffer[512];
 };
 
-static struct workqueue_struct *selfid_workqueue;
-
 static inline struct fw_ohci *fw_ohci(struct fw_card *card)
 {
 	return container_of(card, struct fw_ohci, card);
@@ -3512,8 +3509,6 @@ static int pci_probe(struct pci_dev *dev,
 	spin_lock_init(&ohci->lock);
 	mutex_init(&ohci->phy_reg_mutex);
 
-	INIT_WORK(&ohci->bus_reset_work, bus_reset_work);
-
 	if (!(pci_resource_flags(dev, 0) & IORESOURCE_MEM) ||
 	    pci_resource_len(dev, 0) < OHCI1394_REGISTER_SIZE) {
 		ohci_err(ohci, "invalid MMIO resource\n");
@@ -3668,7 +3663,6 @@ static void pci_remove(struct pci_dev *dev)
 		reg_write(ohci, OHCI1394_IntMaskClear, ~0);
 		flush_writes(ohci);
 	}
-	cancel_work_sync(&ohci->bus_reset_work);
 	fw_core_remove_card(&ohci->card);
 
 	/*
@@ -3741,17 +3735,12 @@ static struct pci_driver fw_ohci_pci_driver = {
 
 static int __init fw_ohci_init(void)
 {
-	selfid_workqueue = alloc_workqueue(KBUILD_MODNAME, WQ_MEM_RECLAIM, 0);
-	if (!selfid_workqueue)
-		return -ENOMEM;
-
 	return pci_register_driver(&fw_ohci_pci_driver);
 }
 
 static void __exit fw_ohci_cleanup(void)
 {
 	pci_unregister_driver(&fw_ohci_pci_driver);
-	destroy_workqueue(selfid_workqueue);
 }
 
 module_init(fw_ohci_init);
-- 
2.48.1

Re: [PATCH 0/3] firewire: ohci: switch to threaded IRQ handler for SelfIDComplete event

[Takashi Sakamoto]

On Sat, Aug 23, 2025 at 12:09:51PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This patchset replaces the module-local workqueue with a threaded IRQ
> handler for handling the SelfIDComplete event in the 1394 OHCI PCI driver.
> 
> The SelfIDComplete event is the first step in maintaining bus topology.
> It occurs after a bus reset or when the topology changes, and must be
> processed outside the hard IRQ context due to the latency involved in
> enumerating the SelfID sequence. Historically, this was handled by a
> module-local workqueue with the WQ_MEM_RECLAIM flag. A threaded IRQ
> handler offers a cleaner and more reliable solution, leveraging the
> kernel's common infrastructure and eliminating the need for maintaining
> a custom workqueue.
> 
> Takashi Sakamoto (3):
>   firewire: ohci: move self_id_complete tracepoint after validating
>     register
>   firewire: ohci: use threaded IRQ handler to handle SelfIDComplete
>     event
>   firewire: ohci: remove module-local workqueue
> 
>  drivers/firewire/ohci.c | 61 +++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto