[PATCH 0/2] firewire: core: remove unnecessary work cancellation

[PATCH 0/2] firewire: core: remove unnecessary work cancellation

[Takashi Sakamoto]

Hi,

The error callback work is scheduled only after the transaction instance
has been removed from the pending list, making it pointless to cancel
the work by iterating over the list.

This series removes the unnecessary cancellation and uses the existing
helper function to schedule error callbacks.


Takashi Sakamoto (2):
  firewire: core: don't disable error work for transaction
  firewire: core: reuse helper function for scheduling error callbacks

 drivers/firewire/core-transaction.c | 30 +++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)


base-commit: 82d59eb7f5431771cfc6fb074b74f466cd4e3353
-- 
2.53.0

[PATCH 1/2] firewire: core: don't disable error work for transaction

[Takashi Sakamoto]

The error callback work is scheduled only when the transaction instance
is no longer on the pending list. Therefore, iterating over the list to
cancel the work is pointless.

Remove the code that cancels the work while iterating over the pending
list. Once scheduled, the work can run and invoke the error callback
without being cancelled.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 48558857e726..029bd9cdc570 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -44,7 +44,7 @@ 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);
+		return timer_delete(&t->split_timeout_timer);
 	else
 		return 1;
 }
@@ -146,16 +146,22 @@ __must_not_hold(&card->transactions.lock)
 	transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time);
 	transaction->rcode = RCODE_CANCELLED;
 
-	enable_and_queue_work(card->async_wq, &transaction->error_work);
+	queue_work(card->async_wq, &transaction->error_work);
 
 	return 0;
 }
 EXPORT_SYMBOL(fw_cancel_transaction);
 
+// Ensure that the transaction is not on the pending list when scheduling this work. Cancelling
+// the work after scheduling makes no sense, as the initiator must be notified of the transaction
+// result.
 static void error_callback_work(struct work_struct *work)
 {
 	struct fw_transaction *t = from_work(t, work, error_work);
 
+	// To detect programming mistakes.
+	WARN_ON(!list_empty(&t->link));
+
 	invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0);
 }
 
@@ -414,6 +420,7 @@ __must_not_hold(&card->transactions.lock)
 	t->with_tstamp = with_tstamp;
 	t->callback_data = callback_data;
 	INIT_WORK(&t->error_work, error_callback_work);
+	INIT_LIST_HEAD(&t->link);
 
 	/*
 	 * Allocate tlabel from the bitmap and put the transaction on
-- 
2.53.0

[PATCH 2/2] firewire: core: reuse helper function for scheduling error callbacks

[Takashi Sakamoto]

The transaction layer has some code that can be replaced with the
existing helper function for scheduling error callbacks.

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

diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c
index 029bd9cdc570..45114da64422 100644
--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -106,6 +106,14 @@ __must_not_hold(&card->transactions.lock)
 	__t;								\
 })
 
+static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp)
+{
+	t->rcode = rcode;
+	t->response_timestamp = response_timestamp;
+
+	queue_work(t->card->async_wq, &t->error_work);
+}
+
 /**
  * fw_cancel_transaction: cancel transaction.
  * @card:		interface where the transaction has been requested
@@ -143,10 +151,9 @@ __must_not_hold(&card->transactions.lock)
 
 	// Timestamping on behalf of hardware.
 	(void)fw_card_read_cycle_time(card, &curr_cycle_time);
-	transaction->response_timestamp = cycle_time_to_ohci_tstamp(curr_cycle_time);
-	transaction->rcode = RCODE_CANCELLED;
 
-	queue_work(card->async_wq, &transaction->error_work);
+	schedule_error_callback(transaction, RCODE_CANCELLED,
+				cycle_time_to_ohci_tstamp(curr_cycle_time));
 
 	return 0;
 }
@@ -165,14 +172,6 @@ static void error_callback_work(struct work_struct *work)
 	invoke_callback(t, t->rcode, t->response_timestamp, NULL, 0);
 }
 
-static void schedule_error_callback(struct fw_transaction *t, int rcode, u32 response_timestamp)
-{
-	t->rcode = rcode;
-	t->response_timestamp = response_timestamp;
-
-	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)
 {
-- 
2.53.0

Re: [PATCH 0/2] firewire: core: remove unnecessary work cancellation

[Takashi Sakamoto]

On Sun, Oct 18, 2026 at 12:33:37AM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> The error callback work is scheduled only after the transaction instance
> has been removed from the pending list, making it pointless to cancel
> the work by iterating over the list.
> 
> This series removes the unnecessary cancellation and uses the existing
> helper function to schedule error callbacks.
> 
> 
> Takashi Sakamoto (2):
>   firewire: core: don't disable error work for transaction
>   firewire: core: reuse helper function for scheduling error callbacks
> 
>  drivers/firewire/core-transaction.c | 30 +++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto