firewire: core: use spin lock specific to transaction

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.

Link: https://lore.kernel.org/r/20250915234747.915922-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 616abb836ef3dd25e6c66a847a243130989fe988..39f95007305f233ab954bcd2107e3662093041a9 100644 (file)
--- 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 8edffafd21c1fc832da03dfb2d7ec11a5dbfecd2..5366d8a781ac79af3ef03e8931b32f0f538b8253 100644 (file)
--- 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 aeb71c39e57ee287099e09b9e0bb934526e13427..8d6801cf2fcabf0d58f140fedc4f265b3d6232fa 100644 (file)
--- 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;