Re: lockdep false positive? -- firewire-core transaction timer vs. scsi-core host lock

[Clemens Ladisch <clemens@ladisch.de>]

Stefan Richter wrote:
> Yong Zhang wrote:
> > I suspect it's introduced by commit 5c40cbfefa828208c671e2f58789e4dd04f79563
> > which call del_timer_sync() in softirq.
> > 
> > comments on del_timer_sync() say "It must not be called from interrupt contexts."
> 
> I hope the del_timer_sync kerneldoc comment is about hardIRQ context,

Then both the comment and its lockdep code would be wrong.

> *otherwise* commit 5c40cbfe is defective indeed.

--8<---------------------------------------------------------------->8--
firewire: core: do not use del_timer_sync() in interrupt context

Because we might be in interrupt context, replace del_timer_sync() with
del_timer().  If the timer was already running when we tried to delete
it, explicitly wait for it to finish to ensure that it does not access
the transaction data after the normal completion code might have freed
it.

Many thanks to Yong Zhang for diagnosing this and to Rusty Russell for
his Locking Guide.

Reported-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
---
 drivers/firewire/core-transaction.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -78,9 +78,16 @@ static int close_transaction(struct fw_t
 	struct fw_transaction *t;
 	unsigned long flags;
 
+retry:
 	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)) {
+				/* wait for the timer to cancel it */
+				spin_unlock_irqrestore(&card->lock, flags);
+				cpu_relax();
+				goto retry;
+			}
 			list_del_init(&t->link);
 			card->tlabel_mask &= ~(1ULL << t->tlabel);
 			break;
@@ -89,7 +96,6 @@ static int close_transaction(struct fw_t
 	spin_unlock_irqrestore(&card->lock, flags);
 
 	if (&t->link != &card->transaction_list) {
-		del_timer_sync(&t->split_timeout_timer);
 		t->callback(card, rcode, NULL, 0, t->callback_data);
 		return 0;
 	}
@@ -918,9 +924,16 @@ void fw_core_handle_response(struct fw_c
 	source	= HEADER_GET_SOURCE(p->header[1]);
 	rcode	= HEADER_GET_RCODE(p->header[1]);
 
+retry:
 	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)) {
+				/* wait for the timer to cancel it */
+				spin_unlock_irqrestore(&card->lock, flags);
+				cpu_relax();
+				goto retry;
+			}
 			list_del_init(&t->link);
 			card->tlabel_mask &= ~(1ULL << t->tlabel);
 			break;
@@ -963,8 +976,6 @@ void fw_core_handle_response(struct fw_c
 		break;
 	}
 
-	del_timer_sync(&t->split_timeout_timer);
-
 	/*
 	 * The response handler may be executed while the request handler
 	 * is still pending.  Cancel the request handler.