[PATCH 0/3] firewire: core: serialize topology building and bus manager work

[PATCH 0/3] firewire: core: serialize topology building and bus manager work

[Takashi Sakamoto]

Hi,

Two functions, fw_core_handle_bus_reset() and bm_work(), acquire fw_card
spin lock, however each purpose is different.  The former function manages
to update some members of fw_card, and the latter function manages just to
access these members of fw_card. This reflects that the members are valid
during current bus generation once determined by the former function.

Current implementation schedules a work item for the latter function under
acquiring the spin lock in the former function. This could causes the
latter function to be stalled by spinning until the former function
finishes, depending on the timing to invoke the work item.

This patchset suppresses the stalling by serializing these two
functions. In former commits, the former function is invoked by IRQ
thread, thus sleep-able. The former function disables the work item
synchronously, then acquires the spin lock to update the members of
fw_card. After that, it releases the spin lock, then enable and schedule
the work item. The latter function is free from the spin lock.

Takashi Sakamoto (3):
  firewire: core: schedule bm_work item outside of spin lock
  firewire: core: disable bus management work temporarily during
    updating topology
  firewire: core: shrink critical section of fw_card spinlock in bm_work

 drivers/firewire/core-card.c     | 30 ++++++++----------------------
 drivers/firewire/core-topology.c | 11 ++++++++++-
 2 files changed, 18 insertions(+), 23 deletions(-)


base-commit: e0cda0dd12e08ecb8d26b8d78dc63e67e7069510
-- 
2.48.1

[PATCH 1/3] firewire: core: schedule bm_work item outside of spin lock

[Takashi Sakamoto]

Before (re)building topology tree, fw_core_handle_bus_reset() schedules
a work item under acquiring fw_card spin lock. The work item invokes
bm_work() which acquires the spin lock at first, then can be stalled to
wait until the building tree finishes. This is inconvenient.

This commit moves the timing to schedule the work item after releasing
the spin lock.

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

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 8fa0772ee723..2f73bcd5696f 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -479,7 +479,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 		card->reset_jiffies = get_jiffies_64();
 		card->bm_node_id  = 0xffff;
 		card->bm_abdicate = bm_abdicate;
-		fw_schedule_bm_work(card, 0);
 
 		local_node = build_tree(card, self_ids, self_id_count, generation);
 
@@ -496,6 +495,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 		}
 	}
 
+	fw_schedule_bm_work(card, 0);
+
 	// Just used by transaction layer.
 	scoped_guard(spinlock, &card->topology_map.lock) {
 		update_topology_map(card->topology_map.buffer, sizeof(card->topology_map.buffer),
-- 
2.48.1

[PATCH 2/3] firewire: core: disable bus management work temporarily during updating topology

[Takashi Sakamoto]

When processing selfID sequence, bus topology tree is (re)built, and some
members of fw_card are determined. Once determined, the members are valid
during the bus generation. The above operations are in the critical
section of fw_card spin lock.

Before building the bus topology, a work item is scheduled for bus manager
work. The bm_work() function is invoked by the work item. The function
tries to acquire the spin lock, then can be stalled until the bus topology
building finishes.

The bus manager should work once the members of fw_card are determined.
This commit suppresses the above situation by disabling the work item
during processing selfID sequence.

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

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 2f73bcd5696f..90b988035a2a 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -460,8 +460,14 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 {
 	struct fw_node *local_node;
 
+	might_sleep();
+
 	trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count);
 
+	// Disable bus management work during updating the cache of bus topology, since the work
+	// accesses to some members of fw_card.
+	disable_delayed_work_sync(&card->bm_work);
+
 	scoped_guard(spinlock, &card->lock) {
 		// If the selfID buffer is not the immediate successor of the
 		// previously processed one, we cannot reliably compare the
@@ -495,6 +501,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
 		}
 	}
 
+	enable_delayed_work(&card->bm_work);
+
 	fw_schedule_bm_work(card, 0);
 
 	// Just used by transaction layer.
-- 
2.48.1

[PATCH 3/3] firewire: core: shrink critical section of fw_card spinlock in bm_work

[Takashi Sakamoto]

Now fw_core_handle_bus_reset() and bm_work() are serialized. Some members
of fw_card are free to access in bm_work()

This commit shrinks critical section of fw_card spinlock in bm_work()

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 96495392a1f6..4fcd5ce4b2ce 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -293,12 +293,8 @@ static void bm_work(struct work_struct *work)
 	int expected_gap_count, generation, grace;
 	bool do_reset = false;
 
-	spin_lock_irq(&card->lock);
-
-	if (card->local_node == NULL) {
-		spin_unlock_irq(&card->lock);
+	if (card->local_node == NULL)
 		return;
-	}
 
 	generation = card->generation;
 
@@ -354,8 +350,6 @@ static void bm_work(struct work_struct *work)
 			goto pick_me;
 		}
 
-		spin_unlock_irq(&card->lock);
-
 		rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
 				irm_id, generation, SCODE_100,
 				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
@@ -365,21 +359,20 @@ static void bm_work(struct work_struct *work)
 		if (rcode == RCODE_GENERATION)
 			return;
 
-		spin_lock_irq(&card->lock);
-
 		if (rcode == RCODE_COMPLETE) {
 			int bm_id = be32_to_cpu(data[0]);
 
 			if (generation == card->generation) {
-				if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
-					card->bm_node_id = 0xffc0 & bm_id;
-				else
-					card->bm_node_id = local_id;
+				// Used by cdev layer for "struct fw_cdev_event_bus_reset".
+				scoped_guard(spinlock, &card->lock) {
+					if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED)
+						card->bm_node_id = 0xffc0 & bm_id;
+					else
+						card->bm_node_id = local_id;
+				}
 			}
 
 			if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) {
-				spin_unlock_irq(&card->lock);
-
 				// Somebody else is BM.  Only act as IRM.
 				if (local_id == irm_id)
 					allocate_broadcast_channel(card, generation);
@@ -393,7 +386,6 @@ static void bm_work(struct work_struct *work)
 			 * some local problem.  Let's try again later and hope
 			 * that the problem has gone away by then.
 			 */
-			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		}
@@ -415,7 +407,6 @@ static void bm_work(struct work_struct *work)
 		 * We weren't BM in the last generation, and the last
 		 * bus reset is less than 125ms ago.  Reschedule this job.
 		 */
-		spin_unlock_irq(&card->lock);
 		fw_schedule_bm_work(card, msecs_to_jiffies(125));
 		return;
 	}
@@ -458,7 +449,6 @@ static void bm_work(struct work_struct *work)
 			if (!root_device_is_running) {
 				// If we haven't probed this device yet, bail out now
 				// and let's try again once that's done.
-				spin_unlock_irq(&card->lock);
 				return;
 			} else if (root_device->cmc) {
 				// We will send out a force root packet for this
@@ -495,8 +485,6 @@ static void bm_work(struct work_struct *work)
 	if (do_reset) {
 		int card_gap_count = card->gap_count;
 
-		spin_unlock_irq(&card->lock);
-
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
 			  new_root_id, expected_gap_count);
 		fw_send_phy_config(card, new_root_id, generation, expected_gap_count);
@@ -517,8 +505,6 @@ static void bm_work(struct work_struct *work)
 	} else {
 		struct fw_device *root_device = fw_node_get_device(root_node);
 
-		spin_unlock_irq(&card->lock);
-
 		if (root_device && root_device->cmc) {
 			// Make sure that the cycle master sends cycle start packets.
 			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
-- 
2.48.1

Re: [PATCH 0/3] firewire: core: serialize topology building and bus manager work

[Takashi Sakamoto]

On Wed, Sep 17, 2025 at 09:03:44AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> Two functions, fw_core_handle_bus_reset() and bm_work(), acquire fw_card
> spin lock, however each purpose is different.  The former function manages
> to update some members of fw_card, and the latter function manages just to
> access these members of fw_card. This reflects that the members are valid
> during current bus generation once determined by the former function.
> 
> Current implementation schedules a work item for the latter function under
> acquiring the spin lock in the former function. This could causes the
> latter function to be stalled by spinning until the former function
> finishes, depending on the timing to invoke the work item.
> 
> This patchset suppresses the stalling by serializing these two
> functions. In former commits, the former function is invoked by IRQ
> thread, thus sleep-able. The former function disables the work item
> synchronously, then acquires the spin lock to update the members of
> fw_card. After that, it releases the spin lock, then enable and schedule
> the work item. The latter function is free from the spin lock.
> 
> Takashi Sakamoto (3):
>   firewire: core: schedule bm_work item outside of spin lock
>   firewire: core: disable bus management work temporarily during
>     updating topology
>   firewire: core: shrink critical section of fw_card spinlock in bm_work
> 
>  drivers/firewire/core-card.c     | 30 ++++++++----------------------
>  drivers/firewire/core-topology.c | 11 ++++++++++-
>  2 files changed, 18 insertions(+), 23 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto