firewire: core: remove obsolete comment

PATCH In review Stefan Richter 1 participant 4 messages Feb 18, 2012 Firewire

Share & tools

Stefan Richter

author

2012-02-18 18:53 UTC #0 lore raw

PATCH 1/2 firewire: core: remove obsolete comment

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 <[email protected]>

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

drivers/firewire/core-transaction.c +0 -8

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

Signature · 3 lines

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

Stefan Richter

author

2012-02-18 18:55 UTC #1 lore raw

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

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 <[email protected]>

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

drivers/firewire/core-transaction.c +8 -5

@@ -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,

Signature · 3 lines

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

Stefan Richter

author

2012-02-18 19:11 UTC #2 lore raw

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

Quoted 19 lines · 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.

Quoted 6 lines

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.

Signature · 3 lines

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

Stefan Richter

author

2012-02-18 19:42 UTC #3 lore raw

PATCH 1/1 firewire: core: do not disable local IRQs while handling requests

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 <[email protected]>

---
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(-)

drivers/firewire/core-transaction.c +8 ~~-13~~

@@ -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);
 }

Signature · 3 lines

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