[PATCH 00/11] firewire: code refactoring mainly for bm_work

[PATCH 00/11] firewire: code refactoring mainly for bm_work

[Takashi Sakamoto]

Hi,

The core function in this subsystem have bm_work work item. The
corresponding function has many lines with comments. It is a sign
that it is time to be refactored, in my experience.

This series includes the first take for the purpose, as well as the
other code improvements of 1394 OHCI PCI driver and the other core
functions.

Takashi Sakamoto (11):
  firewire: ohci: use kcalloc() variant for array allocation
  firewire: core: utilize cleanup function to release workqueue in error
    path
  firewire: ohci: use return value from fw_node_get()
  firewire: core: add helper functions to access to fw_device data in
    fw_node structure
  firewire: core: use cleanup function in bm_work
  firewire: ohci: localize transaction data and rcode per condition
    branch
  firewire: core: code refactoring to evaluate transaction result to
    CSR_BUS_MANAGER_ID
  firewire: core: refer fw_card member to initiate bus reset under
    acquiring lock
  firewire: core: code refactoring to detect both IEEE 1394:1995 IRM and
    Canon MV5i
  firewire: core: code refactoring to investigate root node for bus
    manager
  firewire: core: code refactoring whether root node is cycle master
    capable

 drivers/firewire/core-card.c   | 239 +++++++++++++++++----------------
 drivers/firewire/core-device.c |  18 +--
 drivers/firewire/core.h        |  14 +-
 drivers/firewire/ohci.c        |   7 +-
 4 files changed, 143 insertions(+), 135 deletions(-)


base-commit: a901f493d06631091bf1f644fdbb5cb4f566645d
-- 
2.48.1

[PATCH 01/11] firewire: ohci: use kcalloc() variant for array allocation

[Takashi Sakamoto]

When allocating the list of isochronous context structure, a kzalloc()
variant of managed device API is used. In this case, a kcalloc() variant
is available.

This commit replaces these lines with devm_kcalloc().

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

diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
index 40851b120615..030aed5453a1 100644
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -3482,7 +3482,6 @@ static int pci_probe(struct pci_dev *dev,
 	u32 bus_options, max_receive, link_speed, version;
 	u64 guid;
 	int i, flags, irq, err;
-	size_t size;
 
 	if (dev->vendor == PCI_VENDOR_ID_PINNACLE_SYSTEMS) {
 		dev_err(&dev->dev, "Pinnacle MovieBoard is not yet supported\n");
@@ -3576,8 +3575,7 @@ static int pci_probe(struct pci_dev *dev,
 	reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0);
 	ohci->ir_context_mask = ohci->ir_context_support;
 	ohci->n_ir = hweight32(ohci->ir_context_mask);
-	size = sizeof(struct iso_context) * ohci->n_ir;
-	ohci->ir_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL);
+	ohci->ir_context_list = devm_kcalloc(&dev->dev, ohci->n_ir, sizeof(struct iso_context), GFP_KERNEL);
 	if (!ohci->ir_context_list)
 		return -ENOMEM;
 
@@ -3591,8 +3589,7 @@ static int pci_probe(struct pci_dev *dev,
 	reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, ~0);
 	ohci->it_context_mask = ohci->it_context_support;
 	ohci->n_it = hweight32(ohci->it_context_mask);
-	size = sizeof(struct iso_context) * ohci->n_it;
-	ohci->it_context_list = devm_kzalloc(&dev->dev, size, GFP_KERNEL);
+	ohci->it_context_list = devm_kcalloc(&dev->dev, ohci->n_it, sizeof(struct iso_context), GFP_KERNEL);
 	if (!ohci->it_context_list)
 		return -ENOMEM;
 
-- 
2.48.1

[PATCH 02/11] firewire: core: utilize cleanup function to release workqueue in error path

[Takashi Sakamoto]

The helper macro, retain_and_null_ptr(), introduced by a commit
092d00ead733 ("cleanup: Provide retain_and_null_ptr()") in v6.16 kernel,
is useful in the error path to release the part of structure member.

This commit uses the relatively new function.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index aae774e7a5c3..d128c7a8bf5f 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -570,9 +570,13 @@ void fw_card_initialize(struct fw_card *card,
 }
 EXPORT_SYMBOL(fw_card_initialize);
 
+DEFINE_FREE(workqueue_destroy, struct workqueue_struct *, if (_T) destroy_workqueue(_T))
+
 int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
 		unsigned int supported_isoc_contexts)
 {
+	struct workqueue_struct *isoc_wq __free(workqueue_destroy) = NULL;
+	struct workqueue_struct *async_wq __free(workqueue_destroy) = NULL;
 	int ret;
 
 	// This workqueue should be:
@@ -587,10 +591,10 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
 	//  * == WQ_SYSFS		Parameters are available via sysfs.
 	//  * max_active == n_it + n_ir	A hardIRQ could notify events for multiple isochronous
 	//				contexts if they are scheduled to the same cycle.
-	card->isoc_wq = alloc_workqueue("firewire-isoc-card%u",
-					WQ_UNBOUND | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS,
-					supported_isoc_contexts, card->index);
-	if (!card->isoc_wq)
+	isoc_wq = alloc_workqueue("firewire-isoc-card%u",
+				  WQ_UNBOUND | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS,
+				  supported_isoc_contexts, card->index);
+	if (!isoc_wq)
 		return -ENOMEM;
 
 	// This workqueue should be:
@@ -602,14 +606,14 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
 	//  * == WQ_SYSFS		Parameters are available via sysfs.
 	//  * max_active == 4		A hardIRQ could notify events for a pair of requests and
 	//				response AR/AT contexts.
-	card->async_wq = alloc_workqueue("firewire-async-card%u",
-					 WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS,
-					 4, card->index);
-	if (!card->async_wq) {
-		ret = -ENOMEM;
-		goto err_isoc;
-	}
+	async_wq = alloc_workqueue("firewire-async-card%u",
+				   WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI | WQ_SYSFS,
+				   4, card->index);
+	if (!async_wq)
+		return -ENOMEM;
 
+	card->isoc_wq = isoc_wq;
+	card->async_wq = async_wq;
 	card->max_receive = max_receive;
 	card->link_speed = link_speed;
 	card->guid = guid;
@@ -617,18 +621,18 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid,
 	scoped_guard(mutex, &card_mutex) {
 		generate_config_rom(card, tmp_config_rom);
 		ret = card->driver->enable(card, tmp_config_rom, config_rom_length);
-		if (ret < 0)
-			goto err_async;
+		if (ret < 0) {
+			card->isoc_wq = NULL;
+			card->async_wq = NULL;
+			return ret;
+		}
+		retain_and_null_ptr(isoc_wq);
+		retain_and_null_ptr(async_wq);
 
 		list_add_tail(&card->link, &card_list);
 	}
 
 	return 0;
-err_async:
-	destroy_workqueue(card->async_wq);
-err_isoc:
-	destroy_workqueue(card->isoc_wq);
-	return ret;
 }
 EXPORT_SYMBOL(fw_card_add);
 
-- 
2.48.1

[PATCH 03/11] firewire: ohci: use return value from fw_node_get()

[Takashi Sakamoto]

The programming pattern, referring after increasing reference count, is
supported by fw_node_get().

This commit simplify the programming pattern.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index d128c7a8bf5f..41902dcc10a0 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -306,8 +306,7 @@ static void bm_work(struct work_struct *work)
 
 	generation = card->generation;
 
-	root_node = card->root_node;
-	fw_node_get(root_node);
+	root_node = fw_node_get(card->root_node);
 	root_device = root_node->data;
 	root_device_is_running = root_device &&
 			atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
-- 
2.48.1

[PATCH 04/11] firewire: core: add helper functions to access to fw_device data in fw_node structure

[Takashi Sakamoto]

The data mbmer in fw_node structure is an opaque pointer, while nowadays
it is just used to refer to fw_device associated with the fw_node.

This commit redefines the opaque pointer to a pointer to fw_device
structure, and adds some helper functions to set/get it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-card.c   |  4 ++--
 drivers/firewire/core-device.c | 18 +++++++++---------
 drivers/firewire/core.h        | 14 ++++++++++++--
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 41902dcc10a0..4a4210cda571 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -307,12 +307,12 @@ static void bm_work(struct work_struct *work)
 	generation = card->generation;
 
 	root_node = fw_node_get(card->root_node);
-	root_device = root_node->data;
+	root_device = fw_node_get_device(root_node);
 	root_device_is_running = root_device &&
 			atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
 	root_device_is_cmc = root_device && root_device->cmc;
 
-	irm_device = card->irm_node->data;
+	irm_device = fw_node_get_device(card->irm_node);
 	irm_is_1394_1995_only = irm_device && irm_device->config_rom &&
 			(irm_device->config_rom[2] & 0x000000f0) == 0;
 
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index aeacd4cfd694..6a04a0014694 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -887,7 +887,7 @@ static void fw_device_release(struct device *dev)
 	 * bus manager work looks at this node.
 	 */
 	scoped_guard(spinlock_irqsave, &card->lock)
-		device->node->data = NULL;
+		fw_node_set_device(device->node, NULL);
 
 	fw_node_put(device->node);
 	kfree(device->config_rom);
@@ -1007,7 +1007,7 @@ static void fw_device_init(struct work_struct *work)
 	int ret;
 
 	/*
-	 * All failure paths here set node->data to NULL, so that we
+	 * All failure paths here call fw_node_set_device(node, NULL), so that we
 	 * don't try to do device_for_each_child() on a kfree()'d
 	 * device.
 	 */
@@ -1051,9 +1051,9 @@ static void fw_device_init(struct work_struct *work)
 				struct fw_node *obsolete_node = reused->node;
 
 				device->node = obsolete_node;
-				device->node->data = device;
+				fw_node_set_device(device->node, device);
 				reused->node = current_node;
-				reused->node->data = reused;
+				fw_node_set_device(reused->node, reused);
 
 				reused->max_speed = device->max_speed;
 				reused->node_id = current_node->node_id;
@@ -1292,7 +1292,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 		 * FW_NODE_UPDATED callbacks can update the node_id
 		 * and generation for the device.
 		 */
-		node->data = device;
+		fw_node_set_device(node, device);
 
 		/*
 		 * Many devices are slow to respond after bus resets,
@@ -1307,7 +1307,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 
 	case FW_NODE_INITIATED_RESET:
 	case FW_NODE_LINK_ON:
-		device = node->data;
+		device = fw_node_get_device(node);
 		if (device == NULL)
 			goto create;
 
@@ -1324,7 +1324,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 		break;
 
 	case FW_NODE_UPDATED:
-		device = node->data;
+		device = fw_node_get_device(node);
 		if (device == NULL)
 			break;
 
@@ -1339,7 +1339,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 
 	case FW_NODE_DESTROYED:
 	case FW_NODE_LINK_OFF:
-		if (!node->data)
+		if (!fw_node_get_device(node))
 			break;
 
 		/*
@@ -1354,7 +1354,7 @@ void fw_node_event(struct fw_card *card, struct fw_node *node, int event)
 		 * the device in shutdown state to have that code fail
 		 * to create the device.
 		 */
-		device = node->data;
+		device = fw_node_get_device(node);
 		if (atomic_xchg(&device->state,
 				FW_DEVICE_GONE) == FW_DEVICE_RUNNING) {
 			device->workfn = fw_device_shutdown;
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h
index 9b298af1cac0..083e39034c37 100644
--- a/drivers/firewire/core.h
+++ b/drivers/firewire/core.h
@@ -194,8 +194,8 @@ struct fw_node {
 	/* For serializing node topology into a list. */
 	struct list_head link;
 
-	/* Upper layer specific data. */
-	void *data;
+	// The device when already associated, else NULL.
+	struct fw_device *device;
 
 	struct fw_node *ports[] __counted_by(port_count);
 };
@@ -219,6 +219,16 @@ static inline void fw_node_put(struct fw_node *node)
 	kref_put(&node->kref, release_node);
 }
 
+static inline struct fw_device *fw_node_get_device(struct fw_node *node)
+{
+	return node->device;
+}
+
+static inline void fw_node_set_device(struct fw_node *node, struct fw_device *device)
+{
+	node->device = device;
+}
+
 void fw_core_handle_bus_reset(struct fw_card *card, int node_id,
 	int generation, int self_id_count, u32 *self_ids, bool bm_abdicate);
 void fw_destroy_nodes(struct fw_card *card);
-- 
2.48.1

[PATCH 05/11] firewire: core: use cleanup function in bm_work

[Takashi Sakamoto]

In "bm_work" function, the references to fw_card and fw_node are
released at last. This is achieved by using goto statements. For this
case, the kernel cleanup framework is available.

This commit uses the framework to remove these statements.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 4a4210cda571..5bd89ddf5018 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -280,14 +280,17 @@ void fw_schedule_bm_work(struct fw_card *card, unsigned long delay)
 		fw_card_put(card);
 }
 
+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))
+
 static void bm_work(struct work_struct *work)
 {
 	static const char gap_count_table[] = {
 		63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40
 	};
-	struct fw_card *card = from_work(card, work, bm_work.work);
+	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
 	struct fw_device *root_device, *irm_device;
-	struct fw_node *root_node;
+	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, bm_id, local_id;
 	int gap_count, generation, grace, rcode;
 	bool do_reset = false;
@@ -297,11 +300,13 @@ static void bm_work(struct work_struct *work)
 	bool keep_this_irm;
 	__be32 transaction_data[2];
 
+	lockdep_assert_held(&card->lock);
+
 	spin_lock_irq(&card->lock);
 
 	if (card->local_node == NULL) {
 		spin_unlock_irq(&card->lock);
-		goto out_put_card;
+		return;
 	}
 
 	generation = card->generation;
@@ -366,9 +371,9 @@ static void bm_work(struct work_struct *work)
 				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
 				transaction_data, 8);
 
+		// Another bus reset, BM work has been rescheduled.
 		if (rcode == RCODE_GENERATION)
-			/* Another bus reset, BM work has been rescheduled. */
-			goto out;
+			return;
 
 		bm_id = be32_to_cpu(transaction_data[0]);
 
@@ -382,8 +387,7 @@ static void bm_work(struct work_struct *work)
 			/* Somebody else is BM.  Only act as IRM. */
 			if (local_id == irm_id)
 				allocate_broadcast_channel(card, generation);
-
-			goto out;
+			return;
 		}
 
 		if (rcode == RCODE_SEND_ERROR) {
@@ -393,7 +397,7 @@ static void bm_work(struct work_struct *work)
 			 * that the problem has gone away by then.
 			 */
 			fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8));
-			goto out;
+			return;
 		}
 
 		spin_lock_irq(&card->lock);
@@ -417,7 +421,7 @@ static void bm_work(struct work_struct *work)
 		 */
 		spin_unlock_irq(&card->lock);
 		fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8));
-		goto out;
+		return;
 	}
 
 	/*
@@ -455,7 +459,7 @@ static void bm_work(struct work_struct *work)
 		 * and let's try again once that's done.
 		 */
 		spin_unlock_irq(&card->lock);
-		goto out;
+		return;
 	} else if (root_device_is_cmc) {
 		/*
 		 * We will send out a force root packet for this
@@ -512,7 +516,7 @@ static void bm_work(struct work_struct *work)
 		 */
 		reset_bus(card, card->gap_count != 0);
 		/* Will allocate broadcast channel after the reset. */
-		goto out;
+		return;
 	}
 
 	if (root_device_is_cmc) {
@@ -525,16 +529,11 @@ static void bm_work(struct work_struct *work)
 				CSR_REGISTER_BASE + CSR_STATE_SET,
 				transaction_data, 4);
 		if (rcode == RCODE_GENERATION)
-			goto out;
+			return;
 	}
 
 	if (local_id == irm_id)
 		allocate_broadcast_channel(card, generation);
-
- out:
-	fw_node_put(root_node);
- out_put_card:
-	fw_card_put(card);
 }
 
 void fw_card_initialize(struct fw_card *card,
-- 
2.48.1

[PATCH 06/11] firewire: ohci: localize transaction data and rcode per condition branch

[Takashi Sakamoto]

The function local variable, transaction_data, in bm_work function is
conditionally used. In the case, the branch-level variable is sometimes
useful.

This commit uses this idea.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 5bd89ddf5018..b98797e4f1d4 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -292,13 +292,12 @@ static void bm_work(struct work_struct *work)
 	struct fw_device *root_device, *irm_device;
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, bm_id, local_id;
-	int gap_count, generation, grace, rcode;
+	int gap_count, generation, grace;
 	bool do_reset = false;
 	bool root_device_is_running;
 	bool root_device_is_cmc;
 	bool irm_is_1394_1995_only;
 	bool keep_this_irm;
-	__be32 transaction_data[2];
 
 	lockdep_assert_held(&card->lock);
 
@@ -346,6 +345,11 @@ static void bm_work(struct work_struct *work)
 		 * gap count.  That could well save a reset in the
 		 * next generation.
 		 */
+		__be32 data[2] = {
+			cpu_to_be32(0x3f),
+			cpu_to_be32(local_id),
+		};
+		int rcode;
 
 		if (!card->irm_node->link_on) {
 			new_root_id = local_id;
@@ -361,21 +365,18 @@ static void bm_work(struct work_struct *work)
 			goto pick_me;
 		}
 
-		transaction_data[0] = cpu_to_be32(0x3f);
-		transaction_data[1] = cpu_to_be32(local_id);
-
 		spin_unlock_irq(&card->lock);
 
 		rcode = fw_run_transaction(card, TCODE_LOCK_COMPARE_SWAP,
 				irm_id, generation, SCODE_100,
 				CSR_REGISTER_BASE + CSR_BUS_MANAGER_ID,
-				transaction_data, 8);
+				data, sizeof(data));
 
 		// Another bus reset, BM work has been rescheduled.
 		if (rcode == RCODE_GENERATION)
 			return;
 
-		bm_id = be32_to_cpu(transaction_data[0]);
+		bm_id = be32_to_cpu(data[0]);
 
 		scoped_guard(spinlock_irq, &card->lock) {
 			if (rcode == RCODE_COMPLETE && generation == card->generation)
@@ -523,11 +524,11 @@ static void bm_work(struct work_struct *work)
 		/*
 		 * Make sure that the cycle master sends cycle start packets.
 		 */
-		transaction_data[0] = cpu_to_be32(CSR_STATE_BIT_CMSTR);
-		rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
+		__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
+		int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
 				root_id, generation, SCODE_100,
 				CSR_REGISTER_BASE + CSR_STATE_SET,
-				transaction_data, 4);
+				&data, sizeof(data));
 		if (rcode == RCODE_GENERATION)
 			return;
 	}
-- 
2.48.1

[PATCH 07/11] firewire: core: code refactoring to evaluate transaction result to CSR_BUS_MANAGER_ID

[Takashi Sakamoto]

The call of bm_work should be done after acquiring spin lock of fw_card.
For asynchronous transaction, the lock should be released temporarily
due to event waiting.

A commit 27310d561622 ("firewire: core: use guard macro to maintain
properties of fw_card") applied scoped_guard() to the bm_work function,
however it looks hard to follow to the control flow.

This commit refactors the spin lock acquisition after the transaction.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index b98797e4f1d4..e1a7a151b109 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -291,7 +291,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_device *root_device, *irm_device;
 	struct fw_node *root_node __free(node_unref) = NULL;
-	int root_id, new_root_id, irm_id, bm_id, local_id;
+	int root_id, new_root_id, irm_id, local_id;
 	int gap_count, generation, grace;
 	bool do_reset = false;
 	bool root_device_is_running;
@@ -376,19 +376,22 @@ static void bm_work(struct work_struct *work)
 		if (rcode == RCODE_GENERATION)
 			return;
 
-		bm_id = be32_to_cpu(data[0]);
+		spin_lock_irq(&card->lock);
 
-		scoped_guard(spinlock_irq, &card->lock) {
-			if (rcode == RCODE_COMPLETE && generation == card->generation)
-				card->bm_node_id =
-				    bm_id == 0x3f ? local_id : 0xffc0 | bm_id;
-		}
+		if (rcode == RCODE_COMPLETE) {
+			int bm_id = be32_to_cpu(data[0]);
 
-		if (rcode == RCODE_COMPLETE && bm_id != 0x3f) {
-			/* Somebody else is BM.  Only act as IRM. */
-			if (local_id == irm_id)
-				allocate_broadcast_channel(card, generation);
-			return;
+			if (generation == card->generation)
+				card->bm_node_id = bm_id == 0x3f ? local_id : 0xffc0 | bm_id;
+
+			if (bm_id != 0x3f) {
+				spin_unlock_irq(&card->lock);
+
+				// Somebody else is BM.  Only act as IRM.
+				if (local_id == irm_id)
+					allocate_broadcast_channel(card, generation);
+				return;
+			}
 		}
 
 		if (rcode == RCODE_SEND_ERROR) {
@@ -397,12 +400,11 @@ static void bm_work(struct work_struct *work)
 			 * some local problem.  Let's try again later and hope
 			 * that the problem has gone away by then.
 			 */
+			spin_unlock_irq(&card->lock);
 			fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8));
 			return;
 		}
 
-		spin_lock_irq(&card->lock);
-
 		if (rcode != RCODE_COMPLETE && !keep_this_irm) {
 			/*
 			 * The lock request failed, maybe the IRM
-- 
2.48.1

[PATCH 08/11] firewire: core: refer fw_card member to initiate bus reset under acquiring lock

[Takashi Sakamoto]

The gap_count member of fw_card structure is referred when initiate bus
reset. This reference is done out of acquiring lock. This is not good.

This commit takes the reference within the acquiring lock, with
additional code refactoring.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index e1a7a151b109..630e229c9cc2 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -292,7 +292,7 @@ static void bm_work(struct work_struct *work)
 	struct fw_device *root_device, *irm_device;
 	struct fw_node *root_node __free(node_unref) = NULL;
 	int root_id, new_root_id, irm_id, local_id;
-	int gap_count, generation, grace;
+	int expected_gap_count, generation, grace;
 	bool do_reset = false;
 	bool root_device_is_running;
 	bool root_device_is_cmc;
@@ -485,9 +485,9 @@ static void bm_work(struct work_struct *work)
 	 */
 	if (!card->beta_repeaters_present &&
 	    root_node->max_hops < ARRAY_SIZE(gap_count_table))
-		gap_count = gap_count_table[root_node->max_hops];
+		expected_gap_count = gap_count_table[root_node->max_hops];
 	else
-		gap_count = 63;
+		expected_gap_count = 63;
 
 	/*
 	 * Finally, figure out if we should do a reset or not.  If we have
@@ -495,16 +495,17 @@ static void bm_work(struct work_struct *work)
 	 * have either a new root or a new gap count setting, let's do it.
 	 */
 
-	if (card->bm_retries++ < 5 &&
-	    (card->gap_count != gap_count || new_root_id != root_id))
+	if (card->bm_retries++ < 5 && (card->gap_count != expected_gap_count || new_root_id != root_id))
 		do_reset = true;
 
-	spin_unlock_irq(&card->lock);
-
 	if (do_reset) {
+		int card_gap_count = card->gap_count;
+
+		spin_unlock_irq(&card->lock);
+
 		fw_notice(card, "phy config: new root=%x, gap_count=%d\n",
-			  new_root_id, gap_count);
-		fw_send_phy_config(card, new_root_id, generation, gap_count);
+			  new_root_id, expected_gap_count);
+		fw_send_phy_config(card, new_root_id, generation, expected_gap_count);
 		/*
 		 * Where possible, use a short bus reset to minimize
 		 * disruption to isochronous transfers. But in the event
@@ -517,26 +518,25 @@ static void bm_work(struct work_struct *work)
 		 * may treat it as two, causing a gap count inconsistency
 		 * again. Using a long bus reset prevents this.
 		 */
-		reset_bus(card, card->gap_count != 0);
+		reset_bus(card, card_gap_count != 0);
 		/* Will allocate broadcast channel after the reset. */
-		return;
-	}
+	} else {
+		spin_unlock_irq(&card->lock);
 
-	if (root_device_is_cmc) {
-		/*
-		 * Make sure that the cycle master sends cycle start packets.
-		 */
-		__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
-		int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
-				root_id, generation, SCODE_100,
-				CSR_REGISTER_BASE + CSR_STATE_SET,
-				&data, sizeof(data));
-		if (rcode == RCODE_GENERATION)
-			return;
-	}
+		if (root_device_is_cmc) {
+			// Make sure that the cycle master sends cycle start packets.
+			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
+			int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
+					root_id, generation, SCODE_100,
+					CSR_REGISTER_BASE + CSR_STATE_SET,
+					&data, sizeof(data));
+			if (rcode == RCODE_GENERATION)
+				return;
+		}
 
-	if (local_id == irm_id)
-		allocate_broadcast_channel(card, generation);
+		if (local_id == irm_id)
+			allocate_broadcast_channel(card, generation);
+	}
 }
 
 void fw_card_initialize(struct fw_card *card,
-- 
2.48.1

[PATCH 09/11] firewire: core: code refactoring to detect both IEEE 1394:1995 IRM and Canon MV5i

[Takashi Sakamoto]

The detection of IEEE 1394:1995 and Canon MV5i is just required within
some of the condition branches. In this case, these check can be
capsulated within these branches.

This commit refactors the checks.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 630e229c9cc2..99aa98f195ba 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -289,15 +289,13 @@ static void bm_work(struct work_struct *work)
 		63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40
 	};
 	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
-	struct fw_device *root_device, *irm_device;
+	struct fw_device *root_device;
 	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;
 	bool do_reset = false;
 	bool root_device_is_running;
 	bool root_device_is_cmc;
-	bool irm_is_1394_1995_only;
-	bool keep_this_irm;
 
 	lockdep_assert_held(&card->lock);
 
@@ -316,14 +314,6 @@ static void bm_work(struct work_struct *work)
 			atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
 	root_device_is_cmc = root_device && root_device->cmc;
 
-	irm_device = fw_node_get_device(card->irm_node);
-	irm_is_1394_1995_only = irm_device && irm_device->config_rom &&
-			(irm_device->config_rom[2] & 0x000000f0) == 0;
-
-	/* Canon MV5i works unreliably if it is not root node. */
-	keep_this_irm = irm_device && irm_device->config_rom &&
-			irm_device->config_rom[3] >> 8 == CANON_OUI;
-
 	root_id  = root_node->node_id;
 	irm_id   = card->irm_node->node_id;
 	local_id = card->local_node->node_id;
@@ -349,6 +339,9 @@ static void bm_work(struct work_struct *work)
 			cpu_to_be32(0x3f),
 			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) {
@@ -358,6 +351,13 @@ static void bm_work(struct work_struct *work)
 			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 (irm_is_1394_1995_only && !keep_this_irm) {
 			new_root_id = local_id;
 			fw_notice(card, "%s, making local node (%02x) root\n",
-- 
2.48.1

[PATCH 10/11] firewire: core: code refactoring to investigate root node for bus manager

[Takashi Sakamoto]

In the middle of bm_work function, both the value of gap_count and the
state of root node are investigated. Current implementation is not a good
shape since the investigation is aligned to be flat.

This commit refactors the investigation with two large branches.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 99aa98f195ba..b430a70a7eeb 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -294,7 +294,6 @@ static void bm_work(struct work_struct *work)
 	int root_id, new_root_id, irm_id, local_id;
 	int expected_gap_count, generation, grace;
 	bool do_reset = false;
-	bool root_device_is_running;
 	bool root_device_is_cmc;
 
 	lockdep_assert_held(&card->lock);
@@ -310,8 +309,6 @@ static void bm_work(struct work_struct *work)
 
 	root_node = fw_node_get(card->root_node);
 	root_device = fw_node_get_device(root_node);
-	root_device_is_running = root_device &&
-			atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
 	root_device_is_cmc = root_device && root_device->cmc;
 
 	root_id  = root_node->node_id;
@@ -450,34 +447,35 @@ static void bm_work(struct work_struct *work)
 		 * is inconsistent, so bypass the 5-reset limit.
 		 */
 		card->bm_retries = 0;
-	} else 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;
-	} else if (!root_device_is_running) {
-		/*
-		 * If we haven't probed this device yet, bail out now
-		 * and let's try again once that's done.
-		 */
-		spin_unlock_irq(&card->lock);
-		return;
-	} else if (root_device_is_cmc) {
-		/*
-		 * We will send out a force root packet for this
-		 * node as part of the gap count optimization.
-		 */
-		new_root_id = root_id;
 	} 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;
-	}
+		// 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;
+		} else {
+			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.
+				spin_unlock_irq(&card->lock);
+				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;
+			} 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;
+			}
+		}
+	}
  pick_me:
 	/*
 	 * Pick a gap count from 1394a table E-1.  The table doesn't cover
-- 
2.48.1

[PATCH 11/11] firewire: core: code refactoring whether root node is cycle master capable

[Takashi Sakamoto]

The check of cycle master capability in root node is currently just in a
condition branch. In this case, the required variable should be within the
branch.

This commit is just for the purpose.

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

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index b430a70a7eeb..474d8066e090 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -289,12 +289,10 @@ static void bm_work(struct work_struct *work)
 		63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40
 	};
 	struct fw_card *card __free(card_unref) = from_work(card, work, bm_work.work);
-	struct fw_device *root_device;
 	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;
 	bool do_reset = false;
-	bool root_device_is_cmc;
 
 	lockdep_assert_held(&card->lock);
 
@@ -308,8 +306,6 @@ static void bm_work(struct work_struct *work)
 	generation = card->generation;
 
 	root_node = fw_node_get(card->root_node);
-	root_device = fw_node_get_device(root_node);
-	root_device_is_cmc = root_device && root_device->cmc;
 
 	root_id  = root_node->node_id;
 	irm_id   = card->irm_node->node_id;
@@ -519,9 +515,11 @@ static void bm_work(struct work_struct *work)
 		reset_bus(card, card_gap_count != 0);
 		/* Will allocate broadcast channel after the reset. */
 	} else {
+		struct fw_device *root_device = fw_node_get_device(root_node);
+
 		spin_unlock_irq(&card->lock);
 
-		if (root_device_is_cmc) {
+		if (root_device && root_device->cmc) {
 			// Make sure that the cycle master sends cycle start packets.
 			__be32 data = cpu_to_be32(CSR_STATE_BIT_CMSTR);
 			int rcode = fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
-- 
2.48.1

Re: [PATCH 00/11] firewire: code refactoring mainly for bm_work

[Takashi Sakamoto]

On Mon, Sep 08, 2025 at 10:20:57AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> The core function in this subsystem have bm_work work item. The
> corresponding function has many lines with comments. It is a sign
> that it is time to be refactored, in my experience.
> 
> This series includes the first take for the purpose, as well as the
> other code improvements of 1394 OHCI PCI driver and the other core
> functions.
> 
> Takashi Sakamoto (11):
>   firewire: ohci: use kcalloc() variant for array allocation
>   firewire: core: utilize cleanup function to release workqueue in error
>     path
>   firewire: ohci: use return value from fw_node_get()
>   firewire: core: add helper functions to access to fw_device data in
>     fw_node structure
>   firewire: core: use cleanup function in bm_work
>   firewire: ohci: localize transaction data and rcode per condition
>     branch
>   firewire: core: code refactoring to evaluate transaction result to
>     CSR_BUS_MANAGER_ID
>   firewire: core: refer fw_card member to initiate bus reset under
>     acquiring lock
>   firewire: core: code refactoring to detect both IEEE 1394:1995 IRM and
>     Canon MV5i
>   firewire: core: code refactoring to investigate root node for bus
>     manager
>   firewire: core: code refactoring whether root node is cycle master
>     capable
> 
>  drivers/firewire/core-card.c   | 239 +++++++++++++++++----------------
>  drivers/firewire/core-device.c |  18 +--
>  drivers/firewire/core.h        |  14 +-
>  drivers/firewire/ohci.c        |   7 +-
>  4 files changed, 143 insertions(+), 135 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto