[PATCH 1/2] firewire: core: remove obsolete comment

[PATCH 1/2] firewire: core: remove obsolete comment

[Stefan Richter]

Target-like applications or peer-to-peer-like applications require the
global address handler registration which we have right now, or a per-
card registration.  And node lookup, while it would be nice to have,
would be impossible in the brief time between self-ID-complete event and
completion of firewire-core's topology scanning.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-transaction.c |    8 --------
 1 file changed, 8 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -840,14 +840,6 @@ static void handle_exclusive_region_requ
 						   offset, request->length);
 	spin_unlock_irqrestore(&address_handler_lock, flags);
 
-	/*
-	 * FIXME: lookup the fw_node corresponding to the sender of
-	 * this request and pass that to the address handler instead
-	 * of the node ID.  We may also want to move the address
-	 * allocations to fw_node so we only do this callback if the
-	 * upper layers registered it for this node.
-	 */
-
 	if (handler == NULL)
 		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
 	else


-- 
Stefan Richter
-=====-===-- --=- =--=-
http://arcgraph.de/sr/

[PATCH 2/2] firewire: core: fix race at address_handler unregistration

[Stefan Richter]

Fix the following unlikely but possible race:

CPU 1                             CPU 2
------------------------------------------------------------------------
AR-request tasklet
    lookup handler
                                  unregister handler
				  free handler->callback_data or handler
    call handler->callback

The application which registered the handler has no way to stop nodes
sending new requests to their address range, hence cannot prevent this
race.

Fix it simply by extending the address_handler_lock-protected region
from only around the lookup to around both lookup and call.  We only
need to do so in the exclusive region handler; the FCP region handler
already holds the lock around the handler->callback call.

Alas this removes the current ability to execute the callback in
parallel on different CPUs if it was called for different FireWire cards
at the same time.  (For a single card, the handler is already
serialized.)  If this loss of a rather obscure feature is not tolerable,
a more complex fix would be required:  Add a handler reference counter;
wait in fw_core_remove_address_handler() for this conter to become zero.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-transaction.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -602,6 +602,9 @@ EXPORT_SYMBOL(fw_core_add_address_handle
 
 /**
  * fw_core_remove_address_handler() - unregister an address handler
+ *
+ * When fw_core_remove_address_handler() returns, @handler->callback() is
+ * guaranteed to not run on any CPU anymore.
  */
 void fw_core_remove_address_handler(struct fw_address_handler *handler)
 {
@@ -838,16 +841,16 @@ static void handle_exclusive_region_requ
 	spin_lock_irqsave(&address_handler_lock, flags);
 	handler = lookup_enclosing_address_handler(&address_handler_list,
 						   offset, request->length);
-	spin_unlock_irqrestore(&address_handler_lock, flags);
-
-	if (handler == NULL)
-		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
-	else
+	if (handler)
 		handler->address_callback(card, request,
 					  tcode, destination, source,
 					  p->generation, offset,
 					  request->data, request->length,
 					  handler->callback_data);
+	spin_unlock_irqrestore(&address_handler_lock, flags);
+
+	if (!handler)
+		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
 }
 
 static void handle_fcp_region_request(struct fw_card *card,

-- 
Stefan Richter
-=====-===-- --=- =--=-
http://arcgraph.de/sr/

Re: [PATCH 2/2] firewire: core: fix race at address_handler unregistration

[Stefan Richter]

On Feb 18 Stefan Richter wrote:
> Fix the following unlikely but possible race:
> 
> CPU 1                             CPU 2
> ------------------------------------------------------------------------
> AR-request tasklet
>     lookup handler
>                                   unregister handler
> 				  free handler->callback_data or handler
>     call handler->callback
> 
> The application which registered the handler has no way to stop nodes
> sending new requests to their address range, hence cannot prevent this
> race.
> 
> Fix it simply by extending the address_handler_lock-protected region
> from only around the lookup to around both lookup and call.  We only
> need to do so in the exclusive region handler; the FCP region handler
> already holds the lock around the handler->callback call.
> 
> Alas this removes the current ability to execute the callback in
> parallel on different CPUs if it was called for different FireWire cards
> at the same time.  (For a single card, the handler is already
> serialized.)  If this loss of a rather obscure feature is not tolerable,
> a more complex fix would be required:  Add a handler reference counter;
> wait in fw_core_remove_address_handler() for this conter to become zero.

Oh, and the other downside is that the region in which local IRQs are
disabled is extended.  So I guess I should at least the core, maybe also
the application layer drivers, to spin_lock_bh instead, sooner than later.
-- 
Stefan Richter
-=====-===-- --=- =--=-
http://arcgraph.de/sr/

[PATCH] firewire: core: do not disable local IRQs while handling requests

[Stefan Richter]

fw_core_handle_request() is called by the low-level driver in tasklet
context or process context, and fw_core_add/remove_address_handler() is
called by mid- or high-level code in process context.  So convert
address_handler_lock accesses from those which disable local IRQs to
ones which just disable local softIRQs.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Only lightly tested without lockdep; I have yet to boot back into a
lockdep-enabled kernel.

Also to do:  Convert accesses of other locks of this kind, e.g. in
isochronous I/O handlers, and convert high-level code too.

 drivers/firewire/core-transaction.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -565,7 +565,6 @@ int fw_core_add_address_handler(struct f
 				const struct fw_address_region *region)
 {
 	struct fw_address_handler *other;
-	unsigned long flags;
 	int ret = -EBUSY;
 
 	if (region->start & 0xffff000000000003ULL ||
@@ -575,7 +574,7 @@ int fw_core_add_address_handler(struct f
 	    handler->length == 0)
 		return -EINVAL;
 
-	spin_lock_irqsave(&address_handler_lock, flags);
+	spin_lock_bh(&address_handler_lock);
 
 	handler->offset = region->start;
 	while (handler->offset + handler->length <= region->end) {
@@ -594,7 +593,7 @@ int fw_core_add_address_handler(struct f
 		}
 	}
 
-	spin_unlock_irqrestore(&address_handler_lock, flags);
+	spin_unlock_bh(&address_handler_lock);
 
 	return ret;
 }
@@ -608,11 +607,9 @@ EXPORT_SYMBOL(fw_core_add_address_handle
  */
 void fw_core_remove_address_handler(struct fw_address_handler *handler)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&address_handler_lock, flags);
+	spin_lock_bh(&address_handler_lock);
 	list_del(&handler->link);
-	spin_unlock_irqrestore(&address_handler_lock, flags);
+	spin_unlock_bh(&address_handler_lock);
 }
 EXPORT_SYMBOL(fw_core_remove_address_handler);
 
@@ -829,7 +826,6 @@ static void handle_exclusive_region_requ
 					    unsigned long long offset)
 {
 	struct fw_address_handler *handler;
-	unsigned long flags;
 	int tcode, destination, source;
 
 	destination = HEADER_GET_DESTINATION(p->header[0]);
@@ -838,7 +834,7 @@ static void handle_exclusive_region_requ
 	if (tcode == TCODE_LOCK_REQUEST)
 		tcode = 0x10 + HEADER_GET_EXTENDED_TCODE(p->header[3]);
 
-	spin_lock_irqsave(&address_handler_lock, flags);
+	spin_lock_bh(&address_handler_lock);
 	handler = lookup_enclosing_address_handler(&address_handler_list,
 						   offset, request->length);
 	if (handler)
@@ -847,7 +843,7 @@ static void handle_exclusive_region_requ
 					  p->generation, offset,
 					  request->data, request->length,
 					  handler->callback_data);
-	spin_unlock_irqrestore(&address_handler_lock, flags);
+	spin_unlock_bh(&address_handler_lock);
 
 	if (!handler)
 		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
@@ -859,7 +855,6 @@ static void handle_fcp_region_request(st
 				      unsigned long long offset)
 {
 	struct fw_address_handler *handler;
-	unsigned long flags;
 	int tcode, destination, source;
 
 	if ((offset != (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
@@ -881,7 +876,7 @@ static void handle_fcp_region_request(st
 		return;
 	}
 
-	spin_lock_irqsave(&address_handler_lock, flags);
+	spin_lock_bh(&address_handler_lock);
 	list_for_each_entry(handler, &address_handler_list, link) {
 		if (is_enclosing_handler(handler, offset, request->length))
 			handler->address_callback(card, NULL, tcode,
@@ -891,7 +886,7 @@ static void handle_fcp_region_request(st
 						  request->length,
 						  handler->callback_data);
 	}
-	spin_unlock_irqrestore(&address_handler_lock, flags);
+	spin_unlock_bh(&address_handler_lock);
 
 	fw_send_response(card, request, RCODE_COMPLETE);
 }


-- 
Stefan Richter
-=====-===-- --=- =--=-
http://arcgraph.de/sr/