* [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 [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 [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 [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 [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 [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* 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
* [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 [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* 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
* [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 [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 [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 [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 [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 [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 [flat|nested] 17+ messages in thread