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    [W:0.025 / U:30.408 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site