[PATCH] firewire: core: use appropriate spinlock variants for transaction-related locks

[PATCH] firewire: core: use appropriate spinlock variants for transaction-related locks

[Takashi Sakamoto]

The spinlocks in the fw_card structure protect transaction-related state
from concurrent access. They are acquired mostly from process context.
The exceptions are split_transaction_timeout_callback(), which runs in
softirq context for the timer wheel, and __fw_send_request(), which may
also be called from interrupt context.

Use spin_lock() in split_transaction_timeout_callback() and
spin_lock_irqsave() in the other functions. The latter preserves and
restores the caller's interrupt state, as the callers are not guaranteed
to have interrupts enabled.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index dfa6c97f9dce..33c4ea48c6c9 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -78,8 +78,6 @@ __must_not_hold(&card->transactions.lock)
 
 	lockdep_assert_not_held(&card->transactions.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.
 	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
 		list_for_each_entry_safe(t, tmp, &card->transactions.list, link) {
 			if (try_cancel_split_timeout(t))
@@ -136,9 +134,6 @@ __must_not_hold(&card->transactions.lock)
 
 	// If the request packet has already been sent, we need to see if the transaction is still
 	// pending and remove it in that case (e.g. the split transaction).
-	//
-	// 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) {
 		if (!find_and_pop_transaction_entry(card, iter == transaction))
 			return -ENOENT;
@@ -180,7 +175,7 @@ static void split_transaction_timeout_callback(struct timer_list *timer)
 
 	lockdep_assert_not_held(&card->transactions.lock);
 
-	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
+	scoped_guard(spinlock, &card->transactions.lock) {
 		if (list_empty(&t->link))
 			return;
 		remove_transaction_entry(card, t);
@@ -223,14 +218,12 @@ __must_not_hold(&card->transactions.lock)
 	{
 		unsigned int delta;
 
-		scoped_guard(spinlock, &card->split_timeout.lock) {
+		scoped_guard(spinlock_irqsave, &card->split_timeout.lock) {
 			t->split_timeout_cycle =
 				compute_split_timeout_timestamp(card, packet->timestamp) & 0xffff;
 			delta = 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->transactions.lock)
 			start_split_transaction_timeout(t, delta);
 		return;
@@ -255,8 +248,6 @@ __must_not_hold(&card->transactions.lock)
 		break;
 	}
 
-	// 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) {
 		if (!find_and_pop_transaction_entry(card, iter == t))
 			return;
@@ -427,8 +418,6 @@ __must_not_hold(&card->transactions.lock)
 	 * the list while holding the card spinlock.
 	 */
 
-	// 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) {
@@ -454,15 +443,11 @@ __must_not_hold(&card->transactions.lock)
 	// The node_id field of fw_card can be updated when handling SelfIDComplete.
 	int node_id;
 
-	// 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)
 		node_id = card->node_id;
 	fw_fill_request(&t->packet, tcode, t->tlabel, destination_id, node_id, generation, speed,
 			offset, payload, 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->transactions.lock)
 		list_add_tail(&t->link, &card->transactions.list);
 
@@ -1228,8 +1213,6 @@ __must_not_hold(&card->transactions.lock)
 		break;
 	}
 
-	// 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) {
 		t = find_and_pop_transaction_entry(card,
 				iter->node_id == source && iter->tlabel == tlabel);

base-commit: c85f48c90bf3e62f50b78f7be7d628c06b97c335
-- 
2.53.0

Re: [PATCH] firewire: core: use appropriate spinlock variants for transaction-related locks

[Takashi Sakamoto]

On Mon, Oct 12, 2026 at 10:57:39AM +0900, Takashi Sakamoto wrote:
> The spinlocks in the fw_card structure protect transaction-related state
> from concurrent access. They are acquired mostly from process context.
> The exceptions are split_transaction_timeout_callback(), which runs in
> softirq context for the timer wheel, and __fw_send_request(), which may
> also be called from interrupt context.
> 
> Use spin_lock() in split_transaction_timeout_callback() and
> spin_lock_irqsave() in the other functions. The latter preserves and
> restores the caller's interrupt state, as the callers are not guaranteed
> to have interrupts enabled.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  drivers/firewire/core-transaction.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto