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

[Stefan Richter]

Clemens Ladisch wrote:
> 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().

OK.  And thanks for the pointers into the locking guide.

> If the timer is already running, we know that it will
> clean up the transaction, so we do not need to do any further processing
> in the normal transaction handler.
> 
> Many thanks to Yong Zhang for diagnosing this.
> 
> Reported-by: Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Clemens Ladisch <clemens@xxxxxxxxxx>
> ---
>  drivers/firewire/core-transaction.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> --- a/drivers/firewire/core-transaction.c
> +++ b/drivers/firewire/core-transaction.c
> @@ -81,6 +81,8 @@ static int close_transaction(struct fw_transaction *transaction,
>  	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))
> +				goto timed_out;

+			if (!del_timer(&t->split_timeout_timer)) {
+				spin_unlock_irqrestore(&card->lock, flags);
+				goto timed_out;
+			}

>  			list_del_init(&t->link);
>  			card->tlabel_mask &= ~(1ULL << t->tlabel);
>  			break;
> @@ -89,11 +91,11 @@ static int close_transaction(struct fw_transaction *transaction,
>  	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;
>  	}
>  
> +timed_out:
>  	return -ENOENT;
>  }
>  
> @@ -921,6 +923,8 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  	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))
> +				goto timed_out;

Ditto.

>  			list_del_init(&t->link);
>  			card->tlabel_mask &= ~(1ULL << t->tlabel);
>  			break;
> @@ -929,6 +933,7 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  	spin_unlock_irqrestore(&card->lock, flags);
>  
>  	if (&t->link == &card->transaction_list) {
> +timed_out:
>  		fw_notify("Unsolicited response (source %x, tlabel %x)\n",
>  			  source, tlabel);
>  		return;
> @@ -963,8 +968,6 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
>  		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.

Shall I commit with the added spin_unlocks?  Or do you prefer to send an update?
-- 
Stefan Richter
-=====-==-=- =--- =--=-
http://arcgraph.de/sr/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/