[patch 1/4] firewire: fix race of bus reset with request transmission

[Stefan Richter]

Reported by Jay Fenlason:  A bus reset tasklet may call
fw_flush_transactions and touch transactions (call their callback which
will free them) while the context which submitted the transaction is
still inserting it into the transmission queue.

A simple solution to this problem is to _not_ "flush" the transactions
because of a bus reset (complete the transcations as 'cancelled').  They
will now simply time out (completed as 'cancelled' by the split-timeout
timer).

Jay Fenlason thought of this fix too but I was quicker to type it out.
:-)

Background:
Contexts which access an instance of struct fw_transaction are:
 1. the submitter, until it inserted the packet which is embedded in the
    transaction into the AT req DMA,
 2. the AsReqTrContext tasklet when the request packet was acked by the
    responder node or transmission to the responder failed,
 3. the AsRspRcvContext tasklet when it found a request which matched
    an incoming response,
 4. the card->flush_timer when it picks up timed-out transactions to
    cancel them,
 5. the bus reset tasklet when it cancels transactions (this access is
    eliminated by this patch),
 6. a process which shuts down an fw_card (unregisters it from fw-core
    when the controller is unbound from fw-ohci) --- although in this
    case there shouldn't really be any transactions anymore because we
    wait until all card users finished their business with the card.

All of these contexts run concurrently (except for the 6th, presumably).
The 1st is safe against the 2nd and 3rd because of the way how a request
packet is carefully submitted to the hardware.  A race between 2nd and
3rd has been fixed a while ago (bug 9617).  The 4th is almost safe
against 1st, 2nd, 3rd;  there are issues with it if huge scheduling
latencies occur, to be fixed separately.  The 5th looks safe against
2nd, 3rd, and 4th but is unsafe against 1st.  Maybe this could be fixed
with an explicit state variable in struct fw_transaction.  But this
would require fw_transaction to be rewritten as only dynamically
allocatable object with reference counting --- not a good solution if we
also can simply kill this 5th accessing context (replace it by the 4th).

Signed-off-by: Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx>
---
 drivers/firewire/fw-topology.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -510,8 +510,6 @@ fw_core_handle_bus_reset(struct fw_card 
 	struct fw_node *local_node;
 	unsigned long flags;
 
-	fw_flush_transactions(card);
-
 	/*
 	 * If the selfID buffer is not the immediate successor of the
 	 * previously processed one, we cannot reliably compare the

-- 
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/