* [PATCH 1/6] firewire: core: use scoped_guard() to manage critical section to update topology
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 2/6] firewire: core: maintain phy packet receivers locally in cdev layer Takashi Sakamoto
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
At present, guard() macro is used for the critical section to update
topology. It is inconvenient to add the other critical sections into
the function.
This commit uses scoped_guard() macro instead.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-topology.c | 74 +++++++++++++++-----------------
1 file changed, 34 insertions(+), 40 deletions(-)
diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 5f8fb1201d80..17aaf14cab0b 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -458,46 +458,40 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count);
- guard(spinlock_irqsave)(&card->lock);
-
- /*
- * If the selfID buffer is not the immediate successor of the
- * previously processed one, we cannot reliably compare the
- * old and new topologies.
- */
- if (!is_next_generation(generation, card->generation) &&
- card->local_node != NULL) {
- fw_destroy_nodes(card);
- card->bm_retries = 0;
- }
-
- card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated;
- card->node_id = node_id;
- /*
- * Update node_id before generation to prevent anybody from using
- * a stale node_id together with a current generation.
- */
- smp_wmb();
- card->generation = generation;
- card->reset_jiffies = get_jiffies_64();
- card->bm_node_id = 0xffff;
- card->bm_abdicate = bm_abdicate;
- fw_schedule_bm_work(card, 0);
-
- local_node = build_tree(card, self_ids, self_id_count, generation);
-
- update_topology_map(card, self_ids, self_id_count);
-
- card->color++;
-
- if (local_node == NULL) {
- fw_err(card, "topology build failed\n");
- /* FIXME: We need to issue a bus reset in this case. */
- } else if (card->local_node == NULL) {
- card->local_node = local_node;
- for_each_fw_node(card, local_node, report_found_node);
- } else {
- update_tree(card, local_node);
+ scoped_guard(spinlock, &card->lock) {
+ // If the selfID buffer is not the immediate successor of the
+ // previously processed one, we cannot reliably compare the
+ // old and new topologies.
+ if (!is_next_generation(generation, card->generation) && card->local_node != NULL) {
+ fw_destroy_nodes(card);
+ card->bm_retries = 0;
+ }
+ card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated;
+ card->node_id = node_id;
+ // Update node_id before generation to prevent anybody from using
+ // a stale node_id together with a current generation.
+ smp_wmb();
+ card->generation = generation;
+ card->reset_jiffies = get_jiffies_64();
+ card->bm_node_id = 0xffff;
+ card->bm_abdicate = bm_abdicate;
+ fw_schedule_bm_work(card, 0);
+
+ local_node = build_tree(card, self_ids, self_id_count, generation);
+
+ update_topology_map(card, self_ids, self_id_count);
+
+ card->color++;
+
+ if (local_node == NULL) {
+ fw_err(card, "topology build failed\n");
+ // FIXME: We need to issue a bus reset in this case.
+ } else if (card->local_node == NULL) {
+ card->local_node = local_node;
+ for_each_fw_node(card, local_node, report_found_node);
+ } else {
+ update_tree(card, local_node);
+ }
}
}
EXPORT_SYMBOL(fw_core_handle_bus_reset);
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/6] firewire: core: maintain phy packet receivers locally in cdev layer
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 1/6] firewire: core: use scoped_guard() to manage critical section to update topology Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 3/6] firewire: core: use spin lock specific to topology map Takashi Sakamoto
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
The list of receivers for phy packet is used only by cdev layer, while it
is maintained as a member of fw_card structure.
This commit maintains the list locally in cdev layer.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-card.c | 1 -
drivers/firewire/core-cdev.c | 27 ++++++++++++++++++++-------
include/linux/firewire.h | 2 --
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index b5e01a711145..616abb836ef3 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -556,7 +556,6 @@ void fw_card_initialize(struct fw_card *card,
kref_init(&card->kref);
init_completion(&card->done);
INIT_LIST_HEAD(&card->transaction_list);
- INIT_LIST_HEAD(&card->phy_receiver_list);
spin_lock_init(&card->lock);
card->local_node = NULL;
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 1be8f5eb3e17..112b33099610 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -47,6 +47,9 @@
#define FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW 5
#define FW_CDEV_VERSION_EVENT_ASYNC_TSTAMP 6
+static DEFINE_SPINLOCK(phy_receiver_list_lock);
+static LIST_HEAD(phy_receiver_list);
+
struct client {
u32 version;
struct fw_device *device;
@@ -1669,15 +1672,16 @@ static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
static int ioctl_receive_phy_packets(struct client *client, union ioctl_arg *arg)
{
struct fw_cdev_receive_phy_packets *a = &arg->receive_phy_packets;
- struct fw_card *card = client->device->card;
/* Access policy: Allow this ioctl only on local nodes' device files. */
if (!client->device->is_local)
return -ENOSYS;
- guard(spinlock_irq)(&card->lock);
+ // NOTE: This can be without irq when we can guarantee that __fw_send_request() for local
+ // destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irq, &phy_receiver_list_lock)
+ list_move_tail(&client->phy_receiver_link, &phy_receiver_list);
- list_move_tail(&client->phy_receiver_link, &card->phy_receiver_list);
client->phy_receiver_closure = a->closure;
return 0;
@@ -1687,10 +1691,17 @@ void fw_cdev_handle_phy_packet(struct fw_card *card, struct fw_packet *p)
{
struct client *client;
- guard(spinlock_irqsave)(&card->lock);
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for local
+ // destination never runs in any type of IRQ context.
+ guard(spinlock_irqsave)(&phy_receiver_list_lock);
+
+ list_for_each_entry(client, &phy_receiver_list, phy_receiver_link) {
+ struct inbound_phy_packet_event *e;
+
+ if (client->device->card != card)
+ continue;
- list_for_each_entry(client, &card->phy_receiver_list, phy_receiver_link) {
- struct inbound_phy_packet_event *e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC);
+ e = kmalloc(sizeof(*e) + 8, GFP_ATOMIC);
if (e == NULL)
break;
@@ -1857,7 +1868,9 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
struct client_resource *resource;
unsigned long index;
- scoped_guard(spinlock_irq, &client->device->card->lock)
+ // NOTE: This can be without irq when we can guarantee that __fw_send_request() for local
+ // destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irq, &phy_receiver_list_lock)
list_del(&client->phy_receiver_link);
scoped_guard(mutex, &client->device->client_list_mutex)
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index d38c6e538e5c..f3260aacf730 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -115,8 +115,6 @@ struct fw_card {
int index;
struct list_head link;
- struct list_head phy_receiver_list;
-
struct delayed_work br_work; /* bus reset job */
bool br_short;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/6] firewire: core: use spin lock specific to topology map
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 1/6] firewire: core: use scoped_guard() to manage critical section to update topology Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 2/6] firewire: core: maintain phy packet receivers locally in cdev layer Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 4/6] firewire: core: use spin lock specific to transaction Takashi Sakamoto
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
At present, the operation for read transaction to topology map register is
not protected by any kind of lock primitives. This causes a potential
problem to result in the mixed content of topology map.
This commit adds and uses spin lock specific to topology map.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-topology.c | 22 ++++++++++++++--------
drivers/firewire/core-transaction.c | 6 +++++-
include/linux/firewire.h | 6 +++++-
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 17aaf14cab0b..c62cf93f3f65 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -435,20 +435,22 @@ static void update_tree(struct fw_card *card, struct fw_node *root)
}
}
-static void update_topology_map(struct fw_card *card,
- u32 *self_ids, int self_id_count)
+static void update_topology_map(__be32 *buffer, size_t buffer_size, int root_node_id,
+ const u32 *self_ids, int self_id_count)
{
- int node_count = (card->root_node->node_id & 0x3f) + 1;
- __be32 *map = card->topology_map;
+ __be32 *map = buffer;
+ int node_count = (root_node_id & 0x3f) + 1;
+
+ memset(map, 0, buffer_size);
*map++ = cpu_to_be32((self_id_count + 2) << 16);
- *map++ = cpu_to_be32(be32_to_cpu(card->topology_map[1]) + 1);
+ *map++ = cpu_to_be32(be32_to_cpu(buffer[1]) + 1);
*map++ = cpu_to_be32((node_count << 16) | self_id_count);
while (self_id_count--)
*map++ = cpu_to_be32p(self_ids++);
- fw_compute_block_crc(card->topology_map);
+ fw_compute_block_crc(buffer);
}
void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
@@ -479,8 +481,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
local_node = build_tree(card, self_ids, self_id_count, generation);
- update_topology_map(card, self_ids, self_id_count);
-
card->color++;
if (local_node == NULL) {
@@ -493,5 +493,11 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
update_tree(card, local_node);
}
}
+
+ // Just used by transaction layer.
+ scoped_guard(spinlock, &card->topology_map.lock) {
+ update_topology_map(card->topology_map.buffer, sizeof(card->topology_map.buffer),
+ card->root_node->node_id, self_ids, self_id_count);
+ }
}
EXPORT_SYMBOL(fw_core_handle_bus_reset);
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 623e1d9bd107..8edffafd21c1 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -1196,7 +1196,11 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request
}
start = (offset - topology_map_region.start) / 4;
- memcpy(payload, &card->topology_map[start], length);
+
+ // NOTE: This can be without irqsave when we can guarantee that fw_send_request() for local
+ // destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->topology_map.lock)
+ memcpy(payload, &card->topology_map.buffer[start], length);
fw_send_response(card, request, RCODE_COMPLETE);
}
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index f3260aacf730..aeb71c39e57e 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -129,7 +129,11 @@ struct fw_card {
bool broadcast_channel_allocated;
u32 broadcast_channel;
- __be32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4];
+
+ struct {
+ __be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4];
+ spinlock_t lock;
+ } topology_map;
__be32 maint_utility_register;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/6] firewire: core: use spin lock specific to transaction
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
` (2 preceding siblings ...)
2025-09-15 23:47 ` [PATCH 3/6] firewire: core: use spin lock specific to topology map Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 5/6] firewire: core: use spin lock specific to timer for split transaction Takashi Sakamoto
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
The list of instance for asynchronous transaction to wait for response
subaction is maintained as a member of fw_card structure. The card-wide
spinlock is used at present for any operation over the list, however it
is not necessarily suited for the purpose.
This commit adds and uses the spin lock specific to maintain the list.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-card.c | 12 ++++--
drivers/firewire/core-transaction.c | 59 ++++++++++++++++++-----------
include/linux/firewire.h | 10 +++--
3 files changed, 51 insertions(+), 30 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 616abb836ef3..39f95007305f 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -544,8 +544,12 @@ void fw_card_initialize(struct fw_card *card,
card->index = atomic_inc_return(&index);
card->driver = driver;
card->device = device;
- card->current_tlabel = 0;
- card->tlabel_mask = 0;
+
+ card->transactions.current_tlabel = 0;
+ card->transactions.tlabel_mask = 0;
+ INIT_LIST_HEAD(&card->transactions.list);
+ spin_lock_init(&card->transactions.lock);
+
card->split_timeout_hi = DEFAULT_SPLIT_TIMEOUT / 8000;
card->split_timeout_lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19;
card->split_timeout_cycles = DEFAULT_SPLIT_TIMEOUT;
@@ -555,7 +559,7 @@ void fw_card_initialize(struct fw_card *card,
kref_init(&card->kref);
init_completion(&card->done);
- INIT_LIST_HEAD(&card->transaction_list);
+
spin_lock_init(&card->lock);
card->local_node = NULL;
@@ -772,7 +776,7 @@ void fw_core_remove_card(struct fw_card *card)
destroy_workqueue(card->isoc_wq);
destroy_workqueue(card->async_wq);
- WARN_ON(!list_empty(&card->transaction_list));
+ WARN_ON(!list_empty(&card->transactions.list));
}
EXPORT_SYMBOL(fw_core_remove_card);
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 8edffafd21c1..5366d8a781ac 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -49,12 +49,14 @@ static int close_transaction(struct fw_transaction *transaction, struct fw_card
{
struct fw_transaction *t = NULL, *iter;
- scoped_guard(spinlock_irqsave, &card->lock) {
- list_for_each_entry(iter, &card->transaction_list, link) {
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->transactions.lock) {
+ list_for_each_entry(iter, &card->transactions.list, link) {
if (iter == transaction) {
if (try_cancel_split_timeout(iter)) {
list_del_init(&iter->link);
- card->tlabel_mask &= ~(1ULL << iter->tlabel);
+ card->transactions.tlabel_mask &= ~(1ULL << iter->tlabel);
t = iter;
}
break;
@@ -117,11 +119,11 @@ static void split_transaction_timeout_callback(struct timer_list *timer)
struct fw_transaction *t = timer_container_of(t, timer, split_timeout_timer);
struct fw_card *card = t->card;
- scoped_guard(spinlock_irqsave, &card->lock) {
+ scoped_guard(spinlock_irqsave, &card->transactions.lock) {
if (list_empty(&t->link))
return;
list_del(&t->link);
- card->tlabel_mask &= ~(1ULL << t->tlabel);
+ card->transactions.tlabel_mask &= ~(1ULL << t->tlabel);
}
if (!t->with_tstamp) {
@@ -259,18 +261,21 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel,
}
static int allocate_tlabel(struct fw_card *card)
+__must_hold(&card->transactions_lock)
{
int tlabel;
- tlabel = card->current_tlabel;
- while (card->tlabel_mask & (1ULL << tlabel)) {
+ lockdep_assert_held(&card->transactions.lock);
+
+ tlabel = card->transactions.current_tlabel;
+ while (card->transactions.tlabel_mask & (1ULL << tlabel)) {
tlabel = (tlabel + 1) & 0x3f;
- if (tlabel == card->current_tlabel)
+ if (tlabel == card->transactions.current_tlabel)
return -EBUSY;
}
- card->current_tlabel = (tlabel + 1) & 0x3f;
- card->tlabel_mask |= 1ULL << tlabel;
+ card->transactions.current_tlabel = (tlabel + 1) & 0x3f;
+ card->transactions.tlabel_mask |= 1ULL << tlabel;
return tlabel;
}
@@ -331,7 +336,6 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode
void *payload, size_t length, union fw_transaction_callback callback,
bool with_tstamp, void *callback_data)
{
- unsigned long flags;
int tlabel;
/*
@@ -339,11 +343,11 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode
* the list while holding the card spinlock.
*/
- spin_lock_irqsave(&card->lock, flags);
-
- tlabel = allocate_tlabel(card);
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->transactions.lock)
+ tlabel = allocate_tlabel(card);
if (tlabel < 0) {
- spin_unlock_irqrestore(&card->lock, flags);
if (!with_tstamp) {
callback.without_tstamp(card, RCODE_SEND_ERROR, NULL, 0, callback_data);
} else {
@@ -368,15 +372,22 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode
t->callback = callback;
t->with_tstamp = with_tstamp;
t->callback_data = callback_data;
-
- fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, card->node_id, generation,
- speed, offset, payload, length);
t->packet.callback = transmit_complete_callback;
- list_add_tail(&t->link, &card->transaction_list);
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->lock) {
+ // The node_id field of fw_card can be updated when handling SelfIDComplete.
+ fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, card->node_id,
+ generation, speed, offset, payload, length);
+ }
- spin_unlock_irqrestore(&card->lock, flags);
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->transactions.lock)
+ list_add_tail(&t->link, &card->transactions.list);
+ // Safe with no lock, since the index field of fw_card is immutable once assigned.
trace_async_request_outbound_initiate((uintptr_t)t, card->index, generation, speed,
t->packet.header, payload,
tcode_is_read_request(tcode) ? 0 : length / 4);
@@ -1111,12 +1122,14 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
break;
}
- scoped_guard(spinlock_irqsave, &card->lock) {
- list_for_each_entry(iter, &card->transaction_list, link) {
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->transactions.lock) {
+ list_for_each_entry(iter, &card->transactions.list, link) {
if (iter->node_id == source && iter->tlabel == tlabel) {
if (try_cancel_split_timeout(iter)) {
list_del_init(&iter->link);
- card->tlabel_mask &= ~(1ULL << iter->tlabel);
+ card->transactions.tlabel_mask &= ~(1ULL << iter->tlabel);
t = iter;
}
break;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index aeb71c39e57e..8d6801cf2fca 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -88,11 +88,15 @@ struct fw_card {
int node_id;
int generation;
- int current_tlabel;
- u64 tlabel_mask;
- struct list_head transaction_list;
u64 reset_jiffies;
+ struct {
+ int current_tlabel;
+ u64 tlabel_mask;
+ struct list_head list;
+ spinlock_t lock;
+ } transactions;
+
u32 split_timeout_hi;
u32 split_timeout_lo;
unsigned int split_timeout_cycles;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 5/6] firewire: core: use spin lock specific to timer for split transaction
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
` (3 preceding siblings ...)
2025-09-15 23:47 ` [PATCH 4/6] firewire: core: use spin lock specific to transaction Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-15 23:47 ` [PATCH 6/6] firewire: core: annotate fw_destroy_nodes with must-hold-lock Takashi Sakamoto
2025-09-16 23:36 ` [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
At present the parameters to compute timeout time for split transaction is
protected by card-wide spin lock, while it is not necessarily convenient
in a point to narrower critical section.
This commit adds and uses another spin lock specific for the purpose.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-card.c | 10 +++--
drivers/firewire/core-transaction.c | 63 +++++++++++++++++++----------
include/linux/firewire.h | 15 ++++---
3 files changed, 57 insertions(+), 31 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 39f95007305f..96495392a1f6 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -550,10 +550,12 @@ void fw_card_initialize(struct fw_card *card,
INIT_LIST_HEAD(&card->transactions.list);
spin_lock_init(&card->transactions.lock);
- card->split_timeout_hi = DEFAULT_SPLIT_TIMEOUT / 8000;
- card->split_timeout_lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19;
- card->split_timeout_cycles = DEFAULT_SPLIT_TIMEOUT;
- card->split_timeout_jiffies = isoc_cycles_to_jiffies(DEFAULT_SPLIT_TIMEOUT);
+ card->split_timeout.hi = DEFAULT_SPLIT_TIMEOUT / 8000;
+ card->split_timeout.lo = (DEFAULT_SPLIT_TIMEOUT % 8000) << 19;
+ card->split_timeout.cycles = DEFAULT_SPLIT_TIMEOUT;
+ card->split_timeout.jiffies = isoc_cycles_to_jiffies(DEFAULT_SPLIT_TIMEOUT);
+ spin_lock_init(&card->split_timeout.lock);
+
card->color = 0;
card->broadcast_channel = BROADCAST_CHANNEL_INITIAL;
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 5366d8a781ac..dd3656a0c1ff 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -137,14 +137,18 @@ static void split_transaction_timeout_callback(struct timer_list *timer)
static void start_split_transaction_timeout(struct fw_transaction *t,
struct fw_card *card)
{
- guard(spinlock_irqsave)(&card->lock);
+ unsigned long delta;
if (list_empty(&t->link) || WARN_ON(t->is_split_transaction))
return;
t->is_split_transaction = true;
- mod_timer(&t->split_timeout_timer,
- jiffies + card->split_timeout_jiffies);
+
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->split_timeout.lock)
+ delta = card->split_timeout.jiffies;
+ mod_timer(&t->split_timeout_timer, jiffies + delta);
}
static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_timestamp);
@@ -164,8 +168,12 @@ static void transmit_complete_callback(struct fw_packet *packet,
break;
case ACK_PENDING:
{
- t->split_timeout_cycle =
- compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff;
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->split_timeout.lock) {
+ t->split_timeout_cycle =
+ compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff;
+ }
start_split_transaction_timeout(t, card);
break;
}
@@ -790,11 +798,14 @@ EXPORT_SYMBOL(fw_fill_response);
static u32 compute_split_timeout_timestamp(struct fw_card *card,
u32 request_timestamp)
+__must_hold(&card->split_timeout.lock)
{
unsigned int cycles;
u32 timestamp;
- cycles = card->split_timeout_cycles;
+ lockdep_assert_held(&card->split_timeout.lock);
+
+ cycles = card->split_timeout.cycles;
cycles += request_timestamp & 0x1fff;
timestamp = request_timestamp & ~0x1fff;
@@ -845,9 +856,12 @@ static struct fw_request *allocate_request(struct fw_card *card,
return NULL;
kref_init(&request->kref);
+ // NOTE: This can be without irqsave when we can guarantee that __fw_send_request() for
+ // local destination never runs in any type of IRQ context.
+ scoped_guard(spinlock_irqsave, &card->split_timeout.lock)
+ request->response.timestamp = compute_split_timeout_timestamp(card, p->timestamp);
+
request->response.speed = p->speed;
- request->response.timestamp =
- compute_split_timeout_timestamp(card, p->timestamp);
request->response.generation = p->generation;
request->response.ack = 0;
request->response.callback = free_response_callback;
@@ -1228,16 +1242,17 @@ static const struct fw_address_region registers_region =
.end = CSR_REGISTER_BASE | CSR_CONFIG_ROM, };
static void update_split_timeout(struct fw_card *card)
+__must_hold(&card->split_timeout.lock)
{
unsigned int cycles;
- cycles = card->split_timeout_hi * 8000 + (card->split_timeout_lo >> 19);
+ cycles = card->split_timeout.hi * 8000 + (card->split_timeout.lo >> 19);
/* minimum per IEEE 1394, maximum which doesn't overflow OHCI */
cycles = clamp(cycles, 800u, 3u * 8000u);
- card->split_timeout_cycles = cycles;
- card->split_timeout_jiffies = isoc_cycles_to_jiffies(cycles);
+ card->split_timeout.cycles = cycles;
+ card->split_timeout.jiffies = isoc_cycles_to_jiffies(cycles);
}
static void handle_registers(struct fw_card *card, struct fw_request *request,
@@ -1287,12 +1302,15 @@ static void handle_registers(struct fw_card *card, struct fw_request *request,
case CSR_SPLIT_TIMEOUT_HI:
if (tcode == TCODE_READ_QUADLET_REQUEST) {
- *data = cpu_to_be32(card->split_timeout_hi);
+ *data = cpu_to_be32(card->split_timeout.hi);
} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
- guard(spinlock_irqsave)(&card->lock);
-
- card->split_timeout_hi = be32_to_cpu(*data) & 7;
- update_split_timeout(card);
+ // NOTE: This can be without irqsave when we can guarantee that
+ // __fw_send_request() for local destination never runs in any type of IRQ
+ // context.
+ scoped_guard(spinlock_irqsave, &card->split_timeout.lock) {
+ card->split_timeout.hi = be32_to_cpu(*data) & 7;
+ update_split_timeout(card);
+ }
} else {
rcode = RCODE_TYPE_ERROR;
}
@@ -1300,12 +1318,15 @@ static void handle_registers(struct fw_card *card, struct fw_request *request,
case CSR_SPLIT_TIMEOUT_LO:
if (tcode == TCODE_READ_QUADLET_REQUEST) {
- *data = cpu_to_be32(card->split_timeout_lo);
+ *data = cpu_to_be32(card->split_timeout.lo);
} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
- guard(spinlock_irqsave)(&card->lock);
-
- card->split_timeout_lo = be32_to_cpu(*data) & 0xfff80000;
- update_split_timeout(card);
+ // NOTE: This can be without irqsave when we can guarantee that
+ // __fw_send_request() for local destination never runs in any type of IRQ
+ // context.
+ scoped_guard(spinlock_irqsave, &card->split_timeout.lock) {
+ card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000;
+ update_split_timeout(card);
+ }
} else {
rcode = RCODE_TYPE_ERROR;
}
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 8d6801cf2fca..6d208769d456 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -97,18 +97,21 @@ struct fw_card {
spinlock_t lock;
} transactions;
- u32 split_timeout_hi;
- u32 split_timeout_lo;
- unsigned int split_timeout_cycles;
- unsigned int split_timeout_jiffies;
+ struct {
+ u32 hi;
+ u32 lo;
+ unsigned int cycles;
+ unsigned int jiffies;
+ spinlock_t lock;
+ } split_timeout;
unsigned long long guid;
unsigned max_receive;
int link_speed;
int config_rom_generation;
- spinlock_t lock; /* Take this lock when handling the lists in
- * this struct. */
+ spinlock_t lock;
+
struct fw_node *local_node;
struct fw_node *root_node;
struct fw_node *irm_node;
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 6/6] firewire: core: annotate fw_destroy_nodes with must-hold-lock
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
` (4 preceding siblings ...)
2025-09-15 23:47 ` [PATCH 5/6] firewire: core: use spin lock specific to timer for split transaction Takashi Sakamoto
@ 2025-09-15 23:47 ` Takashi Sakamoto
2025-09-16 23:36 ` [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-15 23:47 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
The function, fw_destroy_nodes(), is used widely within firewire-core
module. It has a prerequisite condition that struct fw_card.lock must
be hold in advance.
This commit adds annotation for it.
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
drivers/firewire/core-topology.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index c62cf93f3f65..8fa0772ee723 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -325,9 +325,11 @@ static void report_found_node(struct fw_card *card,
card->bm_retries = 0;
}
-/* Must be called with card->lock held */
void fw_destroy_nodes(struct fw_card *card)
+__must_hold(&card->lock)
{
+ lockdep_assert_held(&card->lock);
+
card->color++;
if (card->local_node != NULL)
for_each_fw_node(card, card->local_node, report_lost_node);
--
2.48.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/6] firewire: core: partition fw_card spinlock
2025-09-15 23:47 [PATCH 0/6] firewire: core: partition fw_card spinlock Takashi Sakamoto
` (5 preceding siblings ...)
2025-09-15 23:47 ` [PATCH 6/6] firewire: core: annotate fw_destroy_nodes with must-hold-lock Takashi Sakamoto
@ 2025-09-16 23:36 ` Takashi Sakamoto
6 siblings, 0 replies; 8+ messages in thread
From: Takashi Sakamoto @ 2025-09-16 23:36 UTC (permalink / raw)
To: linux1394-devel; +Cc: linux-kernel
On Tue, Sep 16, 2025 at 08:47:41AM +0900, Takashi Sakamoto wrote:
> Hi,
>
> The current implementation uses the fw_card spinlock for a wide range of
> purposes, which goes against the theory that the type of lock should
> protect critical sections as narrowly as possible.
>
> This patchset adds some spinlocks for specific purposes, therefore
> partitioning the existing wide-purpose lock.
>
> Takashi Sakamoto (6):
> firewire: core: use scoped_guard() to manage critical section to
> update topology
> firewire: core: maintain phy packet receivers locally in cdev layer
> firewire: core: use spin lock specific to topology map
> firewire: core: use spin lock specific to transaction
> firewire: core: use spin lock specific to timer for split transaction
> firewire: core: annotate fw_destroy_nodes with must-hold-lock
>
> drivers/firewire/core-card.c | 23 +++--
> drivers/firewire/core-cdev.c | 27 ++++--
> drivers/firewire/core-topology.c | 92 ++++++++++----------
> drivers/firewire/core-transaction.c | 128 ++++++++++++++++++----------
> include/linux/firewire.h | 33 ++++---
> 5 files changed, 185 insertions(+), 118 deletions(-)
Applied to for-next branch.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 8+ messages in thread