lkml.org 
[lkml]   [2010]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: lockdep false positive? -- firewire-core transaction timer vs. scsi-core host lock
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.

\
 
 \ /
  Last update: 2010-08-18 09:05    [from the cache]
©2003-2011 Jasper Spaans