firewire: use split transaction timeout only for split transactions

Instead of starting the split transaction timeout timer when any request is submitted, start it only when the destination's ACK_PENDING has been received. This prevents us from using a timeout that is too short, and, if the controller's AT queue is emptying very slowly, from cancelling a packet that has not yet been sent. Signed-off-by: Clemens Ladisch <clemens@ladisch.de> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

firewire: use split transaction timeout only for split transactions
Instead of starting the split transaction timeout timer when any request is submitted, start it only when the destination's ACK_PENDING has been received. This prevents us from using a timeout that is too short, and, if the controller's AT queue is emptying very slowly, from cancelling a packet that has not yet been sent. Signed-off-by: Clemens Ladisch <clemens@ladisch.de> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Clemens Ladisch · Stefan Richter
1 parent 693a50b511
Showing 2 changed files with 33 additions and 14 deletions Side-by-side Diff
drivers/firewire/core-transaction.c
include/linux/firewire.h
@@ -72,6 +72,15 @@
 #define PHY_CONFIG_ROOT_ID(node_id)	((((node_id) & 0x3f) << 24) | (1 << 23))
 #define PHY_IDENTIFIER(id)		((id) << 30)
  
+/* returns 0 if the split timeout handler is already running */
+static int try_cancel_split_timeout(struct fw_transaction *t)
+{
+	if (t->is_split_transaction)
+		return del_timer(&t->split_timeout_timer);
+	else
+		return 1;
+}
+
 static int close_transaction(struct fw_transaction *transaction,
 			     struct fw_card *card, int rcode)
 {
@@ -81,7 +90,7 @@
 	spin_lock_irqsave(&card->lock, flags);
 	list_for_each_entry(t, &card->transaction_list, link) {
 		if (t == transaction) {
-			if (!del_timer(&t->split_timeout_timer)) {
+			if (!try_cancel_split_timeout(t)) {
 				spin_unlock_irqrestore(&card->lock, flags);
 				goto timed_out;
 			}
  
@@ -141,16 +150,28 @@
 	card->tlabel_mask &= ~(1ULL << t->tlabel);
 	spin_unlock_irqrestore(&card->lock, flags);
  
-	card->driver->cancel_packet(card, &t->packet);
-
-	/*
-	 * At this point cancel_packet will never call the transaction
-	 * callback, since we just took the transaction out of the list.
-	 * So do it here.
-	 */
 	t->callback(card, RCODE_CANCELLED, NULL, 0, t->callback_data);
 }
  
+static void start_split_transaction_timeout(struct fw_transaction *t,
+					    struct fw_card *card)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&card->lock, flags);
+
+	if (list_empty(&t->link) || WARN_ON(t->is_split_transaction)) {
+		spin_unlock_irqrestore(&card->lock, flags);
+		return;
+	}
+
+	t->is_split_transaction = true;
+	mod_timer(&t->split_timeout_timer,
+		  jiffies + card->split_timeout_jiffies);
+
+	spin_unlock_irqrestore(&card->lock, flags);
+}
+
 static void transmit_complete_callback(struct fw_packet *packet,
 				       struct fw_card *card, int status)
 {
@@ -162,7 +183,7 @@
 		close_transaction(t, card, RCODE_COMPLETE);
 		break;
 	case ACK_PENDING:
-		t->timestamp = packet->timestamp;
+		start_split_transaction_timeout(t, card);
 		break;
 	case ACK_BUSY_X:
 	case ACK_BUSY_A:
  
@@ -349,11 +370,9 @@
 	t->node_id = destination_id;
 	t->tlabel = tlabel;
 	t->card = card;
+	t->is_split_transaction = false;
 	setup_timer(&t->split_timeout_timer,
 		    split_transaction_timeout_callback, (unsigned long)t);
-	/* FIXME: start this timer later, relative to t->timestamp */
-	mod_timer(&t->split_timeout_timer,
-		  jiffies + card->split_timeout_jiffies);
 	t->callback = callback;
 	t->callback_data = callback_data;
  
@@ -926,7 +945,7 @@
 	spin_lock_irqsave(&card->lock, flags);
 	list_for_each_entry(t, &card->transaction_list, link) {
 		if (t->node_id == source && t->tlabel == tlabel) {
-			if (!del_timer(&t->split_timeout_timer)) {
+			if (!try_cancel_split_timeout(t)) {
 				spin_unlock_irqrestore(&card->lock, flags);
 				goto timed_out;
 			}
@@ -302,9 +302,9 @@
 struct fw_transaction {
 	int node_id; /* The generation is implied; it is always the current. */
 	int tlabel;
-	int timestamp;
 	struct list_head link;
 	struct fw_card *card;
+	bool is_split_transaction;
 	struct timer_list split_timeout_timer;
  
 	struct fw_packet packet;
...	...	@@ -72,6 +72,15 @@
72	72	#define PHY_CONFIG_ROOT_ID(node_id) ((((node_id) & 0x3f) << 24) \| (1 << 23))
73	73	#define PHY_IDENTIFIER(id) ((id) << 30)
74	74
	75	+/* returns 0 if the split timeout handler is already running */
	76	+static int try_cancel_split_timeout(struct fw_transaction *t)
	77	+{
	78	+ if (t->is_split_transaction)
	79	+ return del_timer(&t->split_timeout_timer);
	80	+ else
	81	+ return 1;
	82	+}
	83	+
75	84	static int close_transaction(struct fw_transaction *transaction,
76	85	struct fw_card *card, int rcode)
77	86	{
...	...	@@ -81,7 +90,7 @@
81	90	spin_lock_irqsave(&card->lock, flags);
82	91	list_for_each_entry(t, &card->transaction_list, link) {
83	92	if (t == transaction) {
84		- if (!del_timer(&t->split_timeout_timer)) {
	93	+ if (!try_cancel_split_timeout(t)) {
85	94	spin_unlock_irqrestore(&card->lock, flags);
86	95	goto timed_out;
87	96	}
88	97
...	...	@@ -141,16 +150,28 @@
141	150	card->tlabel_mask &= ~(1ULL << t->tlabel);
142	151	spin_unlock_irqrestore(&card->lock, flags);
143	152
144		- card->driver->cancel_packet(card, &t->packet);
145		-
146		- /*
147		- * At this point cancel_packet will never call the transaction
148		- * callback, since we just took the transaction out of the list.
149		- * So do it here.
150		- */
151	153	t->callback(card, RCODE_CANCELLED, NULL, 0, t->callback_data);
152	154	}
153	155
	156	+static void start_split_transaction_timeout(struct fw_transaction *t,
	157	+ struct fw_card *card)
	158	+{
	159	+ unsigned long flags;
	160	+
	161	+ spin_lock_irqsave(&card->lock, flags);
	162	+
	163	+ if (list_empty(&t->link) \|\| WARN_ON(t->is_split_transaction)) {
	164	+ spin_unlock_irqrestore(&card->lock, flags);
	165	+ return;
	166	+ }
	167	+
	168	+ t->is_split_transaction = true;
	169	+ mod_timer(&t->split_timeout_timer,
	170	+ jiffies + card->split_timeout_jiffies);
	171	+
	172	+ spin_unlock_irqrestore(&card->lock, flags);
	173	+}
	174	+
154	175	static void transmit_complete_callback(struct fw_packet *packet,
155	176	struct fw_card *card, int status)
156	177	{
...	...	@@ -162,7 +183,7 @@
162	183	close_transaction(t, card, RCODE_COMPLETE);
163	184	break;
164	185	case ACK_PENDING:
165		- t->timestamp = packet->timestamp;
	186	+ start_split_transaction_timeout(t, card);
166	187	break;
167	188	case ACK_BUSY_X:
168	189	case ACK_BUSY_A:
169	190
...	...	@@ -349,11 +370,9 @@
349	370	t->node_id = destination_id;
350	371	t->tlabel = tlabel;
351	372	t->card = card;
	373	+ t->is_split_transaction = false;
352	374	setup_timer(&t->split_timeout_timer,
353	375	split_transaction_timeout_callback, (unsigned long)t);
354		- /* FIXME: start this timer later, relative to t->timestamp */
355		- mod_timer(&t->split_timeout_timer,
356		- jiffies + card->split_timeout_jiffies);
357	376	t->callback = callback;
358	377	t->callback_data = callback_data;
359	378
...	...	@@ -926,7 +945,7 @@
926	945	spin_lock_irqsave(&card->lock, flags);
927	946	list_for_each_entry(t, &card->transaction_list, link) {
928	947	if (t->node_id == source && t->tlabel == tlabel) {
929		- if (!del_timer(&t->split_timeout_timer)) {
	948	+ if (!try_cancel_split_timeout(t)) {
930	949	spin_unlock_irqrestore(&card->lock, flags);
931	950	goto timed_out;
932	951	}
...	...	@@ -302,9 +302,9 @@
302	302	struct fw_transaction {
303	303	int node_id; /* The generation is implied; it is always the current. */
304	304	int tlabel;
305		- int timestamp;
306	305	struct list_head link;
307	306	struct fw_card *card;
	307	+ bool is_split_transaction;
308	308	struct timer_list split_timeout_timer;
309	309
310	310	struct fw_packet packet;