public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] firewire: core: code refactoring for work item of bus manager
@ 2025-09-18 23:08 Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 1/6] firewire: core: remove useless generation check Takashi Sakamoto
                   ` (12 more replies)
  0 siblings, 13 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Hi,

The implementation of bus manager work, bm_work() function, has many
lines for the procedure to contend for bus manager as well as to
transfer phy configuration packet for root node selection and gap count
optimization.

This patchset is to refactor the code for the procedure. A new helper
function is added to contend for bus manager. The outcome of contention
is evaluated as a return code of the helper function, then the root node
at next generation is decided. The phy configuration packet is
transferred and generate bus reset lastly.

Takashi Sakamoto (6):
  firewire: core: remove useless generation check
  firewire: core: use switch statement to evaluate transaction result to
    CSR_BUS_MANAGER_ID
  firewire: core: code refactoring for the case of generation mismatch
  firewire: core: code refactoring to split contention procedure for bus
    manager
  firewire: core; eliminate pick_me goto label
  firewire: core: minor code refactoring to delete useless local
    variable

 drivers/firewire/core-card.c | 336 +++++++++++++++++++----------------
 1 file changed, 178 insertions(+), 158 deletions(-)


base-commit: e6d2338b6f3e522872f3a14fcc5e5de2f58bf23b
-- 
2.48.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/6] firewire: core: remove useless generation check
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Two functions, fw_core_handle_bus_reset() and bm_work(), are serialized
by a commit 3d91fd440cc7 ("firewire: core: disable bus management work
temporarily during updating topology"). Therefore the generation member
of fw_card is immutable in bm_work().

This commit removes useless generation check in bm_work().

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 4fcd5ce4b2ce..ef00125fb01a 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -362,14 +362,12 @@ static void bm_work(struct work_struct *work)
 		if (rcode == RCODE_COMPLETE) {
 			int bm_id = be32_to_cpu(data[0]);
 
-			if (generation == card->generation) {
-				// 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;
-				}
+			// 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) {
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 1/6] firewire: core: remove useless generation check Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The result of the lock transaction to swap bus manager on isochronous
resource manager looks like an ad-hoc style. It is hard to read.

This commit uses switch statement to evaluate the result.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index ef00125fb01a..e9bf8d93f5b7 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -355,11 +355,18 @@ static void bm_work(struct work_struct *work)
 				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
 				data, sizeof(data));
 
-		// Another bus reset, BM work has been rescheduled.
-		if (rcode == RCODE_GENERATION)
+		switch (rcode) {
+		case RCODE_GENERATION:
+			// Another bus reset, BM work has been rescheduled.
 			return;
-
-		if (rcode == RCODE_COMPLETE) {
+		case RCODE_SEND_ERROR:
+			// We have been unable to send the lock request due to
+			// some local problem.  Let's try again later and hope
+			// that the problem has gone away by then.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
+		case RCODE_COMPLETE:
+		{
 			int bm_id = be32_to_cpu(data[0]);
 
 			// Used by cdev layer for "struct fw_cdev_event_bus_reset".
@@ -376,29 +383,20 @@ static void bm_work(struct work_struct *work)
 					allocate_broadcast_channel(card, generation);
 				return;
 			}
+			break;
 		}
-
-		if (rcode == RCODE_SEND_ERROR) {
-			/*
-			 * We have been unable to send the lock request due to
-			 * some local problem.  Let's try again later and hope
-			 * that the problem has gone away by then.
-			 */
-			fw_schedule_bm_work(card, msecs_to_jiffies(125));
-			return;
-		}
-
-		if (rcode != RCODE_COMPLETE && !keep_this_irm) {
-			/*
-			 * The lock request failed, maybe the IRM
-			 * isn't really IRM capable after all. Let's
-			 * do a bus reset and pick the local node as
-			 * root, and thus, IRM.
-			 */
-			new_root_id = local_id;
-			fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-				  fw_rcode_string(rcode), new_root_id);
-			goto pick_me;
+		default:
+			if (!keep_this_irm) {
+				// The lock request failed, maybe the IRM
+				// isn't really IRM capable after all. Let's
+				// do a bus reset and pick the local node as
+				// root, and thus, IRM.
+				new_root_id = local_id;
+				fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+					  fw_rcode_string(rcode), new_root_id);
+				goto pick_me;
+			}
+			break;
 		}
 	} else if (card->bm_generation != generation) {
 		/*
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/6] firewire: core: code refactoring for the case of generation mismatch
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 1/6] firewire: core: remove useless generation check Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Current implementation stores the bus generation at which the bus manager
contending procedure finishes. The condition for the procedure is the
mismatch of the stored generation against current bus generation.

This commit refactors the code for the contending procedure. Two existing
branches are put into a new branch to detect the generation mismatch, thus
the most of change is indentation.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e9bf8d93f5b7..02058af705cc 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -290,7 +290,7 @@ static void bm_work(struct work_struct *work)
 	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, local_id;
-	int expected_gap_count, generation, grace;
+	int expected_gap_count, generation;
 	bool do_reset = false;
 
 	if (card->local_node == NULL)
@@ -304,107 +304,107 @@ static void bm_work(struct work_struct *work)
 	irm_id   = card->irm_node->node_id;
 	local_id = card->local_node->node_id;
 
-	grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
-
-	if ((is_next_generation(generation, card->bm_generation) &&
-	     !card->bm_abdicate) ||
-	    (card->bm_generation != generation && grace)) {
-		/*
-		 * This first step is to figure out who is IRM and
-		 * then try to become bus manager.  If the IRM is not
-		 * well defined (e.g. does not have an active link
-		 * layer or does not responds to our lock request, we
-		 * will have to do a little vigilante bus management.
-		 * In that case, we do a goto into the gap count logic
-		 * so that when we do the reset, we still optimize the
-		 * gap count.  That could well save a reset in the
-		 * next generation.
-		 */
-		__be32 data[2] = {
-			cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
-			cpu_to_be32(local_id),
-		};
-		struct fw_device *irm_device = fw_node_get_device(card->irm_node);
-		bool irm_is_1394_1995_only = false;
-		bool keep_this_irm = false;
-		int rcode;
-
-		if (!card->irm_node->link_on) {
-			new_root_id = local_id;
-			fw_notice(card, "%s, making local node (%02x) root\n",
-				  "IRM has link off", new_root_id);
-			goto pick_me;
-		}
-
-		if (irm_device && irm_device->config_rom) {
-			irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
-
-			// Canon MV5i works unreliably if it is not root node.
-			keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
-		}
+	if (card->bm_generation != generation) {
+		bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
+
+		if (grace ||
+		    (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) {
+			// This first step is to figure out who is IRM and
+			// then try to become bus manager.  If the IRM is not
+			// well defined (e.g. does not have an active link
+			// layer or does not responds to our lock request, we
+			// will have to do a little vigilante bus management.
+			// In that case, we do a goto into the gap count logic
+			// so that when we do the reset, we still optimize the
+			// gap count.  That could well save a reset in the
+			// next generation.
+			__be32 data[2] = {
+				cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
+				cpu_to_be32(local_id),
+			};
+			struct fw_device *irm_device = fw_node_get_device(card->irm_node);
+			bool irm_is_1394_1995_only = false;
+			bool keep_this_irm = false;
+			int rcode;
+
+			if (!card->irm_node->link_on) {
+				new_root_id = local_id;
+				fw_notice(card, "%s, making local node (%02x) root\n",
+					  "IRM has link off", new_root_id);
+				goto pick_me;
+			}
 
-		if (irm_is_1394_1995_only && !keep_this_irm) {
-			new_root_id = local_id;
-			fw_notice(card, "%s, making local node (%02x) root\n",
-				  "IRM is not 1394a compliant", new_root_id);
-			goto pick_me;
-		}
+			if (irm_device && irm_device->config_rom) {
+				irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
 
-		rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
-				irm_id, generation, SCODE_100,
-				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
-				data, sizeof(data));
+				// Canon MV5i works unreliably if it is not root node.
+				keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
+			}
 
-		switch (rcode) {
-		case RCODE_GENERATION:
-			// Another bus reset, BM work has been rescheduled.
-			return;
-		case RCODE_SEND_ERROR:
-			// We have been unable to send the lock request due to
-			// some local problem.  Let's try again later and hope
-			// that the problem has gone away by then.
-			fw_schedule_bm_work(card, msecs_to_jiffies(125));
-			return;
-		case RCODE_COMPLETE:
-		{
-			int bm_id = be32_to_cpu(data[0]);
-
-			// 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 (irm_is_1394_1995_only && !keep_this_irm) {
+				new_root_id = local_id;
+				fw_notice(card, "%s, making local node (%02x) root\n",
+					  "IRM is not 1394a compliant", new_root_id);
+				goto pick_me;
 			}
 
-			if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) {
-				// Somebody else is BM.  Only act as IRM.
-				if (local_id == irm_id)
-					allocate_broadcast_channel(card, generation);
+			rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
+					irm_id, generation, SCODE_100,
+					CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
+					data, sizeof(data));
+
+			switch (rcode) {
+			case RCODE_GENERATION:
+				// Another bus reset, BM work has been rescheduled.
 				return;
+			case RCODE_SEND_ERROR:
+				// We have been unable to send the lock request due to
+				// some local problem.  Let's try again later and hope
+				// that the problem has gone away by then.
+				fw_schedule_bm_work(card, msecs_to_jiffies(125));
+				return;
+			case RCODE_COMPLETE:
+			{
+				int bm_id = be32_to_cpu(data[0]);
+
+				// 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) {
+					// Somebody else is BM.  Only act as IRM.
+					if (local_id == irm_id)
+						allocate_broadcast_channel(card, generation);
+					return;
+				}
+				break;
 			}
-			break;
-		}
-		default:
-			if (!keep_this_irm) {
-				// The lock request failed, maybe the IRM
-				// isn't really IRM capable after all. Let's
-				// do a bus reset and pick the local node as
-				// root, and thus, IRM.
-				new_root_id = local_id;
-				fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-					  fw_rcode_string(rcode), new_root_id);
-				goto pick_me;
+			default:
+				if (!keep_this_irm) {
+					// The lock request failed, maybe the IRM
+					// isn't really IRM capable after all. Let's
+					// do a bus reset and pick the local node as
+					// root, and thus, IRM.
+					new_root_id = local_id;
+					fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+						  fw_rcode_string(rcode), new_root_id);
+					goto pick_me;
+				}
+				break;
 			}
-			break;
+
+			// A node contends for bus manager in this generation.
+			card->bm_generation = generation;
+		} else {
+			// We weren't BM in the last generation, and the last
+			// bus reset is less than 125ms ago.  Reschedule this job.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
 		}
-	} else if (card->bm_generation != generation) {
-		/*
-		 * We weren't BM in the last generation, and the last
-		 * bus reset is less than 125ms ago.  Reschedule this job.
-		 */
-		fw_schedule_bm_work(card, msecs_to_jiffies(125));
-		return;
 	}
 
 	/*
@@ -412,8 +412,6 @@ static void bm_work(struct work_struct *work)
 	 * make sure we have an active cycle master and do gap count
 	 * optimization.
 	 */
-	card->bm_generation = generation;
-
 	if (card->gap_count == GAP_COUNT_MISMATCHED) {
 		/*
 		 * If self IDs have inconsistent gap counts, do a
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/6] firewire: core: code refactoring to split contention procedure for bus manager
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (2 preceding siblings ...)
  2025-09-18 23:08 ` [PATCH 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:08 ` [PATCH 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The precedure to contend for bus manager has much code. It is better to
split it into a helper function.

This commit refactors in the point.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 02058af705cc..6268b595d4fa 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -279,6 +279,102 @@ void fw_schedule_bm_work(struct fw_card *card, unsigned long delay)
 		fw_card_put(card);
 }
 
+enum bm_contention_outcome {
+	// The bus management contention window is not expired.
+	BM_CONTENTION_OUTCOME_WITHIN_WINDOW = 0,
+	// The IRM node has link off.
+	BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF,
+	// The IRM node complies IEEE 1394:1994 only.
+	BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY,
+	// Another bus reset, BM work has been rescheduled.
+	BM_CONTENTION_OUTCOME_AT_NEW_GENERATION,
+	// We have been unable to send the lock request to IRM node due to some local problem.
+	BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION,
+	// The lock request failed, maybe the IRM isn't really IRM capable after all.
+	BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM,
+	// Somebody else is BM.
+	BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM,
+	// The local node succeeds after contending for bus manager.
+	BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM,
+};
+
+static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
+{
+	int generation = card->generation;
+	int local_id = card->local_node->node_id;
+	__be32 data[2] = {
+		cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
+		cpu_to_be32(local_id),
+	};
+	bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
+	bool irm_is_1394_1995_only = false;
+	bool keep_this_irm = false;
+	struct fw_node *irm_node;
+	struct fw_device *irm_device;
+	int rcode;
+
+	if (!grace) {
+		if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate)
+			return BM_CONTENTION_OUTCOME_WITHIN_WINDOW;
+	}
+
+	irm_node = card->irm_node;
+	if (!irm_node->link_on) {
+		fw_notice(card, "IRM has link off, making local node (%02x) root\n", local_id);
+		return BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF;
+	}
+
+	irm_device = fw_node_get_device(irm_node);
+	if (irm_device && irm_device->config_rom) {
+		irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
+
+		// Canon MV5i works unreliably if it is not root node.
+		keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
+	}
+
+	if (irm_is_1394_1995_only && !keep_this_irm) {
+		fw_notice(card, "IRM is not 1394a compliant, making local node (%02x) root\n",
+			  local_id);
+		return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
+	}
+
+	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation,
+				   SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data,
+				   sizeof(data));
+
+	switch (rcode) {
+	case RCODE_GENERATION:
+		return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION;
+	case RCODE_SEND_ERROR:
+		return BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION;
+	case RCODE_COMPLETE:
+	{
+		int bm_id = be32_to_cpu(data[0]);
+
+		// 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)
+			return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM;
+		else
+			return BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM;
+	}
+	default:
+		if (!keep_this_irm) {
+			fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+				  fw_rcode_string(rcode), local_id);
+			return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
+		} else {
+			return BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM;
+		}
+	}
+}
+
 DEFINE_FREE(node_unref, struct fw_node *, if (_T) fw_node_put(_T))
 DEFINE_FREE(card_unref, struct fw_card *, if (_T) fw_card_put(_T))
 
@@ -305,105 +401,40 @@ static void bm_work(struct work_struct *work)
 	local_id = card->local_node->node_id;
 
 	if (card->bm_generation != generation) {
-		bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
-
-		if (grace ||
-		    (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) {
-			// This first step is to figure out who is IRM and
-			// then try to become bus manager.  If the IRM is not
-			// well defined (e.g. does not have an active link
-			// layer or does not responds to our lock request, we
-			// will have to do a little vigilante bus management.
-			// In that case, we do a goto into the gap count logic
-			// so that when we do the reset, we still optimize the
-			// gap count.  That could well save a reset in the
-			// next generation.
-			__be32 data[2] = {
-				cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
-				cpu_to_be32(local_id),
-			};
-			struct fw_device *irm_device = fw_node_get_device(card->irm_node);
-			bool irm_is_1394_1995_only = false;
-			bool keep_this_irm = false;
-			int rcode;
-
-			if (!card->irm_node->link_on) {
-				new_root_id = local_id;
-				fw_notice(card, "%s, making local node (%02x) root\n",
-					  "IRM has link off", new_root_id);
-				goto pick_me;
-			}
+		enum bm_contention_outcome result = contend_for_bm(card);
 
-			if (irm_device && irm_device->config_rom) {
-				irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
-
-				// Canon MV5i works unreliably if it is not root node.
-				keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
-			}
-
-			if (irm_is_1394_1995_only && !keep_this_irm) {
-				new_root_id = local_id;
-				fw_notice(card, "%s, making local node (%02x) root\n",
-					  "IRM is not 1394a compliant", new_root_id);
-				goto pick_me;
-			}
-
-			rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
-					irm_id, generation, SCODE_100,
-					CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
-					data, sizeof(data));
-
-			switch (rcode) {
-			case RCODE_GENERATION:
-				// Another bus reset, BM work has been rescheduled.
-				return;
-			case RCODE_SEND_ERROR:
-				// We have been unable to send the lock request due to
-				// some local problem.  Let's try again later and hope
-				// that the problem has gone away by then.
-				fw_schedule_bm_work(card, msecs_to_jiffies(125));
-				return;
-			case RCODE_COMPLETE:
-			{
-				int bm_id = be32_to_cpu(data[0]);
-
-				// 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) {
-					// Somebody else is BM.  Only act as IRM.
-					if (local_id == irm_id)
-						allocate_broadcast_channel(card, generation);
-					return;
-				}
-				break;
-			}
-			default:
-				if (!keep_this_irm) {
-					// The lock request failed, maybe the IRM
-					// isn't really IRM capable after all. Let's
-					// do a bus reset and pick the local node as
-					// root, and thus, IRM.
-					new_root_id = local_id;
-					fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-						  fw_rcode_string(rcode), new_root_id);
-					goto pick_me;
-				}
-				break;
-			}
-
-			// A node contends for bus manager in this generation.
-			card->bm_generation = generation;
-		} else {
-			// We weren't BM in the last generation, and the last
-			// bus reset is less than 125ms ago.  Reschedule this job.
+		switch (result) {
+		case BM_CONTENTION_OUTCOME_WITHIN_WINDOW:
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
+		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
+			// BM work has been rescheduled.
+			return;
+		case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION:
+			// Let's try again later and hope that the local problem has gone away by
+			// then.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
+		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
+			// Let's do a bus reset and pick the local node as root, and thus, IRM.
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
+			if (local_id == irm_id) {
+				// Only acts as IRM.
+				allocate_broadcast_channel(card, generation);
+			}
+			fallthrough;
+		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
+		default:
+			card->bm_generation = generation;
+			break;
 		}
 	}
 
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/6] firewire: core; eliminate pick_me goto label
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (3 preceding siblings ...)
  2025-09-18 23:08 ` [PATCH 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:46   ` Takashi Sakamoto
  2025-09-19 17:52   ` kernel test robot
  2025-09-18 23:08 ` [PATCH 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto
                   ` (7 subsequent siblings)
  12 siblings, 2 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

This commit uses condition statements instead of pick_me goto label.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 6268b595d4fa..b766ce5fdea4 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -388,6 +388,7 @@ static void bm_work(struct work_struct *work)
 	int root_id, new_root_id, irm_id, local_id;
 	int expected_gap_count, generation;
 	bool do_reset = false;
+	bool stand_for_root;
 
 	if (card->local_node == NULL)
 		return;
@@ -408,11 +409,11 @@ static void bm_work(struct work_struct *work)
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
 			// BM work has been rescheduled.
 			return;
@@ -423,8 +424,8 @@ static void bm_work(struct work_struct *work)
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
 			// Let's do a bus reset and pick the local node as root, and thus, IRM.
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
 			if (local_id == irm_id) {
 				// Only acts as IRM.
@@ -434,60 +435,61 @@ static void bm_work(struct work_struct *work)
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
 		default:
 			card->bm_generation = generation;
+			stand_for_root = false;
 			break;
 		}
 	}
 
-	/*
-	 * We're bus manager for this generation, so next step is to
-	 * make sure we have an active cycle master and do gap count
-	 * optimization.
-	 */
-	if (card->gap_count == GAP_COUNT_MISMATCHED) {
-		/*
-		 * If self IDs have inconsistent gap counts, do a
-		 * bus reset ASAP. The config rom read might never
-		 * complete, so don't wait for it. However, still
-		 * send a PHY configuration packet prior to the
-		 * bus reset. The PHY configuration packet might
-		 * fail, but 1394-2008 8.4.5.2 explicitly permits
-		 * it in this case, so it should be safe to try.
-		 */
-		new_root_id = local_id;
-		/*
-		 * We must always send a bus reset if the gap count
-		 * is inconsistent, so bypass the 5-reset limit.
-		 */
-		card->bm_retries = 0;
-	} else {
-		// Now investigate root node.
-		struct fw_device *root_device = fw_node_get_device(root_node);
-
-		if (root_device == NULL) {
-			// Either link_on is false, or we failed to read the
-			// config rom.  In either case, pick another root.
-			new_root_id = local_id;
+	// We're bus manager for this generation, so next step is to make sure we have an active
+	// cycle master and do gap count optimization.
+	if (!stand_for_root) {
+		if (card->gap_count == GAP_COUNT_MISMATCHED) {
+			// If self IDs have inconsistent gap counts, do a
+			// bus reset ASAP. The config rom read might never
+			// complete, so don't wait for it. However, still
+			// send a PHY configuration packet prior to the
+			// bus reset. The PHY configuration packet might
+			// fail, but 1394-2008 8.4.5.2 explicitly permits
+			// it in this case, so it should be safe to try.
+			stand_for_root = true;
+
+			// We must always send a bus reset if the gap count
+			// is inconsistent, so bypass the 5-reset limit.
+			card->bm_retries = 0;
 		} else {
-			bool root_device_is_running =
-				atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
+			// Now investigate root node.
+			struct fw_device *root_device = fw_node_get_device(root_node);
 
-			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.
-				return;
-			} else if (root_device->cmc) {
-				// We will send out a force root packet for this
-				// node as part of the gap count optimization.
-				new_root_id = root_id;
+			if (root_device == NULL) {
+				// Either link_on is false, or we failed to read the
+				// config rom.  In either case, pick another root.
+				stand_for_root = true;
 			} else {
-				// Current root has an active link layer and we
-				// successfully read the config rom, but it's not
-				// cycle master capable.
-				new_root_id = local_id;
+				bool root_device_is_running =
+					atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
+
+				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.
+					return;
+				} else if (!root_device->cmc) {
+					// Current root has an active link layer and we
+					// successfully read the config rom, but it's not
+					// cycle master capable.
+					stand_for_root = true;
+				}
 			}
 		}
 	}
- pick_me:
+
+	if (stand_for_root) {
+		new_root_id = local_id;
+	} else {
+		// We will send out a force root packet for this node as part of the gap count
+		// optimization on behalf of the node.
+		new_root_id = root_id;
+	}
+
 	/*
 	 * Pick a gap count from 1394a table E-1.  The table doesn't cover
 	 * the typically much larger 1394b beta repeater delays though.
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/6] firewire: core: minor code refactoring to delete useless local variable
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (4 preceding siblings ...)
  2025-09-18 23:08 ` [PATCH 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
@ 2025-09-18 23:08 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:08 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The do_reset local variable has less merit. Let's remove it.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index b766ce5fdea4..527a99ef7c90 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -387,7 +387,6 @@ static void bm_work(struct work_struct *work)
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, local_id;
 	int expected_gap_count, generation;
-	bool do_reset = false;
 	bool stand_for_root;
 
 	if (card->local_node == NULL)
@@ -500,16 +499,10 @@ static void bm_work(struct work_struct *work)
 	else
 		expected_gap_count = 63;
 
-	/*
-	 * Finally, figure out if we should do a reset or not.  If we have
-	 * done less than 5 resets with the same physical topology and we
-	 * have either a new root or a new gap count setting, let's do it.
-	 */
-
-	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id))
-		do_reset = true;
-
-	if (do_reset) {
+	// Finally, figure out if we should do a reset or not. If we have done less than 5 resets
+	// with the same physical topology and we have either a new root or a new gap count
+	// setting, let's do it.
+	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) {
 		int card_gap_count = card->gap_count;
 
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/6] firewire: core; eliminate pick_me goto label
  2025-09-18 23:08 ` [PATCH 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
@ 2025-09-18 23:46   ` Takashi Sakamoto
  2025-09-19 17:52   ` kernel test robot
  1 sibling, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:46 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Hi,

On Fri, Sep 19, 2025 at 08:08:56AM +0900, Takashi Sakamoto wrote:
> This commit uses condition statements instead of pick_me goto label.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  drivers/firewire/core-card.c | 102 ++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
> index 6268b595d4fa..b766ce5fdea4 100644
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -388,6 +388,7 @@ static void bm_work(struct work_struct *work)
>  	int root_id, new_root_id, irm_id, local_id;
>  	int expected_gap_count, generation;
>  	bool do_reset = false;
> +	bool stand_for_root;
>  
>  	if (card->local_node == NULL)
>  		return;
> @@ -408,11 +409,11 @@ static void bm_work(struct work_struct *work)
>  			fw_schedule_bm_work(card, msecs_to_jiffies(125));
>  			return;
>  		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
>  			// BM work has been rescheduled.
>  			return;
> @@ -423,8 +424,8 @@ static void bm_work(struct work_struct *work)
>  			return;
>  		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
>  			// Let's do a bus reset and pick the local node as root, and thus, IRM.
> -			new_root_id = local_id;
> -			goto pick_me;
> +			stand_for_root = true;
> +			break;
>  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
>  			if (local_id == irm_id) {
>  				// Only acts as IRM.
> @@ -434,60 +435,61 @@ static void bm_work(struct work_struct *work)
>  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
>  		default:
>  			card->bm_generation = generation;
> +			stand_for_root = false;
>  			break;
>  		}
>  	}
>  
> -	/*
> -	 * We're bus manager for this generation, so next step is to
> -	 * make sure we have an active cycle master and do gap count
> -	 * optimization.
> -	 */
> -	if (card->gap_count == GAP_COUNT_MISMATCHED) {
> -		/*
> -		 * If self IDs have inconsistent gap counts, do a
> -		 * bus reset ASAP. The config rom read might never
> -		 * complete, so don't wait for it. However, still
> -		 * send a PHY configuration packet prior to the
> -		 * bus reset. The PHY configuration packet might
> -		 * fail, but 1394-2008 8.4.5.2 explicitly permits
> -		 * it in this case, so it should be safe to try.
> -		 */
> -		new_root_id = local_id;
> -		/*
> -		 * We must always send a bus reset if the gap count
> -		 * is inconsistent, so bypass the 5-reset limit.
> -		 */
> -		card->bm_retries = 0;
> -	} else {
> -		// Now investigate root node.
> -		struct fw_device *root_device = fw_node_get_device(root_node);
> -
> -		if (root_device == NULL) {
> -			// Either link_on is false, or we failed to read the
> -			// config rom.  In either case, pick another root.
> -			new_root_id = local_id;
> +	// We're bus manager for this generation, so next step is to make sure we have an active
> +	// cycle master and do gap count optimization.
> +	if (!stand_for_root) {

I realized that the "stand_for_root" local variable would be
ununitialized here at the case of "card->bm_generation == generation".
Let me post take 2.


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (5 preceding siblings ...)
  2025-09-18 23:08 ` [PATCH 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-20  1:54   ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 1/6] firewire: core: remove useless generation check Takashi Sakamoto
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Hi,

This patchset is the revised version of my previous one:
https://lore.kernel.org/lkml/20250918230857.127400-1-o-takashi@sakamocchi.jp/

Changes from v1:
* Ensure to initialize local variable

Takashi Sakamoto (6):
  firewire: core: remove useless generation check
  firewire: core: use switch statement to evaluate transaction result to
    CSR_BUS_MANAGER_ID
  firewire: core: code refactoring for the case of generation mismatch
  firewire: core: code refactoring to split contention procedure for bus
    manager
  firewire: core; eliminate pick_me goto label
  firewire: core: minor code refactoring to delete useless local
    variable

 drivers/firewire/core-card.c | 335 ++++++++++++++++++-----------------
 1 file changed, 177 insertions(+), 158 deletions(-)


base-commit: e6d2338b6f3e522872f3a14fcc5e5de2f58bf23b
-- 
2.48.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/6] firewire: core: remove useless generation check
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (6 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Two functions, fw_core_handle_bus_reset() and bm_work(), are serialized
by a commit 3d91fd440cc7 ("firewire: core: disable bus management work
temporarily during updating topology"). Therefore the generation member
of fw_card is immutable in bm_work().

This commit removes useless generation check in bm_work().

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 4fcd5ce4b2ce..ef00125fb01a 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -362,14 +362,12 @@ static void bm_work(struct work_struct *work)
 		if (rcode == RCODE_COMPLETE) {
 			int bm_id = be32_to_cpu(data[0]);
 
-			if (generation == card->generation) {
-				// 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;
-				}
+			// 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) {
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (7 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 1/6] firewire: core: remove useless generation check Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The result of the lock transaction to swap bus manager on isochronous
resource manager looks like an ad-hoc style. It is hard to read.

This commit uses switch statement to evaluate the result.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index ef00125fb01a..e9bf8d93f5b7 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -355,11 +355,18 @@ static void bm_work(struct work_struct *work)
 				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
 				data, sizeof(data));
 
-		// Another bus reset, BM work has been rescheduled.
-		if (rcode == RCODE_GENERATION)
+		switch (rcode) {
+		case RCODE_GENERATION:
+			// Another bus reset, BM work has been rescheduled.
 			return;
-
-		if (rcode == RCODE_COMPLETE) {
+		case RCODE_SEND_ERROR:
+			// We have been unable to send the lock request due to
+			// some local problem.  Let's try again later and hope
+			// that the problem has gone away by then.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
+		case RCODE_COMPLETE:
+		{
 			int bm_id = be32_to_cpu(data[0]);
 
 			// Used by cdev layer for "struct fw_cdev_event_bus_reset".
@@ -376,29 +383,20 @@ static void bm_work(struct work_struct *work)
 					allocate_broadcast_channel(card, generation);
 				return;
 			}
+			break;
 		}
-
-		if (rcode == RCODE_SEND_ERROR) {
-			/*
-			 * We have been unable to send the lock request due to
-			 * some local problem.  Let's try again later and hope
-			 * that the problem has gone away by then.
-			 */
-			fw_schedule_bm_work(card, msecs_to_jiffies(125));
-			return;
-		}
-
-		if (rcode != RCODE_COMPLETE && !keep_this_irm) {
-			/*
-			 * The lock request failed, maybe the IRM
-			 * isn't really IRM capable after all. Let's
-			 * do a bus reset and pick the local node as
-			 * root, and thus, IRM.
-			 */
-			new_root_id = local_id;
-			fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-				  fw_rcode_string(rcode), new_root_id);
-			goto pick_me;
+		default:
+			if (!keep_this_irm) {
+				// The lock request failed, maybe the IRM
+				// isn't really IRM capable after all. Let's
+				// do a bus reset and pick the local node as
+				// root, and thus, IRM.
+				new_root_id = local_id;
+				fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+					  fw_rcode_string(rcode), new_root_id);
+				goto pick_me;
+			}
+			break;
 		}
 	} else if (card->bm_generation != generation) {
 		/*
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/6] firewire: core: code refactoring for the case of generation mismatch
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (8 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

Current implementation stores the bus generation at which the bus manager
contending procedure finishes. The condition for the procedure is the
mismatch of the stored generation against current bus generation.

This commit refactors the code for the contending procedure. Two existing
branches are put into a new branch to detect the generation mismatch, thus
the most of change is indentation.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e9bf8d93f5b7..02058af705cc 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -290,7 +290,7 @@ static void bm_work(struct work_struct *work)
 	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, local_id;
-	int expected_gap_count, generation, grace;
+	int expected_gap_count, generation;
 	bool do_reset = false;
 
 	if (card->local_node == NULL)
@@ -304,107 +304,107 @@ static void bm_work(struct work_struct *work)
 	irm_id   = card->irm_node->node_id;
 	local_id = card->local_node->node_id;
 
-	grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
-
-	if ((is_next_generation(generation, card->bm_generation) &&
-	     !card->bm_abdicate) ||
-	    (card->bm_generation != generation && grace)) {
-		/*
-		 * This first step is to figure out who is IRM and
-		 * then try to become bus manager.  If the IRM is not
-		 * well defined (e.g. does not have an active link
-		 * layer or does not responds to our lock request, we
-		 * will have to do a little vigilante bus management.
-		 * In that case, we do a goto into the gap count logic
-		 * so that when we do the reset, we still optimize the
-		 * gap count.  That could well save a reset in the
-		 * next generation.
-		 */
-		__be32 data[2] = {
-			cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
-			cpu_to_be32(local_id),
-		};
-		struct fw_device *irm_device = fw_node_get_device(card->irm_node);
-		bool irm_is_1394_1995_only = false;
-		bool keep_this_irm = false;
-		int rcode;
-
-		if (!card->irm_node->link_on) {
-			new_root_id = local_id;
-			fw_notice(card, "%s, making local node (%02x) root\n",
-				  "IRM has link off", new_root_id);
-			goto pick_me;
-		}
-
-		if (irm_device && irm_device->config_rom) {
-			irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
-
-			// Canon MV5i works unreliably if it is not root node.
-			keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
-		}
+	if (card->bm_generation != generation) {
+		bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
+
+		if (grace ||
+		    (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) {
+			// This first step is to figure out who is IRM and
+			// then try to become bus manager.  If the IRM is not
+			// well defined (e.g. does not have an active link
+			// layer or does not responds to our lock request, we
+			// will have to do a little vigilante bus management.
+			// In that case, we do a goto into the gap count logic
+			// so that when we do the reset, we still optimize the
+			// gap count.  That could well save a reset in the
+			// next generation.
+			__be32 data[2] = {
+				cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
+				cpu_to_be32(local_id),
+			};
+			struct fw_device *irm_device = fw_node_get_device(card->irm_node);
+			bool irm_is_1394_1995_only = false;
+			bool keep_this_irm = false;
+			int rcode;
+
+			if (!card->irm_node->link_on) {
+				new_root_id = local_id;
+				fw_notice(card, "%s, making local node (%02x) root\n",
+					  "IRM has link off", new_root_id);
+				goto pick_me;
+			}
 
-		if (irm_is_1394_1995_only && !keep_this_irm) {
-			new_root_id = local_id;
-			fw_notice(card, "%s, making local node (%02x) root\n",
-				  "IRM is not 1394a compliant", new_root_id);
-			goto pick_me;
-		}
+			if (irm_device && irm_device->config_rom) {
+				irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
 
-		rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
-				irm_id, generation, SCODE_100,
-				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
-				data, sizeof(data));
+				// Canon MV5i works unreliably if it is not root node.
+				keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
+			}
 
-		switch (rcode) {
-		case RCODE_GENERATION:
-			// Another bus reset, BM work has been rescheduled.
-			return;
-		case RCODE_SEND_ERROR:
-			// We have been unable to send the lock request due to
-			// some local problem.  Let's try again later and hope
-			// that the problem has gone away by then.
-			fw_schedule_bm_work(card, msecs_to_jiffies(125));
-			return;
-		case RCODE_COMPLETE:
-		{
-			int bm_id = be32_to_cpu(data[0]);
-
-			// 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 (irm_is_1394_1995_only && !keep_this_irm) {
+				new_root_id = local_id;
+				fw_notice(card, "%s, making local node (%02x) root\n",
+					  "IRM is not 1394a compliant", new_root_id);
+				goto pick_me;
 			}
 
-			if (bm_id != BUS_MANAGER_ID_NOT_REGISTERED) {
-				// Somebody else is BM.  Only act as IRM.
-				if (local_id == irm_id)
-					allocate_broadcast_channel(card, generation);
+			rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
+					irm_id, generation, SCODE_100,
+					CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
+					data, sizeof(data));
+
+			switch (rcode) {
+			case RCODE_GENERATION:
+				// Another bus reset, BM work has been rescheduled.
 				return;
+			case RCODE_SEND_ERROR:
+				// We have been unable to send the lock request due to
+				// some local problem.  Let's try again later and hope
+				// that the problem has gone away by then.
+				fw_schedule_bm_work(card, msecs_to_jiffies(125));
+				return;
+			case RCODE_COMPLETE:
+			{
+				int bm_id = be32_to_cpu(data[0]);
+
+				// 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) {
+					// Somebody else is BM.  Only act as IRM.
+					if (local_id == irm_id)
+						allocate_broadcast_channel(card, generation);
+					return;
+				}
+				break;
 			}
-			break;
-		}
-		default:
-			if (!keep_this_irm) {
-				// The lock request failed, maybe the IRM
-				// isn't really IRM capable after all. Let's
-				// do a bus reset and pick the local node as
-				// root, and thus, IRM.
-				new_root_id = local_id;
-				fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-					  fw_rcode_string(rcode), new_root_id);
-				goto pick_me;
+			default:
+				if (!keep_this_irm) {
+					// The lock request failed, maybe the IRM
+					// isn't really IRM capable after all. Let's
+					// do a bus reset and pick the local node as
+					// root, and thus, IRM.
+					new_root_id = local_id;
+					fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+						  fw_rcode_string(rcode), new_root_id);
+					goto pick_me;
+				}
+				break;
 			}
-			break;
+
+			// A node contends for bus manager in this generation.
+			card->bm_generation = generation;
+		} else {
+			// We weren't BM in the last generation, and the last
+			// bus reset is less than 125ms ago.  Reschedule this job.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
 		}
-	} else if (card->bm_generation != generation) {
-		/*
-		 * We weren't BM in the last generation, and the last
-		 * bus reset is less than 125ms ago.  Reschedule this job.
-		 */
-		fw_schedule_bm_work(card, msecs_to_jiffies(125));
-		return;
 	}
 
 	/*
@@ -412,8 +412,6 @@ static void bm_work(struct work_struct *work)
 	 * make sure we have an active cycle master and do gap count
 	 * optimization.
 	 */
-	card->bm_generation = generation;
-
 	if (card->gap_count == GAP_COUNT_MISMATCHED) {
 		/*
 		 * If self IDs have inconsistent gap counts, do a
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/6] firewire: core: code refactoring to split contention procedure for bus manager
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (9 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The precedure to contend for bus manager has much code. It is better to
split it into a helper function.

This commit refactors in the point.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 02058af705cc..6268b595d4fa 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -279,6 +279,102 @@ void fw_schedule_bm_work(struct fw_card *card, unsigned long delay)
 		fw_card_put(card);
 }
 
+enum bm_contention_outcome {
+	// The bus management contention window is not expired.
+	BM_CONTENTION_OUTCOME_WITHIN_WINDOW = 0,
+	// The IRM node has link off.
+	BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF,
+	// The IRM node complies IEEE 1394:1994 only.
+	BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY,
+	// Another bus reset, BM work has been rescheduled.
+	BM_CONTENTION_OUTCOME_AT_NEW_GENERATION,
+	// We have been unable to send the lock request to IRM node due to some local problem.
+	BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION,
+	// The lock request failed, maybe the IRM isn't really IRM capable after all.
+	BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM,
+	// Somebody else is BM.
+	BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM,
+	// The local node succeeds after contending for bus manager.
+	BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM,
+};
+
+static enum bm_contention_outcome contend_for_bm(struct fw_card *card)
+{
+	int generation = card->generation;
+	int local_id = card->local_node->node_id;
+	__be32 data[2] = {
+		cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
+		cpu_to_be32(local_id),
+	};
+	bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
+	bool irm_is_1394_1995_only = false;
+	bool keep_this_irm = false;
+	struct fw_node *irm_node;
+	struct fw_device *irm_device;
+	int rcode;
+
+	if (!grace) {
+		if (!is_next_generation(generation, card->bm_generation) || card->bm_abdicate)
+			return BM_CONTENTION_OUTCOME_WITHIN_WINDOW;
+	}
+
+	irm_node = card->irm_node;
+	if (!irm_node->link_on) {
+		fw_notice(card, "IRM has link off, making local node (%02x) root\n", local_id);
+		return BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF;
+	}
+
+	irm_device = fw_node_get_device(irm_node);
+	if (irm_device && irm_device->config_rom) {
+		irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
+
+		// Canon MV5i works unreliably if it is not root node.
+		keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
+	}
+
+	if (irm_is_1394_1995_only && !keep_this_irm) {
+		fw_notice(card, "IRM is not 1394a compliant, making local node (%02x) root\n",
+			  local_id);
+		return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
+	}
+
+	rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP, irm_node->node_id, generation,
+				   SCODE_100, CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID, data,
+				   sizeof(data));
+
+	switch (rcode) {
+	case RCODE_GENERATION:
+		return BM_CONTENTION_OUTCOME_AT_NEW_GENERATION;
+	case RCODE_SEND_ERROR:
+		return BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION;
+	case RCODE_COMPLETE:
+	{
+		int bm_id = be32_to_cpu(data[0]);
+
+		// 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)
+			return BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM;
+		else
+			return BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM;
+	}
+	default:
+		if (!keep_this_irm) {
+			fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
+				  fw_rcode_string(rcode), local_id);
+			return BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY;
+		} else {
+			return BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM;
+		}
+	}
+}
+
 DEFINE_FREE(node_unref, struct fw_node *, if (_T) fw_node_put(_T))
 DEFINE_FREE(card_unref, struct fw_card *, if (_T) fw_card_put(_T))
 
@@ -305,105 +401,40 @@ static void bm_work(struct work_struct *work)
 	local_id = card->local_node->node_id;
 
 	if (card->bm_generation != generation) {
-		bool grace = time_is_before_jiffies64(card->reset_jiffies + msecs_to_jiffies(125));
-
-		if (grace ||
-		    (is_next_generation(generation, card->bm_generation) && !card->bm_abdicate)) {
-			// This first step is to figure out who is IRM and
-			// then try to become bus manager.  If the IRM is not
-			// well defined (e.g. does not have an active link
-			// layer or does not responds to our lock request, we
-			// will have to do a little vigilante bus management.
-			// In that case, we do a goto into the gap count logic
-			// so that when we do the reset, we still optimize the
-			// gap count.  That could well save a reset in the
-			// next generation.
-			__be32 data[2] = {
-				cpu_to_be32(BUS_MANAGER_ID_NOT_REGISTERED),
-				cpu_to_be32(local_id),
-			};
-			struct fw_device *irm_device = fw_node_get_device(card->irm_node);
-			bool irm_is_1394_1995_only = false;
-			bool keep_this_irm = false;
-			int rcode;
-
-			if (!card->irm_node->link_on) {
-				new_root_id = local_id;
-				fw_notice(card, "%s, making local node (%02x) root\n",
-					  "IRM has link off", new_root_id);
-				goto pick_me;
-			}
+		enum bm_contention_outcome result = contend_for_bm(card);
 
-			if (irm_device && irm_device->config_rom) {
-				irm_is_1394_1995_only = (irm_device->config_rom[2] & 0x000000f0) == 0;
-
-				// Canon MV5i works unreliably if it is not root node.
-				keep_this_irm = irm_device->config_rom[3] >> 8 == CANON_OUI;
-			}
-
-			if (irm_is_1394_1995_only && !keep_this_irm) {
-				new_root_id = local_id;
-				fw_notice(card, "%s, making local node (%02x) root\n",
-					  "IRM is not 1394a compliant", new_root_id);
-				goto pick_me;
-			}
-
-			rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
-					irm_id, generation, SCODE_100,
-					CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
-					data, sizeof(data));
-
-			switch (rcode) {
-			case RCODE_GENERATION:
-				// Another bus reset, BM work has been rescheduled.
-				return;
-			case RCODE_SEND_ERROR:
-				// We have been unable to send the lock request due to
-				// some local problem.  Let's try again later and hope
-				// that the problem has gone away by then.
-				fw_schedule_bm_work(card, msecs_to_jiffies(125));
-				return;
-			case RCODE_COMPLETE:
-			{
-				int bm_id = be32_to_cpu(data[0]);
-
-				// 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) {
-					// Somebody else is BM.  Only act as IRM.
-					if (local_id == irm_id)
-						allocate_broadcast_channel(card, generation);
-					return;
-				}
-				break;
-			}
-			default:
-				if (!keep_this_irm) {
-					// The lock request failed, maybe the IRM
-					// isn't really IRM capable after all. Let's
-					// do a bus reset and pick the local node as
-					// root, and thus, IRM.
-					new_root_id = local_id;
-					fw_notice(card, "BM lock failed (%s), making local node (%02x) root\n",
-						  fw_rcode_string(rcode), new_root_id);
-					goto pick_me;
-				}
-				break;
-			}
-
-			// A node contends for bus manager in this generation.
-			card->bm_generation = generation;
-		} else {
-			// We weren't BM in the last generation, and the last
-			// bus reset is less than 125ms ago.  Reschedule this job.
+		switch (result) {
+		case BM_CONTENTION_OUTCOME_WITHIN_WINDOW:
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
+		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
+			// BM work has been rescheduled.
+			return;
+		case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION:
+			// Let's try again later and hope that the local problem has gone away by
+			// then.
+			fw_schedule_bm_work(card, msecs_to_jiffies(125));
+			return;
+		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
+			// Let's do a bus reset and pick the local node as root, and thus, IRM.
+			new_root_id = local_id;
+			goto pick_me;
+		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
+			if (local_id == irm_id) {
+				// Only acts as IRM.
+				allocate_broadcast_channel(card, generation);
+			}
+			fallthrough;
+		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
+		default:
+			card->bm_generation = generation;
+			break;
 		}
 	}
 
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/6] firewire: core; eliminate pick_me goto label
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (10 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  2025-09-18 23:54 ` [PATCH v2 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

This commit uses condition statements instead of pick_me goto label.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 6268b595d4fa..58d1f58a4a0f 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -388,6 +388,7 @@ static void bm_work(struct work_struct *work)
 	int root_id, new_root_id, irm_id, local_id;
 	int expected_gap_count, generation;
 	bool do_reset = false;
+	bool stand_for_root = false;
 
 	if (card->local_node == NULL)
 		return;
@@ -408,11 +409,11 @@ static void bm_work(struct work_struct *work)
 			fw_schedule_bm_work(card, msecs_to_jiffies(125));
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
 			// BM work has been rescheduled.
 			return;
@@ -423,8 +424,8 @@ static void bm_work(struct work_struct *work)
 			return;
 		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
 			// Let's do a bus reset and pick the local node as root, and thus, IRM.
-			new_root_id = local_id;
-			goto pick_me;
+			stand_for_root = true;
+			break;
 		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
 			if (local_id == irm_id) {
 				// Only acts as IRM.
@@ -438,56 +439,56 @@ static void bm_work(struct work_struct *work)
 		}
 	}
 
-	/*
-	 * We're bus manager for this generation, so next step is to
-	 * make sure we have an active cycle master and do gap count
-	 * optimization.
-	 */
-	if (card->gap_count == GAP_COUNT_MISMATCHED) {
-		/*
-		 * If self IDs have inconsistent gap counts, do a
-		 * bus reset ASAP. The config rom read might never
-		 * complete, so don't wait for it. However, still
-		 * send a PHY configuration packet prior to the
-		 * bus reset. The PHY configuration packet might
-		 * fail, but 1394-2008 8.4.5.2 explicitly permits
-		 * it in this case, so it should be safe to try.
-		 */
-		new_root_id = local_id;
-		/*
-		 * We must always send a bus reset if the gap count
-		 * is inconsistent, so bypass the 5-reset limit.
-		 */
-		card->bm_retries = 0;
-	} else {
-		// Now investigate root node.
-		struct fw_device *root_device = fw_node_get_device(root_node);
-
-		if (root_device == NULL) {
-			// Either link_on is false, or we failed to read the
-			// config rom.  In either case, pick another root.
-			new_root_id = local_id;
+	// We're bus manager for this generation, so next step is to make sure we have an active
+	// cycle master and do gap count optimization.
+	if (!stand_for_root) {
+		if (card->gap_count == GAP_COUNT_MISMATCHED) {
+			// If self IDs have inconsistent gap counts, do a
+			// bus reset ASAP. The config rom read might never
+			// complete, so don't wait for it. However, still
+			// send a PHY configuration packet prior to the
+			// bus reset. The PHY configuration packet might
+			// fail, but 1394-2008 8.4.5.2 explicitly permits
+			// it in this case, so it should be safe to try.
+			stand_for_root = true;
+
+			// We must always send a bus reset if the gap count
+			// is inconsistent, so bypass the 5-reset limit.
+			card->bm_retries = 0;
 		} else {
-			bool root_device_is_running =
-				atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
+			// Now investigate root node.
+			struct fw_device *root_device = fw_node_get_device(root_node);
 
-			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.
-				return;
-			} else if (root_device->cmc) {
-				// We will send out a force root packet for this
-				// node as part of the gap count optimization.
-				new_root_id = root_id;
+			if (root_device == NULL) {
+				// Either link_on is false, or we failed to read the
+				// config rom.  In either case, pick another root.
+				stand_for_root = true;
 			} else {
-				// Current root has an active link layer and we
-				// successfully read the config rom, but it's not
-				// cycle master capable.
-				new_root_id = local_id;
+				bool root_device_is_running =
+					atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
+
+				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.
+					return;
+				} else if (!root_device->cmc) {
+					// Current root has an active link layer and we
+					// successfully read the config rom, but it's not
+					// cycle master capable.
+					stand_for_root = true;
+				}
 			}
 		}
 	}
- pick_me:
+
+	if (stand_for_root) {
+		new_root_id = local_id;
+	} else {
+		// We will send out a force root packet for this node as part of the gap count
+		// optimization on behalf of the node.
+		new_root_id = root_id;
+	}
+
 	/*
 	 * Pick a gap count from 1394a table E-1.  The table doesn't cover
 	 * the typically much larger 1394b beta repeater delays though.
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/6] firewire: core: minor code refactoring to delete useless local variable
  2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
                   ` (11 preceding siblings ...)
  2025-09-18 23:54 ` [PATCH v2 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
@ 2025-09-18 23:54 ` Takashi Sakamoto
  12 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-18 23:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

The do_reset local variable has less merit. Let's remove it.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 58d1f58a4a0f..4a5459696093 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -387,7 +387,6 @@ static void bm_work(struct work_struct *work)
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, local_id;
 	int expected_gap_count, generation;
-	bool do_reset = false;
 	bool stand_for_root = false;
 
 	if (card->local_node == NULL)
@@ -499,16 +498,10 @@ static void bm_work(struct work_struct *work)
 	else
 		expected_gap_count = 63;
 
-	/*
-	 * Finally, figure out if we should do a reset or not.  If we have
-	 * done less than 5 resets with the same physical topology and we
-	 * have either a new root or a new gap count setting, let's do it.
-	 */
-
-	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id))
-		do_reset = true;
-
-	if (do_reset) {
+	// Finally, figure out if we should do a reset or not. If we have done less than 5 resets
+	// with the same physical topology and we have either a new root or a new gap count
+	// setting, let's do it.
+	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id)) {
 		int card_gap_count = card->gap_count;
 
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/6] firewire: core; eliminate pick_me goto label
  2025-09-18 23:08 ` [PATCH 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
  2025-09-18 23:46   ` Takashi Sakamoto
@ 2025-09-19 17:52   ` kernel test robot
  1 sibling, 0 replies; 17+ messages in thread
From: kernel test robot @ 2025-09-19 17:52 UTC (permalink / raw)
  To: Takashi Sakamoto, linux1394-devel; +Cc: llvm, oe-kbuild-all, linux-kernel

Hi Takashi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on e6d2338b6f3e522872f3a14fcc5e5de2f58bf23b]

url:    https://github.com/intel-lab-lkp/linux/commits/Takashi-Sakamoto/firewire-core-remove-useless-generation-check/20250919-115832
base:   e6d2338b6f3e522872f3a14fcc5e5de2f58bf23b
patch link:    https://lore.kernel.org/r/20250918230857.127400-6-o-takashi%40sakamocchi.jp
patch subject: [PATCH 5/6] firewire: core; eliminate pick_me goto label
config: x86_64-randconfig-001-20250919 (https://download.01.org/0day-ci/archive/20250920/202509200157.ZsFKlqm4-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250920/202509200157.ZsFKlqm4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509200157.ZsFKlqm4-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/firewire/core-card.c:404:6: warning: variable 'stand_for_root' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     404 |         if (card->bm_generation != generation) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firewire/core-card.c:445:7: note: uninitialized use occurs here
     445 |         if (!stand_for_root) {
         |              ^~~~~~~~~~~~~~
   drivers/firewire/core-card.c:404:2: note: remove the 'if' if its condition is always true
     404 |         if (card->bm_generation != generation) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/firewire/core-card.c:391:21: note: initialize the variable 'stand_for_root' to silence this warning
     391 |         bool stand_for_root;
         |                            ^
         |                             = 0
   1 warning generated.


vim +404 drivers/firewire/core-card.c

e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  380  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  381  static void bm_work(struct work_struct *work)
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  382  {
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  383  	static const char gap_count_table[] = {
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  384  		63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  385  	};
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  386  	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  387  	struct fw_node *root_node __free(node_unref) = NULL;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  388  	int root_id, new_root_id, irm_id, local_id;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  389  	int expected_gap_count, generation;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  390  	bool do_reset = false;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  391  	bool stand_for_root;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  392  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  393  	if (card->local_node == NULL)
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  394  		return;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  395  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  396  	generation = card->generation;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  397  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  398  	root_node = fw_node_get(card->root_node);
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  399  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  400  	root_id  = root_node->node_id;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  401  	irm_id   = card->irm_node->node_id;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  402  	local_id = card->local_node->node_id;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  403  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19 @404  	if (card->bm_generation != generation) {
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  405  		enum bm_contention_outcome result = contend_for_bm(card);
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  406  
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  407  		switch (result) {
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  408  		case BM_CONTENTION_OUTCOME_WITHIN_WINDOW:
379b870c28c6a6 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-15  409  			fw_schedule_bm_work(card, msecs_to_jiffies(125));
25feb1a96e21ae drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  410  			return;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  411  		case BM_CONTENTION_OUTCOME_IRM_HAS_LINK_OFF:
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  412  			stand_for_root = true;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  413  			break;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  414  		case BM_CONTENTION_OUTCOME_IRM_COMPLIES_1394_1995_ONLY:
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  415  			stand_for_root = true;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  416  			break;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  417  		case BM_CONTENTION_OUTCOME_AT_NEW_GENERATION:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  418  			// BM work has been rescheduled.
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  419  			return;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  420  		case BM_CONTENTION_OUTCOME_LOCAL_PROBLEM_AT_TRANSACTION:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  421  			// Let's try again later and hope that the local problem has gone away by
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  422  			// then.
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  423  			fw_schedule_bm_work(card, msecs_to_jiffies(125));
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  424  			return;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  425  		case BM_CONTENTION_OUTCOME_IRM_IS_NOT_CAPABLE_FOR_IRM:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  426  			// Let's do a bus reset and pick the local node as root, and thus, IRM.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  427  			stand_for_root = true;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  428  			break;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  429  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_ANOTHER_NODE_AS_BM:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  430  			if (local_id == irm_id) {
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  431  				// Only acts as IRM.
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  432  				allocate_broadcast_channel(card, generation);
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  433  			}
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  434  			fallthrough;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  435  		case BM_CONTENTION_OUTCOME_IRM_HOLDS_LOCAL_NODE_AS_BM:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  436  		default:
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  437  			card->bm_generation = generation;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  438  			stand_for_root = false;
e309c29e1b26f4 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  439  			break;
931c4834c8d1e1 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-01-26  440  		}
ff94548bbb2edf drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  441  	}
931c4834c8d1e1 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-01-26  442  
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  443  	// We're bus manager for this generation, so next step is to make sure we have an active
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  444  	// cycle master and do gap count optimization.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  445  	if (!stand_for_root) {
91bf158f8cdf6f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-13  446  		if (card->gap_count == GAP_COUNT_MISMATCHED) {
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  447  			// If self IDs have inconsistent gap counts, do a
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  448  			// bus reset ASAP. The config rom read might never
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  449  			// complete, so don't wait for it. However, still
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  450  			// send a PHY configuration packet prior to the
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  451  			// bus reset. The PHY configuration packet might
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  452  			// fail, but 1394-2008 8.4.5.2 explicitly permits
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  453  			// it in this case, so it should be safe to try.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  454  			stand_for_root = true;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  455  
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  456  			// We must always send a bus reset if the gap count
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  457  			// is inconsistent, so bypass the 5-reset limit.
7ed4380009e96d drivers/firewire/core-card.c Takashi Sakamoto  2024-02-07  458  			card->bm_retries = 0;
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  459  		} else {
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  460  			// Now investigate root node.
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  461  			struct fw_device *root_device = fw_node_get_device(root_node);
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  462  
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  463  			if (root_device == NULL) {
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  464  				// Either link_on is false, or we failed to read the
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  465  				// config rom.  In either case, pick another root.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  466  				stand_for_root = true;
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  467  			} else {
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  468  				bool root_device_is_running =
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  469  					atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  470  
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  471  				if (!root_device_is_running) {
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  472  					// If we haven't probed this device yet, bail out now
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  473  					// and let's try again once that's done.
25feb1a96e21ae drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  474  					return;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  475  				} else if (!root_device->cmc) {
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  476  					// Current root has an active link layer and we
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  477  					// successfully read the config rom, but it's not
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  478  					// cycle master capable.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  479  					stand_for_root = true;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  480  				}
83db801ce8c644 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-01-26  481  			}
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  482  		}
cae2d92cdcae3f drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  483  	}
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  484  
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  485  	if (stand_for_root) {
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  486  		new_root_id = local_id;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  487  	} else {
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  488  		// We will send out a force root packet for this node as part of the gap count
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  489  		// optimization on behalf of the node.
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  490  		new_root_id = root_id;
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  491  	}
de5d138456fb29 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-19  492  
24d40125f1f59a drivers/firewire/fw-card.c   Stefan Richter    2007-06-18  493  	/*
24d40125f1f59a drivers/firewire/fw-card.c   Stefan Richter    2007-06-18  494  	 * Pick a gap count from 1394a table E-1.  The table doesn't cover
24d40125f1f59a drivers/firewire/fw-card.c   Stefan Richter    2007-06-18  495  	 * the typically much larger 1394b beta repeater delays though.
24d40125f1f59a drivers/firewire/fw-card.c   Stefan Richter    2007-06-18  496  	 */
24d40125f1f59a drivers/firewire/fw-card.c   Stefan Richter    2007-06-18  497  	if (!card->beta_repeaters_present &&
15803478fdea96 drivers/firewire/fw-card.c   Stefan Richter    2008-02-24  498  	    root_node->max_hops < ARRAY_SIZE(gap_count_table))
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  499  		expected_gap_count = gap_count_table[root_node->max_hops];
83db801ce8c644 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-01-26  500  	else
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  501  		expected_gap_count = 63;
83db801ce8c644 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-01-26  502  
c781c06d119d04 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-05-07  503  	/*
25b1c3d8889f98 drivers/firewire/fw-card.c   Stefan Richter    2008-03-24  504  	 * Finally, figure out if we should do a reset or not.  If we have
25b1c3d8889f98 drivers/firewire/fw-card.c   Stefan Richter    2008-03-24  505  	 * done less than 5 resets with the same physical topology and we
c781c06d119d04 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-05-07  506  	 * have either a new root or a new gap count setting, let's do it.
c781c06d119d04 drivers/firewire/fw-card.c   Kristian Høgsberg 2007-05-07  507  	 */
19a15b937b2663 drivers/firewire/fw-card.c   Kristian Høgsberg 2006-12-19  508  
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  509  	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id))
25b1c3d8889f98 drivers/firewire/fw-card.c   Stefan Richter    2008-03-24  510  		do_reset = true;
19a15b937b2663 drivers/firewire/fw-card.c   Kristian Høgsberg 2006-12-19  511  
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  512  	if (do_reset) {
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  513  		int card_gap_count = card->gap_count;
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  514  
26b4950de174bc drivers/firewire/core-card.c Stefan Richter    2012-02-18  515  		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  516  			  new_root_id, expected_gap_count);
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  517  		fw_send_phy_config(card, new_root_id, generation, expected_gap_count);
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  518  		/*
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  519  		 * Where possible, use a short bus reset to minimize
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  520  		 * disruption to isochronous transfers. But in the event
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  521  		 * of a gap count inconsistency, use a long bus reset.
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  522  		 *
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  523  		 * As noted in 1394a 8.4.6.2, nodes on a mixed 1394/1394a bus
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  524  		 * may set different gap counts after a bus reset. On a mixed
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  525  		 * 1394/1394a bus, a short bus reset can get doubled. Some
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  526  		 * nodes may treat the double reset as one bus reset and others
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  527  		 * may treat it as two, causing a gap count inconsistency
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  528  		 * again. Using a long bus reset prevents this.
d0b06dc48fb159 drivers/firewire/core-card.c Takashi Sakamoto  2024-02-29  529  		 */
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  530  		reset_bus(card, card_gap_count != 0);
cbae787c0f288c drivers/firewire/fw-card.c   Stefan Richter    2009-03-10  531  		/* Will allocate broadcast channel after the reset. */
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  532  	} else {
a4bac55d99d379 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  533  		struct fw_device *root_device = fw_node_get_device(root_node);
a4bac55d99d379 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  534  
a4bac55d99d379 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  535  		if (root_device && root_device->cmc) {
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  536  			// Make sure that the cycle master sends cycle start packets.
b70a5f33381f78 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  537  			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
b70a5f33381f78 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  538  			int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
c374ab424249b6 drivers/firewire/core-card.c Clemens Ladisch   2010-06-10  539  					root_id, generation, SCODE_100,
c374ab424249b6 drivers/firewire/core-card.c Clemens Ladisch   2010-06-10  540  					CSR_REGISTER_BASE + CSR_STATE_SET,
b70a5f33381f78 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  541  					&data, sizeof(data));
c374ab424249b6 drivers/firewire/core-card.c Clemens Ladisch   2010-06-10  542  			if (rcode == RCODE_GENERATION)
25feb1a96e21ae drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  543  				return;
c374ab424249b6 drivers/firewire/core-card.c Clemens Ladisch   2010-06-10  544  		}
c374ab424249b6 drivers/firewire/core-card.c Clemens Ladisch   2010-06-10  545  
cbae787c0f288c drivers/firewire/fw-card.c   Stefan Richter    2009-03-10  546  		if (local_id == irm_id)
cbae787c0f288c drivers/firewire/fw-card.c   Stefan Richter    2009-03-10  547  			allocate_broadcast_channel(card, generation);
19a15b937b2663 drivers/firewire/fw-card.c   Kristian Høgsberg 2006-12-19  548  	}
8c2d2fcd6b7934 drivers/firewire/core-card.c Takashi Sakamoto  2025-09-08  549  }
19a15b937b2663 drivers/firewire/fw-card.c   Kristian Høgsberg 2006-12-19  550  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager
  2025-09-18 23:54 ` [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
@ 2025-09-20  1:54   ` Takashi Sakamoto
  0 siblings, 0 replies; 17+ messages in thread
From: Takashi Sakamoto @ 2025-09-20  1:54 UTC (permalink / raw)
  To: linux1394-devel; +Cc: linux-kernel

On Fri, Sep 19, 2025 at 08:54:42AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This patchset is the revised version of my previous one:
> https://lore.kernel.org/lkml/20250918230857.127400-1-o-takashi@sakamocchi.jp/
> 
> Changes from v1:
> * Ensure to initialize local variable
> 
> Takashi Sakamoto (6):
>   firewire: core: remove useless generation check
>   firewire: core: use switch statement to evaluate transaction result to
>     CSR_BUS_MANAGER_ID
>   firewire: core: code refactoring for the case of generation mismatch
>   firewire: core: code refactoring to split contention procedure for bus
>     manager
>   firewire: core; eliminate pick_me goto label
>   firewire: core: minor code refactoring to delete useless local
>     variable
> 
>  drivers/firewire/core-card.c | 335 ++++++++++++++++++-----------------
>  1 file changed, 177 insertions(+), 158 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2025-09-20  1:54 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-18 23:08 [PATCH 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
2025-09-18 23:08 ` [PATCH 1/6] firewire: core: remove useless generation check Takashi Sakamoto
2025-09-18 23:08 ` [PATCH 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
2025-09-18 23:08 ` [PATCH 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
2025-09-18 23:08 ` [PATCH 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
2025-09-18 23:08 ` [PATCH 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
2025-09-18 23:46   ` Takashi Sakamoto
2025-09-19 17:52   ` kernel test robot
2025-09-18 23:08 ` [PATCH 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 0/6] firewire: core: code refactoring for work item of bus manager Takashi Sakamoto
2025-09-20  1:54   ` Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 1/6] firewire: core: remove useless generation check Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 2/6] firewire: core: use switch statement to evaluate transaction result to CSR_BUS_MANAGER_ID Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 3/6] firewire: core: code refactoring for the case of generation mismatch Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 4/6] firewire: core: code refactoring to split contention procedure for bus manager Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 5/6] firewire: core; eliminate pick_me goto label Takashi Sakamoto
2025-09-18 23:54 ` [PATCH v2 6/6] firewire: core: minor code refactoring to delete useless local variable Takashi Sakamoto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox