[PATCH v3 0/6] firewire: core: make execution context assumptions explicit for transaction layer

[PATCH v3 0/6] firewire: core: make execution context assumptions explicit for transaction layer

[Takashi Sakamoto]

Hi,

This series is take 3 of the previous series[1].

Recent code changes have guaranteed the execution contexts of several
functions in the transaction layer. Based on this, this series adds
might_sleep() calls where appropriate. It also adds context analysis
annotations and lockdep checks for several locks to document the expected
locking context and help detect incorrect locking behavior.

Chanes from v2:
* Switch from sparse to Compiler-based context analysis assisted by Thread
  Safety Analysis in LLVM/Clang v23 or later.

[1][PATCH v2 0/5] firewire: core: make execution context assumptions explicit
https://lore.kernel.org/lkml/20261009121048.282982-1-o-takashi@sakamocchi.jp/


Takashi Sakamoto (6):
  firewire: core: add might_sleep() checks to transaction layer
  firewire: core: enable context analysis for transaction layer
  firewire: core: use context analysis annotations and lockdep checks
    for transactions lock
  firewire: core: use context analysis annotations and lockdep checks
    for split timeout lock
  firewire: core: use context analysis annotations and lockdep checks
    for topology map lock
  firewire: core: narrow the card lock scope when accessing node_id

 drivers/firewire/Makefile           |  2 +
 drivers/firewire/core-topology.c    |  3 +
 drivers/firewire/core-transaction.c | 97 +++++++++++++++++++++++------
 include/linux/firewire.h            | 16 ++---
 4 files changed, 91 insertions(+), 27 deletions(-)


base-commit: a8cf19ce243bf2cd0d338823fa79cd927e34b830
-- 
2.53.0

[PATCH v3 1/6] firewire: core: add might_sleep() checks to transaction layer

[Takashi Sakamoto]

Some functions are now guaranteed to run in process context.

Add might_sleep() calls to these functions to document and check this
requirement.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index a0379fc3e60e..1b9c642abe43 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -114,6 +114,9 @@ void fw_cancel_pending_transactions(struct fw_card *card)
  */
 int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction)
 {
+	// Allow the call of struct fw_card_driver.cancel_packet() to wait for any event.
+	might_sleep();
+
 	// Cancel the packet transmission if it's still queued. That will schedule the packet
 	// transmission callback which cancels the transaction.
 	if (card->driver->cancel_packet(card, &transaction->packet) == 0)
@@ -488,6 +491,9 @@ int fw_run_transaction(struct fw_card *card, int tcode, int destination_id,
 	struct transaction_callback_data d;
 	struct fw_transaction t;
 
+	// Due to the call of wait_for_completion().
+	might_sleep();
+
 	timer_setup_on_stack(&t.split_timeout_timer, NULL, 0);
 	init_completion(&d.done);
 	d.payload = payload;
@@ -524,6 +530,9 @@ void fw_send_phy_config(struct fw_card *card,
 	long timeout = msecs_to_jiffies(100);
 	u32 data = 0;
 
+	// Due to the call of wait_for_completion_timeout().
+	might_sleep();
+
 	phy_packet_set_packet_identifier(&data, PHY_PACKET_PACKET_IDENTIFIER_PHY_CONFIG);
 
 	if (node_id != FW_PHY_CONFIG_NO_NODE_ID) {
@@ -697,6 +706,9 @@ EXPORT_SYMBOL(fw_core_add_address_handler);
  */
 void fw_core_remove_address_handler(struct fw_address_handler *handler)
 {
+	// Due to synchronize_rcu().
+	might_sleep();
+
 	scoped_guard(spinlock, &address_handler_list_lock)
 		list_del_rcu(&handler->link);
 
@@ -1104,6 +1116,10 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
 	unsigned long long offset;
 	unsigned int tcode;
 
+	// Allow the call of allocate_request() to perform object allocation with GFP_KERNEL,
+	// as well as the address handlers to work with mutexes.
+	might_sleep();
+
 	if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE)
 		return;
 
@@ -1142,6 +1158,9 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
 	size_t data_length;
 	int tcode, tlabel, source, rcode;
 
+	// Allow the call of struct fw_card_driver.cancel_packet() to wait for any event.
+	might_sleep();
+
 	tcode = async_header_get_tcode(p->header);
 	tlabel = async_header_get_tlabel(p->header);
 	source = async_header_get_source(p->header);
-- 
2.53.0

[PATCH v3 2/6] firewire: core: enable context analysis for transaction layer

[Takashi Sakamoto]

The kernel is transitioning from Sparse-based context analysis to
compier-based context analysis, which requires LLVM/Clang 23 or later.
Individual compilation units can opt in to the analysis through their
Makefiles.

Enable context analysis for the transaction layer.

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

diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index 1ff550e93a8c..e27fca37b4b2 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -3,6 +3,8 @@
 # Makefile for the Linux IEEE 1394 implementation
 #
 
+CONTEXT_ANALYSIS_core-transaction.o := y
+
 firewire-core-y += core-trace.o core-card.o core-cdev.o core-device.o \
                    core-iso.o core-topology.o core-transaction.o
 firewire-ohci-y += ohci.o
-- 
2.53.0

[PATCH v3 3/6] firewire: core: use context analysis annotations and lockdep checks for transactions lock

[Takashi Sakamoto]

The transaction lock protects the list of pending transactions and the
transaction label.

Add context analysis annotations for the transaction lock. Compiler-based
context analysis has difficulty tracking the locking scope when the lock
is accessed through an indirect reference (e.g. t->card->lock), making
some functions difficult to annotate. Use lockdep assertions to cover
these cases.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 32 +++++++++++++++++++++++++++--
 include/linux/firewire.h            |  6 +++---
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 1b9c642abe43..4466499dc1c9 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -36,18 +36,24 @@
 #define HEADER_DESTINATION_IS_BROADCAST(header) \
 	((async_header_get_destination(header) & 0x3f) == 0x3f)
 
-/* returns 0 if the split timeout handler is already running */
+// Returns 0 if the split timeout handler is already running.
+//
+// card->transactions.lock must be acquired in advance.
 static int try_cancel_split_timeout(struct fw_transaction *t)
 {
+	lockdep_assert_held(&t->card->transactions.lock);
+
 	if (t->is_split_transaction)
 		return timer_delete(&t->split_timeout_timer) || disable_work(&t->error_work);
 	else
 		return 1;
 }
 
-// card->transactions.lock must be acquired in advance.
 static void remove_transaction_entry(struct fw_card *card, struct fw_transaction *entry)
+__must_hold(&card->transactions.lock)
 {
+	lockdep_assert_held(&card->transactions.lock);
+
 	list_del_init(&entry->link);
 	card->transactions.tlabel_mask &= ~(1ULL << entry->tlabel);
 }
@@ -65,10 +71,13 @@ static void invoke_callback(struct fw_transaction *t, int rcode, u32 response_ts
 
 // Must be called without holding card->transactions.lock.
 void fw_cancel_pending_transactions(struct fw_card *card)
+__must_not_hold(&card->transactions.lock)
 {
 	struct fw_transaction *t, *tmp;
 	LIST_HEAD(pending_list);
 
+	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) {
@@ -113,7 +122,10 @@ void fw_cancel_pending_transactions(struct fw_card *card)
  * pending transaction.
  */
 int fw_cancel_transaction(struct fw_card *card, struct fw_transaction *transaction)
+__must_not_hold(&card->transactions.lock)
 {
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	// Allow the call of struct fw_card_driver.cancel_packet() to wait for any event.
 	might_sleep();
 
@@ -160,11 +172,14 @@ static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 res
 	queue_work(t->card->async_wq, &t->error_work);
 }
 
+// card->transactions.lock must be released in advance.
 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;
 
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	scoped_guard(spinlock_irqsave, &card->transactions.lock) {
 		if (list_empty(&t->link))
 			return;
@@ -177,6 +192,8 @@ static void split_transaction_timeout_callback(struct timer_list *timer)
 // card->transactions.lock should be acquired in advance for the linked list.
 static void start_split_transaction_timeout(struct fw_transaction *t, unsigned int delta)
 {
+	lockdep_assert_held(&t->card->transactions.lock);
+
 	if (list_empty(&t->link) || WARN_ON(t->is_split_transaction))
 		return;
 
@@ -189,10 +206,15 @@ static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_tim
 
 static void transmit_complete_callback(struct fw_packet *packet,
 				       struct fw_card *card, int status)
+__must_not_hold(&card->transactions.lock)
 {
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	struct fw_transaction *t =
 	    container_of(packet, struct fw_transaction, packet);
 
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	trace_async_request_outbound_complete((uintptr_t)t, card->index, packet->generation,
 					      packet->speed, status, packet->timestamp);
 
@@ -388,9 +410,12 @@ void __fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode
 		int destination_id, int generation, int speed, unsigned long long offset,
 		void *payload, size_t length, union fw_transaction_callback callback,
 		bool with_tstamp, void *callback_data)
+__must_not_hold(&card->transactions.lock)
 {
 	int tlabel;
 
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	t->card = card;
 	t->callback = callback;
 	t->with_tstamp = with_tstamp;
@@ -1152,12 +1177,15 @@ void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
 EXPORT_SYMBOL(fw_core_handle_request);
 
 void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
+__must_not_hold(&card->transactions.lock)
 {
 	struct fw_transaction *t = NULL;
 	u32 *data;
 	size_t data_length;
 	int tcode, tlabel, source, rcode;
 
+	lockdep_assert_not_held(&card->transactions.lock);
+
 	// Allow the call of struct fw_card_driver.cancel_packet() to wait for any event.
 	might_sleep();
 
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index dbb5c825a1cc..89db958dea2e 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -92,9 +92,9 @@ struct fw_card {
 	u64 reset_jiffies;
 
 	struct {
-		int current_tlabel;
-		u64 tlabel_mask;
-		struct list_head list;
+		int current_tlabel __guarded_by(&lock);
+		u64 tlabel_mask __guarded_by(&lock);
+		struct list_head list __guarded_by(&lock);
 		spinlock_t lock;
 	} transactions;
 
-- 
2.53.0

[PATCH v3 4/6] firewire: core: use context analysis annotations and lockdep checks for split timeout lock

[Takashi Sakamoto]

The spinlock protects parameters related to split transaction timeouts.

Add context analysis annotations and lockdep checks for this lock.

Link: https://lore.kernel.org/r/20261009121048.282982-5-o-takashi@sakamocchi.jp
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-transaction.c | 31 ++++++++++++++++++-----------
 include/linux/firewire.h            |  8 ++++----
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 4466499dc1c9..bd6d28f9bc50 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -206,6 +206,7 @@ static u32 compute_split_timeout_timestamp(struct fw_card *card, u32 request_tim
 
 static void transmit_complete_callback(struct fw_packet *packet,
 				       struct fw_card *card, int status)
+__must_not_hold(&card->split_timeout.lock)
 __must_not_hold(&card->transactions.lock)
 {
 	lockdep_assert_not_held(&card->transactions.lock);
@@ -213,6 +214,7 @@ __must_not_hold(&card->transactions.lock)
 	struct fw_transaction *t =
 	    container_of(packet, struct fw_transaction, packet);
 
+	lockdep_assert_not_held(&card->split_timeout.lock);
 	lockdep_assert_not_held(&card->transactions.lock);
 
 	trace_async_request_outbound_complete((uintptr_t)t, card->index, packet->generation,
@@ -896,11 +898,14 @@ __must_hold(&card->split_timeout.lock)
 
 static struct fw_request *allocate_request(struct fw_card *card,
 					   struct fw_packet *p)
+__must_not_hold(&card->split_timeout.lock)
 {
 	struct fw_request *request;
 	u32 *data, length;
 	int request_tcode;
 
+	lockdep_assert_not_held(&card->split_timeout.lock);
+
 	request_tcode = async_header_get_tcode(p->header);
 	switch (request_tcode) {
 	case TCODE_WRITE_QUADLET_REQUEST:
@@ -1318,6 +1323,8 @@ __must_hold(&card->split_timeout.lock)
 {
 	unsigned int cycles;
 
+	lockdep_assert_held(&card->split_timeout.lock);
+
 	cycles = card->split_timeout.hi * 8000 + (card->split_timeout.lo >> 19);
 
 	/* minimum per IEEE 1394, maximum which doesn't overflow OHCI */
@@ -1373,28 +1380,28 @@ static void handle_registers(struct fw_card *card, struct fw_request *request,
 		break;
 
 	case CSR_SPLIT_TIMEOUT_HI:
-		if (tcode == TCODE_READ_QUADLET_REQUEST) {
-			*data = cpu_to_be32(card->split_timeout.hi);
-		} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
-			scoped_guard(spinlock, &card->split_timeout.lock) {
+		scoped_guard(spinlock, &card->split_timeout.lock) {
+			if (tcode == TCODE_READ_QUADLET_REQUEST) {
+				*data = cpu_to_be32(card->split_timeout.hi);
+			} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
 				card->split_timeout.hi = be32_to_cpu(*data) & 7;
 				update_split_timeout(card);
+			} else {
+				rcode = RCODE_TYPE_ERROR;
 			}
-		} else {
-			rcode = RCODE_TYPE_ERROR;
 		}
 		break;
 
 	case CSR_SPLIT_TIMEOUT_LO:
-		if (tcode == TCODE_READ_QUADLET_REQUEST) {
-			*data = cpu_to_be32(card->split_timeout.lo);
-		} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
-			scoped_guard(spinlock, &card->split_timeout.lock) {
+		scoped_guard(spinlock, &card->split_timeout.lock) {
+			if (tcode == TCODE_READ_QUADLET_REQUEST) {
+				*data = cpu_to_be32(card->split_timeout.lo);
+			} else if (tcode == TCODE_WRITE_QUADLET_REQUEST) {
 				card->split_timeout.lo = be32_to_cpu(*data) & 0xfff80000;
 				update_split_timeout(card);
+			} else {
+				rcode = RCODE_TYPE_ERROR;
 			}
-		} else {
-			rcode = RCODE_TYPE_ERROR;
 		}
 		break;
 
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index 89db958dea2e..a3660ad1a1d4 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -99,10 +99,10 @@ struct fw_card {
 	} transactions;
 
 	struct {
-		u32 hi;
-		u32 lo;
-		unsigned int cycles;
-		unsigned int jiffies;
+		u32 hi __guarded_by(&lock);
+		u32 lo __guarded_by(&lock);
+		unsigned int cycles __guarded_by(&lock);
+		unsigned int jiffies __guarded_by(&lock);
 		spinlock_t lock;
 	} split_timeout;
 
-- 
2.53.0

[PATCH v3 5/6] firewire: core: use context analysis annotations and lockdep checks for topology map lock

[Takashi Sakamoto]

The spinlock protects the topology map from concurrent access.

Add context analysis annotations and lockdep checks for this lock.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-topology.c    | 3 +++
 drivers/firewire/core-transaction.c | 3 +++
 include/linux/firewire.h            | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index ee6b54f89859..512b7e4473af 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -474,9 +474,12 @@ static void update_topology_map(__be32 *buffer, size_t buffer_size, int root_nod
 
 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)
+__must_not_hold(&card->topology_map.lock)
 {
 	struct fw_node *local_node;
 
+	lockdep_assert_not_held(&card->topology_map.lock);
+
 	trace_bus_reset_handle(card->index, generation, node_id, bm_abdicate, self_ids, self_id_count);
 
 	scoped_guard(spinlock, &card->lock) {
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index bd6d28f9bc50..ec04bd82831c 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -1288,9 +1288,12 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request
 		int tcode, int destination, int source, int generation,
 		unsigned long long offset, void *payload, size_t length,
 		void *callback_data)
+__must_not_hold(&card->topology_map.lock)
 {
 	int start;
 
+	lockdep_assert_not_held(&card->topology_map.lock);
+
 	if (!tcode_is_read_request(tcode)) {
 		fw_send_response(card, request, RCODE_TYPE_ERROR);
 		return;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index a3660ad1a1d4..b55c2fdd739d 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -139,7 +139,7 @@ struct fw_card {
 	u32 broadcast_channel;
 
 	struct {
-		__be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4];
+		__be32 buffer[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4] __guarded_by(&lock);
 		spinlock_t lock;
 	} topology_map;
 
-- 
2.53.0

[PATCH v3 6/6] firewire: core: narrow the card lock scope when accessing node_id

[Takashi Sakamoto]

When preparing the request subaction for an asynchronous transaction,
the node_id field in the fw_card structure is accessed under the
card-level spinlock. However, the lock is held across a call to a helper
function, unnecessarily extending the critical section.

Narrow the critical section to cover only the access to node_id.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index ec04bd82831c..b55bcfaca54a 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -453,13 +453,15 @@ __must_not_hold(&card->transactions.lock)
 	timer_setup(&t->split_timeout_timer, split_transaction_timeout_callback, 0);
 	t->packet.callback = transmit_complete_callback;
 
+	// 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) {
-		// 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);
-	}
+	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.
-- 
2.53.0

Re: [PATCH v3 0/6] firewire: core: make execution context assumptions explicit for transaction layer

[Takashi Sakamoto]

On Sun, Oct 11, 2026 at 03:31:40PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This series is take 3 of the previous series[1].
> 
> Recent code changes have guaranteed the execution contexts of several
> functions in the transaction layer. Based on this, this series adds
> might_sleep() calls where appropriate. It also adds context analysis
> annotations and lockdep checks for several locks to document the expected
> locking context and help detect incorrect locking behavior.
> 
> Chanes from v2:
> * Switch from sparse to Compiler-based context analysis assisted by Thread
>   Safety Analysis in LLVM/Clang v23 or later.
> 
> [1][PATCH v2 0/5] firewire: core: make execution context assumptions explicit
> https://lore.kernel.org/lkml/20261009121048.282982-1-o-takashi@sakamocchi.jp/
> 
> 
> Takashi Sakamoto (6):
>   firewire: core: add might_sleep() checks to transaction layer
>   firewire: core: enable context analysis for transaction layer
>   firewire: core: use context analysis annotations and lockdep checks
>     for transactions lock
>   firewire: core: use context analysis annotations and lockdep checks
>     for split timeout lock
>   firewire: core: use context analysis annotations and lockdep checks
>     for topology map lock
>   firewire: core: narrow the card lock scope when accessing node_id
> 
>  drivers/firewire/Makefile           |  2 +
>  drivers/firewire/core-topology.c    |  3 +
>  drivers/firewire/core-transaction.c | 97 +++++++++++++++++++++++------
>  include/linux/firewire.h            | 16 ++---
>  4 files changed, 91 insertions(+), 27 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto