[PATCH 0/5] firewire: fixes, status update

[PATCH 0/5] firewire: fixes, status update

[Stefan Richter]

Following as replies:  Some patches which are IMO suitable to be merged
during the .33-rc time frame.

The first patch enabled me to run the three FCP clients FFADO, Kino
(both libraw1394 based userspace), and firedtv (kernelspace) at the same
time.  Thanks to Clemens Ladisch.

firewire: fix use of multiple AV/C devices, allow multiple FCP listeners
firewire: cdev: fix another memory leak in an error path
firewire: ohci: always use packet-per-buffer mode for isochronous reception
firewire, ieee1394: update MAINTAINERS entries
firewire, ieee1394: update Kconfig help

 MAINTAINERS                             |   15 ---
 drivers/Kconfig                         |    2 
 drivers/firewire/Kconfig                |   44 ++------
 drivers/firewire/core-cdev.c            |   27 +++--
 drivers/firewire/core-transaction.c     |  118 +++++++++++++++++++-----
 drivers/firewire/ohci.c                 |    4 
 drivers/ieee1394/Kconfig                |   59 ++++++++----
 drivers/media/dvb/firewire/firedtv-fw.c |   12 --
 include/linux/firewire-cdev.h           |    3 
 include/linux/firewire.h                |    4 
 10 files changed, 182 insertions(+), 106 deletions(-)
-- 
Stefan Richter
-=====-==--= ==-- ==-=-
http://arcgraph.de/sr/

[PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Stefan Richter]

Date: Thu, 24 Dec 2009 12:05:58 +0100
From: Clemens Ladisch <cladisch@fastmail.net>

Control of more than one AV/C device at once --- e.g. camcorders, tape
decks, audio devices, TV tuners --- failed or worked only unreliably,
depending on driver implementation.  This affected kernelspace and
userspace drivers alike and was caused by firewire-core's inability to
accept multiple registrations of FCP listeners.

The fix allows multiple address handlers to be registered for the FCP
command and response registers.  When a request for these registers is
received, all handlers are invoked, and the Firewire response is
generated by the core and not by any handler.

The cdev API does not change, i.e., userspace is still expected to send
a response for FCP requests; this response is silently ignored.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (changelog, rebased, whitespace)
---
 drivers/firewire/core-cdev.c            |   26 +++--
 drivers/firewire/core-transaction.c     |  118 ++++++++++++++++++++----
 drivers/media/dvb/firewire/firedtv-fw.c |   12 --
 include/linux/firewire-cdev.h           |    3 +
 include/linux/firewire.h                |    4 
 5 files changed, 119 insertions(+), 44 deletions(-)

Index: linux-2.6.33-rc2/include/linux/firewire.h
===================================================================
--- linux-2.6.33-rc2.orig/include/linux/firewire.h
+++ linux-2.6.33-rc2/include/linux/firewire.h
@@ -248,8 +248,8 @@ typedef void (*fw_transaction_callback_t
 					  void *data, size_t length,
 					  void *callback_data);
 /*
- * Important note:  The callback must guarantee that either fw_send_response()
- * or kfree() is called on the @request.
+ * Important note:  Except for the FCP registers, the callback must guarantee
+ * that either fw_send_response() or kfree() is called on the @request.
  */
 typedef void (*fw_address_callback_t)(struct fw_card *card,
 				      struct fw_request *request,
Index: linux-2.6.33-rc2/include/linux/firewire-cdev.h
===================================================================
--- linux-2.6.33-rc2.orig/include/linux/firewire-cdev.h
+++ linux-2.6.33-rc2/include/linux/firewire-cdev.h
@@ -340,6 +340,9 @@ struct fw_cdev_send_response {
  * The @closure field is passed back to userspace in the response event.
  * The @handle field is an out parameter, returning a handle to the allocated
  * range to be used for later deallocation of the range.
+ *
+ * The address range is allocated on all local nodes.  The address allocation
+ * is exclusive except for the FCP command and response registers.
  */
 struct fw_cdev_allocate {
 	__u64 offset;
Index: linux-2.6.33-rc2/drivers/firewire/core-transaction.c
===================================================================
--- linux-2.6.33-rc2.orig/drivers/firewire/core-transaction.c
+++ linux-2.6.33-rc2/drivers/firewire/core-transaction.c
@@ -432,14 +432,20 @@ static struct fw_address_handler *lookup
 	return NULL;
 }
 
+static bool is_enclosing_handler(struct fw_address_handler *handler,
+				 unsigned long long offset, size_t length)
+{
+	return handler->offset <= offset &&
+		offset + length <= handler->offset + handler->length;
+}
+
 static struct fw_address_handler *lookup_enclosing_address_handler(
 	struct list_head *list, unsigned long long offset, size_t length)
 {
 	struct fw_address_handler *handler;
 
 	list_for_each_entry(handler, list, link) {
-		if (handler->offset <= offset &&
-		    offset + length <= handler->offset + handler->length)
+		if (is_enclosing_handler(handler, offset, length))
 			return handler;
 	}
 
@@ -465,6 +471,12 @@ const struct fw_address_region fw_unit_s
 	{ .start = 0xfffff0000900ULL, .end = 0x1000000000000ULL, };
 #endif  /*  0  */
 
+static bool is_in_fcp_region(u64 offset, size_t length)
+{
+	return offset >= (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
+		offset + length <= (CSR_REGISTER_BASE | CSR_FCP_END);
+}
+
 /**
  * fw_core_add_address_handler - register for incoming requests
  * @handler: callback
@@ -477,8 +489,11 @@ const struct fw_address_region fw_unit_s
  * give the details of the particular request.
  *
  * Return value:  0 on success, non-zero otherwise.
+ *
  * The start offset of the handler's address region is determined by
  * fw_core_add_address_handler() and is returned in handler->offset.
+ *
+ * Address allocations are exclusive, except for the FCP registers.
  */
 int fw_core_add_address_handler(struct fw_address_handler *handler,
 				const struct fw_address_region *region)
@@ -498,10 +513,12 @@ int fw_core_add_address_handler(struct f
 
 	handler->offset = region->start;
 	while (handler->offset + handler->length <= region->end) {
-		other =
-		    lookup_overlapping_address_handler(&address_handler_list,
-						       handler->offset,
-						       handler->length);
+		if (is_in_fcp_region(handler->offset, handler->length))
+			other = NULL;
+		else
+			other = lookup_overlapping_address_handler
+					(&address_handler_list,
+					 handler->offset, handler->length);
 		if (other != NULL) {
 			handler->offset += other->length;
 		} else {
@@ -668,6 +685,9 @@ static struct fw_request *allocate_reque
 void fw_send_response(struct fw_card *card,
 		      struct fw_request *request, int rcode)
 {
+	if (WARN_ONCE(!request, "invalid for FCP address handlers"))
+		return;
+
 	/* unified transaction or broadcast transaction: don't respond */
 	if (request->ack != ACK_PENDING ||
 	    HEADER_DESTINATION_IS_BROADCAST(request->request_header[0])) {
@@ -686,26 +706,15 @@ void fw_send_response(struct fw_card *ca
 }
 EXPORT_SYMBOL(fw_send_response);
 
-void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
+static void handle_exclusive_region_request(struct fw_card *card,
+					    struct fw_packet *p,
+					    struct fw_request *request,
+					    unsigned long long offset)
 {
 	struct fw_address_handler *handler;
-	struct fw_request *request;
-	unsigned long long offset;
 	unsigned long flags;
 	int tcode, destination, source;
 
-	if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE)
-		return;
-
-	request = allocate_request(p);
-	if (request == NULL) {
-		/* FIXME: send statically allocated busy packet. */
-		return;
-	}
-
-	offset      =
-		((unsigned long long)
-		 HEADER_GET_OFFSET_HIGH(p->header[1]) << 32) | p->header[2];
 	tcode       = HEADER_GET_TCODE(p->header[0]);
 	destination = HEADER_GET_DESTINATION(p->header[0]);
 	source      = HEADER_GET_SOURCE(p->header[1]);
@@ -732,6 +741,73 @@ void fw_core_handle_request(struct fw_ca
 					  request->data, request->length,
 					  handler->callback_data);
 }
+
+static void handle_fcp_region_request(struct fw_card *card,
+				      struct fw_packet *p,
+				      struct fw_request *request,
+				      unsigned long long offset)
+{
+	struct fw_address_handler *handler;
+	unsigned long flags;
+	int tcode, destination, source;
+
+	if ((offset != (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
+	     offset != (CSR_REGISTER_BASE | CSR_FCP_RESPONSE)) ||
+	    request->length > 0x200) {
+		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
+
+		return;
+	}
+
+	tcode       = HEADER_GET_TCODE(p->header[0]);
+	destination = HEADER_GET_DESTINATION(p->header[0]);
+	source      = HEADER_GET_SOURCE(p->header[1]);
+
+	if (tcode != TCODE_WRITE_QUADLET_REQUEST &&
+	    tcode != TCODE_WRITE_BLOCK_REQUEST) {
+		fw_send_response(card, request, RCODE_TYPE_ERROR);
+
+		return;
+	}
+
+	spin_lock_irqsave(&address_handler_lock, flags);
+	list_for_each_entry(handler, &address_handler_list, link) {
+		if (is_enclosing_handler(handler, offset, request->length))
+			handler->address_callback(card, NULL, tcode,
+						  destination, source,
+						  p->generation, p->speed,
+						  offset, request->data,
+						  request->length,
+						  handler->callback_data);
+	}
+	spin_unlock_irqrestore(&address_handler_lock, flags);
+
+	fw_send_response(card, request, RCODE_COMPLETE);
+}
+
+void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
+{
+	struct fw_request *request;
+	unsigned long long offset;
+
+	if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE)
+		return;
+
+	request = allocate_request(p);
+	if (request == NULL) {
+		/* FIXME: send statically allocated busy packet. */
+		return;
+	}
+
+	offset = ((u64)HEADER_GET_OFFSET_HIGH(p->header[1]) << 32) |
+		p->header[2];
+
+	if (!is_in_fcp_region(offset, request->length))
+		handle_exclusive_region_request(card, p, request, offset);
+	else
+		handle_fcp_region_request(card, p, request, offset);
+
+}
 EXPORT_SYMBOL(fw_core_handle_request);
 
 void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
Index: linux-2.6.33-rc2/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.33-rc2.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.33-rc2/drivers/firewire/core-cdev.c
@@ -601,8 +601,9 @@ static void release_request(struct clien
 	struct inbound_transaction_resource *r = container_of(resource,
 			struct inbound_transaction_resource, resource);
 
-	fw_send_response(client->device->card, r->request,
-			 RCODE_CONFLICT_ERROR);
+	if (r->request)
+		fw_send_response(client->device->card, r->request,
+				 RCODE_CONFLICT_ERROR);
 	kfree(r);
 }
 
@@ -645,7 +646,8 @@ static void handle_request(struct fw_car
  failed:
 	kfree(r);
 	kfree(e);
-	fw_send_response(card, request, RCODE_CONFLICT_ERROR);
+	if (request)
+		fw_send_response(card, request, RCODE_CONFLICT_ERROR);
 }
 
 static void release_address_handler(struct client *client,
@@ -715,15 +717,17 @@ static int ioctl_send_response(struct cl
 
 	r = container_of(resource, struct inbound_transaction_resource,
 			 resource);
-	if (request->length < r->length)
-		r->length = request->length;
-
-	if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) {
-		ret = -EFAULT;
-		goto out;
+	if (r->request) {
+		if (request->length < r->length)
+			r->length = request->length;
+		if (copy_from_user(r->data, u64_to_uptr(request->data),
+				   r->length)) {
+			ret = -EFAULT;
+			goto out;
+		}
+		fw_send_response(client->device->card, r->request,
+				 request->rcode);
 	}
-
-	fw_send_response(client->device->card, r->request, request->rcode);
  out:
 	kfree(r);
 
Index: linux-2.6.33-rc2/drivers/media/dvb/firewire/firedtv-fw.c
===================================================================
--- linux-2.6.33-rc2.orig/drivers/media/dvb/firewire/firedtv-fw.c
+++ linux-2.6.33-rc2/drivers/media/dvb/firewire/firedtv-fw.c
@@ -202,14 +202,8 @@ static void handle_fcp(struct fw_card *c
 	unsigned long flags;
 	int su;
 
-	if ((tcode != TCODE_WRITE_QUADLET_REQUEST &&
-	     tcode != TCODE_WRITE_BLOCK_REQUEST) ||
-	    offset != CSR_REGISTER_BASE + CSR_FCP_RESPONSE ||
-	    length == 0 ||
-	    (((u8 *)payload)[0] & 0xf0) != 0) {
-		fw_send_response(card, request, RCODE_TYPE_ERROR);
+	if (length < 2 || (((u8 *)payload)[0] & 0xf0) != 0)
 		return;
-	}
 
 	su = ((u8 *)payload)[1] & 0x7;
 
@@ -230,10 +224,8 @@ static void handle_fcp(struct fw_card *c
 	}
 	spin_unlock_irqrestore(&node_list_lock, flags);
 
-	if (fdtv) {
+	if (fdtv)
 		avc_recv(fdtv, payload, length);
-		fw_send_response(card, request, RCODE_COMPLETE);
-	}
 }
 
 static struct fw_address_handler fcp_handler = {

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

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Pieter Palmers]

Stefan Richter wrote:
> Date: Thu, 24 Dec 2009 12:05:58 +0100
> From: Clemens Ladisch <cladisch@fastmail.net>
> 
> Control of more than one AV/C device at once --- e.g. camcorders, tape
> decks, audio devices, TV tuners --- failed or worked only unreliably,
> depending on driver implementation.  This affected kernelspace and
> userspace drivers alike and was caused by firewire-core's inability to
> accept multiple registrations of FCP listeners.
> 
> The fix allows multiple address handlers to be registered for the FCP
> command and response registers.  When a request for these registers is
> received, all handlers are invoked, and the Firewire response is
> generated by the core and not by any handler.

I'm not convinced that this patch makes sense. I can't come up with a 
scenario where multiple listeners would want access to an FCP transaction.

If the application is the originator of the request, it's also the only 
one that can make sense out of the response as in the general case you 
need to know what you asked in order to know what the response means. So 
there is little use in having multiple receivers for a response.

I case the application is the FCP target, there is also little use in 
multiplexing as it discards any responses generated by the application, 
rendering it useless as an FCP target.

IMHO it is impossible to reliably match an FCP response to its request 
without relying on ordering. There is no information in the FCP frame to 
allow other mechanisms. If two applications send the same AV/C command, 
but with slightly different parameters, there is no way to tell what 
response is meant for what application.

Furthermore, some (most?) of the devices don't support incoming FCP 
commands while another one is still being processed. In any case they 
don't have to, and can send a 'REJECTED' response to this second 
request. Where does this response end up?

After re-reading section 8 of the AV/C Digital Interface Command Set 
General Specification v4 (TA1999026), I think that the only way to 
reliably implement this is by moving the FCP and AV/C transaction logic 
into the kernel.

I don't see how you can reliably implement the following scenario using 
the current API/ABI:

"""
APP1 sends AVC_cmd_1 to device
device responds with INTERIM

APP2 sends AVC_cmd_1 to device
device responds with REJECTED as it doesn't support two AVC_cmd_1's
this response should not end up at APP1, as that has a correct request 
pending.

device sends response to INTERIM, which should end up at APP1.
"""

I think this can only be implemented by tracking which AVC commands have 
been sent to what device, regardless of the userspace application (or 
kernel driver) that sent them, and disallowing any transactions that 
result in scenarios such as the above.


Greets,

Pieter

> 
> The cdev API does not change, i.e., userspace is still expected to send
> a response for FCP requests; this response is silently ignored.
> 
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (changelog, rebased, whitespace)
> ---
>  drivers/firewire/core-cdev.c            |   26 +++--
>  drivers/firewire/core-transaction.c     |  118 ++++++++++++++++++++----
>  drivers/media/dvb/firewire/firedtv-fw.c |   12 --
>  include/linux/firewire-cdev.h           |    3 +
>  include/linux/firewire.h                |    4 
>  5 files changed, 119 insertions(+), 44 deletions(-)
> 
> Index: linux-2.6.33-rc2/include/linux/firewire.h
> ===================================================================
> --- linux-2.6.33-rc2.orig/include/linux/firewire.h
> +++ linux-2.6.33-rc2/include/linux/firewire.h
> @@ -248,8 +248,8 @@ typedef void (*fw_transaction_callback_t
>  					  void *data, size_t length,
>  					  void *callback_data);
>  /*
> - * Important note:  The callback must guarantee that either fw_send_response()
> - * or kfree() is called on the @request.
> + * Important note:  Except for the FCP registers, the callback must guarantee
> + * that either fw_send_response() or kfree() is called on the @request.
>   */
>  typedef void (*fw_address_callback_t)(struct fw_card *card,
>  				      struct fw_request *request,
> Index: linux-2.6.33-rc2/include/linux/firewire-cdev.h
> ===================================================================
> --- linux-2.6.33-rc2.orig/include/linux/firewire-cdev.h
> +++ linux-2.6.33-rc2/include/linux/firewire-cdev.h
> @@ -340,6 +340,9 @@ struct fw_cdev_send_response {
>   * The @closure field is passed back to userspace in the response event.
>   * The @handle field is an out parameter, returning a handle to the allocated
>   * range to be used for later deallocation of the range.
> + *
> + * The address range is allocated on all local nodes.  The address allocation
> + * is exclusive except for the FCP command and response registers.
>   */
>  struct fw_cdev_allocate {
>  	__u64 offset;
> Index: linux-2.6.33-rc2/drivers/firewire/core-transaction.c
> ===================================================================
> --- linux-2.6.33-rc2.orig/drivers/firewire/core-transaction.c
> +++ linux-2.6.33-rc2/drivers/firewire/core-transaction.c
> @@ -432,14 +432,20 @@ static struct fw_address_handler *lookup
>  	return NULL;
>  }
>  
> +static bool is_enclosing_handler(struct fw_address_handler *handler,
> +				 unsigned long long offset, size_t length)
> +{
> +	return handler->offset <= offset &&
> +		offset + length <= handler->offset + handler->length;
> +}
> +
>  static struct fw_address_handler *lookup_enclosing_address_handler(
>  	struct list_head *list, unsigned long long offset, size_t length)
>  {
>  	struct fw_address_handler *handler;
>  
>  	list_for_each_entry(handler, list, link) {
> -		if (handler->offset <= offset &&
> -		    offset + length <= handler->offset + handler->length)
> +		if (is_enclosing_handler(handler, offset, length))
>  			return handler;
>  	}
>  
> @@ -465,6 +471,12 @@ const struct fw_address_region fw_unit_s
>  	{ .start = 0xfffff0000900ULL, .end = 0x1000000000000ULL, };
>  #endif  /*  0  */
>  
> +static bool is_in_fcp_region(u64 offset, size_t length)
> +{
> +	return offset >= (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
> +		offset + length <= (CSR_REGISTER_BASE | CSR_FCP_END);
> +}
> +
>  /**
>   * fw_core_add_address_handler - register for incoming requests
>   * @handler: callback
> @@ -477,8 +489,11 @@ const struct fw_address_region fw_unit_s
>   * give the details of the particular request.
>   *
>   * Return value:  0 on success, non-zero otherwise.
> + *
>   * The start offset of the handler's address region is determined by
>   * fw_core_add_address_handler() and is returned in handler->offset.
> + *
> + * Address allocations are exclusive, except for the FCP registers.
>   */
>  int fw_core_add_address_handler(struct fw_address_handler *handler,
>  				const struct fw_address_region *region)
> @@ -498,10 +513,12 @@ int fw_core_add_address_handler(struct f
>  
>  	handler->offset = region->start;
>  	while (handler->offset + handler->length <= region->end) {
> -		other =
> -		    lookup_overlapping_address_handler(&address_handler_list,
> -						       handler->offset,
> -						       handler->length);
> +		if (is_in_fcp_region(handler->offset, handler->length))
> +			other = NULL;
> +		else
> +			other = lookup_overlapping_address_handler
> +					(&address_handler_list,
> +					 handler->offset, handler->length);
>  		if (other != NULL) {
>  			handler->offset += other->length;
>  		} else {
> @@ -668,6 +685,9 @@ static struct fw_request *allocate_reque
>  void fw_send_response(struct fw_card *card,
>  		      struct fw_request *request, int rcode)
>  {
> +	if (WARN_ONCE(!request, "invalid for FCP address handlers"))
> +		return;
> +
>  	/* unified transaction or broadcast transaction: don't respond */
>  	if (request->ack != ACK_PENDING ||
>  	    HEADER_DESTINATION_IS_BROADCAST(request->request_header[0])) {
> @@ -686,26 +706,15 @@ void fw_send_response(struct fw_card *ca
>  }
>  EXPORT_SYMBOL(fw_send_response);
>  
> -void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
> +static void handle_exclusive_region_request(struct fw_card *card,
> +					    struct fw_packet *p,
> +					    struct fw_request *request,
> +					    unsigned long long offset)
>  {
>  	struct fw_address_handler *handler;
> -	struct fw_request *request;
> -	unsigned long long offset;
>  	unsigned long flags;
>  	int tcode, destination, source;
>  
> -	if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE)
> -		return;
> -
> -	request = allocate_request(p);
> -	if (request == NULL) {
> -		/* FIXME: send statically allocated busy packet. */
> -		return;
> -	}
> -
> -	offset      =
> -		((unsigned long long)
> -		 HEADER_GET_OFFSET_HIGH(p->header[1]) << 32) | p->header[2];
>  	tcode       = HEADER_GET_TCODE(p->header[0]);
>  	destination = HEADER_GET_DESTINATION(p->header[0]);
>  	source      = HEADER_GET_SOURCE(p->header[1]);
> @@ -732,6 +741,73 @@ void fw_core_handle_request(struct fw_ca
>  					  request->data, request->length,
>  					  handler->callback_data);
>  }
> +
> +static void handle_fcp_region_request(struct fw_card *card,
> +				      struct fw_packet *p,
> +				      struct fw_request *request,
> +				      unsigned long long offset)
> +{
> +	struct fw_address_handler *handler;
> +	unsigned long flags;
> +	int tcode, destination, source;
> +
> +	if ((offset != (CSR_REGISTER_BASE | CSR_FCP_COMMAND) &&
> +	     offset != (CSR_REGISTER_BASE | CSR_FCP_RESPONSE)) ||
> +	    request->length > 0x200) {
> +		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
> +
> +		return;
> +	}
> +
> +	tcode       = HEADER_GET_TCODE(p->header[0]);
> +	destination = HEADER_GET_DESTINATION(p->header[0]);
> +	source      = HEADER_GET_SOURCE(p->header[1]);
> +
> +	if (tcode != TCODE_WRITE_QUADLET_REQUEST &&
> +	    tcode != TCODE_WRITE_BLOCK_REQUEST) {
> +		fw_send_response(card, request, RCODE_TYPE_ERROR);
> +
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&address_handler_lock, flags);
> +	list_for_each_entry(handler, &address_handler_list, link) {
> +		if (is_enclosing_handler(handler, offset, request->length))
> +			handler->address_callback(card, NULL, tcode,
> +						  destination, source,
> +						  p->generation, p->speed,
> +						  offset, request->data,
> +						  request->length,
> +						  handler->callback_data);
> +	}
> +	spin_unlock_irqrestore(&address_handler_lock, flags);
> +
> +	fw_send_response(card, request, RCODE_COMPLETE);
> +}
> +
> +void fw_core_handle_request(struct fw_card *card, struct fw_packet *p)
> +{
> +	struct fw_request *request;
> +	unsigned long long offset;
> +
> +	if (p->ack != ACK_PENDING && p->ack != ACK_COMPLETE)
> +		return;
> +
> +	request = allocate_request(p);
> +	if (request == NULL) {
> +		/* FIXME: send statically allocated busy packet. */
> +		return;
> +	}
> +
> +	offset = ((u64)HEADER_GET_OFFSET_HIGH(p->header[1]) << 32) |
> +		p->header[2];
> +
> +	if (!is_in_fcp_region(offset, request->length))
> +		handle_exclusive_region_request(card, p, request, offset);
> +	else
> +		handle_fcp_region_request(card, p, request, offset);
> +
> +}
>  EXPORT_SYMBOL(fw_core_handle_request);
>  
>  void fw_core_handle_response(struct fw_card *card, struct fw_packet *p)
> Index: linux-2.6.33-rc2/drivers/firewire/core-cdev.c
> ===================================================================
> --- linux-2.6.33-rc2.orig/drivers/firewire/core-cdev.c
> +++ linux-2.6.33-rc2/drivers/firewire/core-cdev.c
> @@ -601,8 +601,9 @@ static void release_request(struct clien
>  	struct inbound_transaction_resource *r = container_of(resource,
>  			struct inbound_transaction_resource, resource);
>  
> -	fw_send_response(client->device->card, r->request,
> -			 RCODE_CONFLICT_ERROR);
> +	if (r->request)
> +		fw_send_response(client->device->card, r->request,
> +				 RCODE_CONFLICT_ERROR);
>  	kfree(r);
>  }
>  
> @@ -645,7 +646,8 @@ static void handle_request(struct fw_car
>   failed:
>  	kfree(r);
>  	kfree(e);
> -	fw_send_response(card, request, RCODE_CONFLICT_ERROR);
> +	if (request)
> +		fw_send_response(card, request, RCODE_CONFLICT_ERROR);
>  }
>  
>  static void release_address_handler(struct client *client,
> @@ -715,15 +717,17 @@ static int ioctl_send_response(struct cl
>  
>  	r = container_of(resource, struct inbound_transaction_resource,
>  			 resource);
> -	if (request->length < r->length)
> -		r->length = request->length;
> -
> -	if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) {
> -		ret = -EFAULT;
> -		goto out;
> +	if (r->request) {
> +		if (request->length < r->length)
> +			r->length = request->length;
> +		if (copy_from_user(r->data, u64_to_uptr(request->data),
> +				   r->length)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +		fw_send_response(client->device->card, r->request,
> +				 request->rcode);
>  	}
> -
> -	fw_send_response(client->device->card, r->request, request->rcode);
>   out:
>  	kfree(r);
>  
> Index: linux-2.6.33-rc2/drivers/media/dvb/firewire/firedtv-fw.c
> ===================================================================
> --- linux-2.6.33-rc2.orig/drivers/media/dvb/firewire/firedtv-fw.c
> +++ linux-2.6.33-rc2/drivers/media/dvb/firewire/firedtv-fw.c
> @@ -202,14 +202,8 @@ static void handle_fcp(struct fw_card *c
>  	unsigned long flags;
>  	int su;
>  
> -	if ((tcode != TCODE_WRITE_QUADLET_REQUEST &&
> -	     tcode != TCODE_WRITE_BLOCK_REQUEST) ||
> -	    offset != CSR_REGISTER_BASE + CSR_FCP_RESPONSE ||
> -	    length == 0 ||
> -	    (((u8 *)payload)[0] & 0xf0) != 0) {
> -		fw_send_response(card, request, RCODE_TYPE_ERROR);
> +	if (length < 2 || (((u8 *)payload)[0] & 0xf0) != 0)
>  		return;
> -	}
>  
>  	su = ((u8 *)payload)[1] & 0x7;
>  
> @@ -230,10 +224,8 @@ static void handle_fcp(struct fw_card *c
>  	}
>  	spin_unlock_irqrestore(&node_list_lock, flags);
>  
> -	if (fdtv) {
> +	if (fdtv)
>  		avc_recv(fdtv, payload, length);
> -		fw_send_response(card, request, RCODE_COMPLETE);
> -	}
>  }
>  
>  static struct fw_address_handler fcp_handler = {
> 

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Stefan Richter]

Pieter Palmers wrote:
> Stefan Richter wrote:
>> Date: Thu, 24 Dec 2009 12:05:58 +0100
>> From: Clemens Ladisch <cladisch@fastmail.net>
>>
>> Control of more than one AV/C device at once --- e.g. camcorders, tape
>> decks, audio devices, TV tuners --- failed or worked only unreliably,
>> depending on driver implementation.  This affected kernelspace and
>> userspace drivers alike and was caused by firewire-core's inability to
>> accept multiple registrations of FCP listeners.
>>
>> The fix allows multiple address handlers to be registered for the FCP
>> command and response registers.  When a request for these registers is
>> received, all handlers are invoked, and the Firewire response is
>> generated by the core and not by any handler.
> 
> I'm not convinced that this patch makes sense.

Oh yes, it makes a lot of sense because it simply fixes firewire-core
for the status quo.

To remind, the status quo is in the old stack as well as in the new
stack:  It is the responsibility of applications (high-level drivers)
  - to sort out whether an FCP request or FCP response is destined to
    them,
  - to serialize their own FCP transactions.
Neither ieee1394 core nor firewire-core cares for any of the above yet,
nor do they serialize FCP transactions to particular nodes or units or
subunits.

With this status quo in mind, the patch does nothing more or less than
bringing firewire-core's FCP related functionality to the same level as
ieee1394 core's.  The design of FCP handling in firewire-core and
ieee1394 is the same, only firewire-core's implementation of it was
lacking until now.

But what you are criticizing is the design, not the implementation:

> I can't come up with a
> scenario where multiple listeners would want access to an FCP transaction.
> 
> If the application is the originator of the request, it's also the only
> one that can make sense out of the response as in the general case you
> need to know what you asked in order to know what the response means. So
> there is little use in having multiple receivers for a response.

True.  We only have multiple receivers now because the kernel is not
(yet) in a position to sort out who the receiver is.

> I case the application is the FCP target, there is also little use in
> multiplexing as it discards any responses generated by the application,
> rendering it useless as an FCP target.

This is a misunderstandig of the term "Firewire response" in the
changelog.  The patch only takes care to supersede FCP related IEEE 1394
write response packets from applications (high-level drivers) by its own
write response packet.  This is in contrast to _FCP responses_ which are
of course not discarded.

On the terminology:  One FCP transaction is encapsulated into two IEEE
1394 transactions (in case of the typical immediate FCP transaction,
three IEEE 1394 transactions in case of deferred FCP transactions).
Typical case:
  - FCP request from A to B ==
    #1 IEEE 1394 block write request from A to B's FCP_Request CSR,
    #2 IEEE 1394 block write response from B to A,
  - FCP response from B to A ==
    #3 IEEE 1394 block write request from B to A's FCP_Response CSR,
    #4 IEEE 1394 block write response from A to B.
A is a controller, B is a target.  The IEEE 1394 write response can be
either a write response packet or an ack_complete packet; in case of
OHCI 1394 based controllers or targets, it is the former.

A Linux kernel on B generates packet #2 on behalf of 0...n FCP target
drivers running on B, and a Linux kernel on A generates packet #4 on
behalf of 0...n FCP controller drivers running on A.  Packets #1 and #3
are generated by one of the FCP drivers and passed through by a Linux
kernel without looking at them (yet).

> IMHO it is impossible to reliably match an FCP response to its request
> without relying on ordering. There is no information in the FCP frame to
> allow other mechanisms. If two applications send the same AV/C command,
> but with slightly different parameters, there is no way to tell what
> response is meant for what application.

True.  Though it has not been a practical problem yet.  Well, it has not
been a practical problem for video yet (camcorders, set top boxes etc.).
I am saying this based on linux1394-devel and linux1394-user traffic
over many years now and based on occasional querying of distribution
bugzillas.

If it is a practical problem for audio, then I have not heard of it
until now.  I guess you may want to run several independent processes
which concurrently control the same FCP target, do you?  (Streaming
driver == jack backend or ALSA backend, routing driver == mixer panel, ...)

> Furthermore, some (most?) of the devices don't support incoming FCP
> commands while another one is still being processed. In any case they
> don't have to, and can send a 'REJECTED' response to this second
> request. Where does this response end up?

Like with other responses, all FCP listeners get this response and match
it with their respective pending requests.  As you describe, this
becomes ambiguous if there is more than one pending request to the same
subunit, carrying the same command.

> After re-reading section 8 of the AV/C Digital Interface Command Set
> General Specification v4 (TA1999026), I think that the only way to
> reliably implement this is by moving the FCP and AV/C transaction logic
> into the kernel.
> 
> I don't see how you can reliably implement the following scenario using
> the current API/ABI:
> 
> """
> APP1 sends AVC_cmd_1 to device
> device responds with INTERIM
> 
> APP2 sends AVC_cmd_1 to device
> device responds with REJECTED as it doesn't support two AVC_cmd_1's
> this response should not end up at APP1, as that has a correct request
> pending.
> 
> device sends response to INTERIM, which should end up at APP1.
> """
> 
> I think this can only be implemented by tracking which AVC commands have
> been sent to what device, regardless of the userspace application (or
> kernel driver) that sent them, and disallowing any transactions that
> result in scenarios such as the above.

You are right about all this, but it does not devalue the discussed fix
patch.

If or when somebody designs a programming interface and implements the
infrastructure with the features that you described, the functionality
which this patch merely fixes can be removed, but not before.

Of course a hard requirement of such an interface is that the FCP
related parts of present libraw1394 v2 API can be layered on top of it.
It is a dated and deficient API, but it is here to stay.  (I.e. the
three FCP listening related functions and the generic IEEE 1394 write
transaction functions.)  A soft requirement is that present libraw1394
implementations remain functional.  (Without having it thought through
yet, I think both requirements can be meat without disproportional effort.)

Until then, the immediately necessary task is to fix the bug that
firewire-core does not allow more than one FCP listener at a time.  And
this is what Clemens' patch is doing perfectly.

---------------
PS:  I noted your request for kernel-assisted serialized FCP
transactions at
http://ieee1394.wiki.kernel.org/index.php/To_Do#firewire-core .

Of course such kernel infrastructure does not cover the case of several
controllers running on different nodes, controlling the same target.  In
this case, whether it all works still depends on the capabilities and
correctness of the controllers and the target.
-- 
Stefan Richter
-=====-==--= ==-- ==-=-
http://arcgraph.de/sr/

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Pieter Palmers]

Stefan Richter wrote:
> Pieter Palmers wrote:
>> Stefan Richter wrote:
>>> Date: Thu, 24 Dec 2009 12:05:58 +0100
>>> From: Clemens Ladisch <cladisch@fastmail.net>
>>>
>>> Control of more than one AV/C device at once --- e.g. camcorders, tape
>>> decks, audio devices, TV tuners --- failed or worked only unreliably,
>>> depending on driver implementation.  This affected kernelspace and
>>> userspace drivers alike and was caused by firewire-core's inability to
>>> accept multiple registrations of FCP listeners.
>>>
>>> The fix allows multiple address handlers to be registered for the FCP
>>> command and response registers.  When a request for these registers is
>>> received, all handlers are invoked, and the Firewire response is
>>> generated by the core and not by any handler.
>> I'm not convinced that this patch makes sense.
> 
> Oh yes, it makes a lot of sense because it simply fixes firewire-core
> for the status quo.
> 
> To remind, the status quo is in the old stack as well as in the new
> stack:  It is the responsibility of applications (high-level drivers)
>   - to sort out whether an FCP request or FCP response is destined to
>     them,
>   - to serialize their own FCP transactions.
> Neither ieee1394 core nor firewire-core cares for any of the above yet,
> nor do they serialize FCP transactions to particular nodes or units or
> subunits.
> 
> With this status quo in mind, the patch does nothing more or less than
> bringing firewire-core's FCP related functionality to the same level as
> ieee1394 core's.  The design of FCP handling in firewire-core and
> ieee1394 is the same, only firewire-core's implementation of it was
> lacking until now.

OK, I see.

> 
> But what you are criticizing is the design, not the implementation:

[...]

>> IMHO it is impossible to reliably match an FCP response to its request
>> without relying on ordering. There is no information in the FCP frame to
>> allow other mechanisms. If two applications send the same AV/C command,
>> but with slightly different parameters, there is no way to tell what
>> response is meant for what application.
> 
> True.  Though it has not been a practical problem yet.  Well, it has not
> been a practical problem for video yet (camcorders, set top boxes etc.).
> I am saying this based on linux1394-devel and linux1394-user traffic
> over many years now and based on occasional querying of distribution
> bugzillas.
> 
> If it is a practical problem for audio, then I have not heard of it
> until now.  I guess you may want to run several independent processes
> which concurrently control the same FCP target, do you?  (Streaming
> driver == jack backend or ALSA backend, routing driver == mixer panel, ...)

I'm pretty confident that it is a practical problem, but a bit similar 
to a race condition. In any case, we have different processes for audio 
and control indeed, and I have seen them interfere. If you start both 
the audio process and the mixer simultaneously, failure is highly likely 
as the discovery process uses some commands very frequently.

This is not a 'practical' problem because most people (learn to) leave 
enough time between starting jackd and starting the mixer. This makes 
things usable, but I don't really consider this to be a satisfactory 
solution.

> 
>> Furthermore, some (most?) of the devices don't support incoming FCP
>> commands while another one is still being processed. In any case they
>> don't have to, and can send a 'REJECTED' response to this second
>> request. Where does this response end up?
> 
> Like with other responses, all FCP listeners get this response and match
> it with their respective pending requests.  As you describe, this
> becomes ambiguous if there is more than one pending request to the same
> subunit, carrying the same command.
> 
>> After re-reading section 8 of the AV/C Digital Interface Command Set
>> General Specification v4 (TA1999026), I think that the only way to
>> reliably implement this is by moving the FCP and AV/C transaction logic
>> into the kernel.
>>
>> I don't see how you can reliably implement the following scenario using
>> the current API/ABI:
>>
>> """
>> APP1 sends AVC_cmd_1 to device
>> device responds with INTERIM
>>
>> APP2 sends AVC_cmd_1 to device
>> device responds with REJECTED as it doesn't support two AVC_cmd_1's
>> this response should not end up at APP1, as that has a correct request
>> pending.
>>
>> device sends response to INTERIM, which should end up at APP1.
>> """
>>
>> I think this can only be implemented by tracking which AVC commands have
>> been sent to what device, regardless of the userspace application (or
>> kernel driver) that sent them, and disallowing any transactions that
>> result in scenarios such as the above.
> 
> You are right about all this, but it does not devalue the discussed fix
> patch.
> 
> If or when somebody designs a programming interface and implements the
> infrastructure with the features that you described, the functionality
> which this patch merely fixes can be removed, but not before.
> 
> Of course a hard requirement of such an interface is that the FCP
> related parts of present libraw1394 v2 API can be layered on top of it.
> It is a dated and deficient API, but it is here to stay.  (I.e. the
> three FCP listening related functions and the generic IEEE 1394 write
> transaction functions.)  A soft requirement is that present libraw1394
> implementations remain functional.  (Without having it thought through
> yet, I think both requirements can be meat without disproportional effort.)
> 
> Until then, the immediately necessary task is to fix the bug that
> firewire-core does not allow more than one FCP listener at a time.  And
> this is what Clemens' patch is doing perfectly.

I see this, it was a misunderstanding on my part. Sorry for that.

Pieter

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Stefan Richter]

Pieter Palmers wrote:
[lack of system-wide FCP transaction serialization]
> This is not a 'practical' problem because most people (learn to) leave
> enough time between starting jackd and starting the mixer. This makes
> things usable, but I don't really consider this to be a satisfactory
> solution.

Ah, so this is yet another of those software flaws which are not talked
about because people take the workaround as something inevitable.

>>> I think this can only be implemented by tracking which AVC commands have
>>> been sent to what device, regardless of the userspace application (or
>>> kernel driver) that sent them, and disallowing any transactions that
>>> result in scenarios such as the above.

BTW, what about AV/C RESERVE?  According to the spec, it means that a
controller gets exclusive FCP access to a target.  At this point, a
node-wide FCP serializer will have to decide on a policy:  The simpler
policy would be to remain oblivious of RESERVE.  The more difficult to
implement policy would be to keep track of reservations and only allow a
single controller process (or user, or...) to perform subsequent
transactions for the duration of a reservation.  (I for one would keep
it simple.)
-- 
Stefan Richter
-=====-==--= ==-- ==-=-
http://arcgraph.de/sr/

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Stefan Richter]

Stefan Richter wrote:
> BTW, what about AV/C RESERVE?  According to the spec, it means that a
> controller gets exclusive FCP access to a target.  At this point, a
> node-wide FCP serializer will have to decide on a policy:

(because the spec does not have a concept of reservation IDs.  It
stupidly takes node IDs for what should be reservation IDs even though
it distinguishes between the concepts of 'controller' and 'node'.)

> The simpler policy would be to remain oblivious of RESERVE.  The more
> difficult to implement policy would be to keep track of reservations
> and only allow a single controller process (or user, or...) to perform
> subsequent transactions for the duration of a reservation.

Err, no, forget that.  A reservation prevents only control commands, but
AFAIU not even all control commands because subunit specifications may
enumerate control commands that are still allowed during a reservation.
So the serializer would have to have knowledge of subunit specs.  That
would quickly get out of hand.

> (I for one would keep it simple.)
-- 
Stefan Richter
-=====-==--= ==-- ==-=-
http://arcgraph.de/sr/

Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

[Stefan Richter]

Stefan Richter wrote:
> Date: Thu, 24 Dec 2009 12:05:58 +0100
> From: Clemens Ladisch <cladisch@fastmail.net>
> 
> Control of more than one AV/C device at once --- e.g. camcorders, tape
> decks, audio devices, TV tuners --- failed or worked only unreliably,
> depending on driver implementation.  This affected kernelspace and
> userspace drivers alike and was caused by firewire-core's inability to
> accept multiple registrations of FCP listeners.
> 
> The fix allows multiple address handlers to be registered for the FCP
> command and response registers.  When a request for these registers is
> received, all handlers are invoked, and the Firewire response is
> generated by the core and not by any handler.
> 
> The cdev API does not change, i.e., userspace is still expected to send
> a response for FCP requests; this response is silently ignored.
> 
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (changelog, rebased, whitespace)

I tested this on another box which has more kernel debug options enabled
than my current mainly used box.  Alas, there is a serious regression on
that box:

1.) testlibraw now always shows:

  - testing FCP monitoring on local node
    got fcp command from node 0 of 8 bytes:ERROR: fcp payload not correct
 6b 6b 6b 6b 6b 6b 6b 6b
    got fcp response from node 0 of 8 bytes:ERROR: fcp payload not correct
 6b 6b 6b 6b 6b 6b 6b 6b

2.) gscanbus's AV/C controls work but are now very quick to segfault.

3.) Kino frequently shows 6b:6b:6b:6b as timestamp when a DV camcorder
in "record" mode is connected.

4.) Kino is unable to determine the presence of a DV camcorder if the
camcorder is in "play" mode.

Only the AV/C kernel driver firedtv still works (tested with kaffeine).

0x6b is the POISON_FREE pattern in <linux/poison.h>.  So, we apparently
have a use-after-free issue with FCP responses in firewire-core's cdev
interface now.

None of this happened before the patch.  (There this box with otherwise
identical kernel and modules only exhibits the problem that was fixed by
the patch, i.e. no more than one FCP listener possible at a time.)
-- 
Stefan Richter
-=====-==-=- ---= =-===
http://arcgraph.de/sr/

[PATCH] firewire: core: fix use-after-free regression in FCP handler

[Stefan Richter]

Commit db5d247a "firewire: fix use of multiple AV/C devices, allow
multiple FCP listeners" introduced a regression into 2.6.33-rc3:
The core freed payloads of incoming requests to FCP_Request or
FCP_Response before a userspace driver accessed them.

We need to copy such payloads for each registered userspace client
and free the copies according to the lifetime rules of non-FCP client
request resources.

(This could possibly be optimized by reference counts instead of
copies.)

The presently only kernelspace driver which listens for FCP requests,
firedtv, was not affected because it already copies FCP frames into an
own buffer before returning to firewire-core's FCP handler dispatcher.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-cdev.c |   50 +++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 14 deletions(-)

Index: linux-2.6.32.2/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.32.2.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.32.2/drivers/firewire/core-cdev.c
@@ -35,6 +35,7 @@
 #include <linux/preempt.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
+#include <linux/string.h>
 #include <linux/time.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
@@ -595,13 +596,20 @@ static int ioctl_send_request(struct cli
 			    client->device->max_speed);
 }
 
+static inline bool is_fcp_request(struct fw_request *request)
+{
+	return request == NULL;
+}
+
 static void release_request(struct client *client,
 			    struct client_resource *resource)
 {
 	struct inbound_transaction_resource *r = container_of(resource,
 			struct inbound_transaction_resource, resource);
 
-	if (r->request)
+	if (is_fcp_request(r->request))
+		kfree(r->data);
+	else
 		fw_send_response(client->device->card, r->request,
 				 RCODE_CONFLICT_ERROR);
 	kfree(r);
@@ -616,6 +624,7 @@ static void handle_request(struct fw_car
 	struct address_handler_resource *handler = callback_data;
 	struct inbound_transaction_resource *r;
 	struct inbound_transaction_event *e;
+	void *fcp_frame = NULL;
 	int ret;
 
 	r = kmalloc(sizeof(*r), GFP_ATOMIC);
@@ -627,6 +636,18 @@ static void handle_request(struct fw_car
 	r->data    = payload;
 	r->length  = length;
 
+	if (is_fcp_request(request)) {
+		/*
+		 * FIXME: Let core-transaction.c manage a
+		 * single reference-counted copy?
+		 */
+		fcp_frame = kmemdup(payload, length, GFP_ATOMIC);
+		if (fcp_frame == NULL)
+			goto failed;
+
+		r->data = fcp_frame;
+	}
+
 	r->resource.release = release_request;
 	ret = add_client_resource(handler->client, &r->resource, GFP_ATOMIC);
 	if (ret < 0)
@@ -640,13 +661,15 @@ static void handle_request(struct fw_car
 	e->request.closure = handler->closure;
 
 	queue_event(handler->client, &e->event,
-		    &e->request, sizeof(e->request), payload, length);
+		    &e->request, sizeof(e->request), r->data, length);
 	return;
 
  failed:
 	kfree(r);
 	kfree(e);
-	if (request)
+	kfree(fcp_frame);
+
+	if (!is_fcp_request(request))
 		fw_send_response(card, request, RCODE_CONFLICT_ERROR);
 }
 
@@ -717,18 +740,17 @@ static int ioctl_send_response(struct cl
 
 	r = container_of(resource, struct inbound_transaction_resource,
 			 resource);
-	if (r->request) {
-		if (request->length < r->length)
-			r->length = request->length;
-		if (copy_from_user(r->data, u64_to_uptr(request->data),
-				   r->length)) {
-			ret = -EFAULT;
-			kfree(r->request);
-			goto out;
-		}
-		fw_send_response(client->device->card, r->request,
-				 request->rcode);
+	if (is_fcp_request(r->request))
+		goto out;
+
+	if (request->length < r->length)
+		r->length = request->length;
+	if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) {
+		ret = -EFAULT;
+		kfree(r->request);
+		goto out;
 	}
+	fw_send_response(client->device->card, r->request, request->rcode);
  out:
 	kfree(r);
 

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

[PATCH 2/5] firewire: cdev: fix another memory leak in an error path

[Stefan Richter]

If copy_from_user in an FW_CDEV_IOC_SEND_RESPONSE ioctl failed, the
fw_request pointed to by the inbound_transaction_resource is no
longer referenced and needs to be freed.

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

Index: linux-2.6.33-rc2/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.33-rc2.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.33-rc2/drivers/firewire/core-cdev.c
@@ -723,6 +723,7 @@ static int ioctl_send_response(struct cl
 		if (copy_from_user(r->data, u64_to_uptr(request->data),
 				   r->length)) {
 			ret = -EFAULT;
+			kfree(r->request);
 			goto out;
 		}
 		fw_send_response(client->device->card, r->request,

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

[PATCH 3/5] firewire: ohci: always use packet-per-buffer mode for isochronous reception

[Stefan Richter]

This is a minimal change meant for the short term:  Never set the
ohci->use_dualbuffer flag to true.

There are two reasons to do so:

  - Packet-per-buffer mode and dual-buffer mode do not behave the same
    under certain circumstances, notably if several packets are covered
    by a single fw_cdev_iso_packet descriptor.
    http://marc.info/?l=linux1394-devel&m=124965653718313
    Therefore the driver stack should not silently choose one or the
    other mode but should leave the choice to the high-level driver
    (regardless if kernel driver or userspace driver).  Or simply always
    only offer packet-per-buffer mode, since a considerable number of
    controllers, even current ones, does not offer dual-buffer support.

  - Even under circumstances where packet-per-buffer mode and
    dual-buffer mode behave exactly the same --- notably when used
    through libraw1394, libdc1394, as well as the current two kernel
    drivers which use isochronous reception (firewire-net and firedtv)
    --- we are still faced with the problem that several OHCI 1.1
    controllers have bugs in dual-buffer mode.  Although it looks like
    we have identified most of those buggy controllers by now, we
    cannot be quite sure about that.

So, use packet-per-buffer by default from now on.  This change should
be followed up by a more complete solution:  Either extend the
in-kernel API and the userspace ABI by a choice between the two IR modes
or remove all dual-buffer related code from firewire-ohci.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/ohci.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.33-rc2/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.33-rc2.orig/drivers/firewire/ohci.c
+++ linux-2.6.33-rc2/drivers/firewire/ohci.c
@@ -2226,7 +2226,6 @@ static int ohci_queue_iso_receive_dualbu
 	if (rest == 0)
 		return -EINVAL;
 
-	/* FIXME: make packet-per-buffer/dual-buffer a context option */
 	while (rest > 0) {
 		d = context_get_descriptors(&ctx->context,
 					    z + header_z, &d_bus);
@@ -2470,7 +2469,10 @@ static int __devinit pci_probe(struct pc
 	}
 
 	version = reg_read(ohci, OHCI1394_Version) & 0x00ff00ff;
+#if 0
+	/* FIXME: make it a context option or remove dual-buffer mode */
 	ohci->use_dualbuffer = version >= OHCI_VERSION_1_1;
+#endif
 
 	/* dual-buffer mode is broken if more than one IR context is active */
 	if (dev->vendor == PCI_VENDOR_ID_AGERE &&

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

[PATCH 4/5] firewire, ieee1394: update MAINTAINERS entries

[Stefan Richter]

Ben and Kristian have not been involved in maintenance of the IEEE 1394
drivers for quite some time; submitters are not required to Cc them on
patches.

The linux1394.org domain has been dead for a while and is no longer
under control of a Linux developer.  The current web site of the
Linux 1394 project is http://ieee1394.wiki.kernel.org/.

The classic drivers/ieee1394/ stack is now obsolete from the development
point of view, though still a useful alternative in productive use.  But
nobody should attempt to submit style cleanup patches for it or to
develop new drivers on top of this stack, hence mark its MAINTAINERS
entry as Obsolete.

drivers/ieee1394/raw1394*, like the rest of the old stack, does not
receive bigger code changes anymore, hence shrink the MAINTAINERS
database a bit by dropping raw1394's special entry.  If something
important and urgent is going to come up for raw1394, I will make sure
that Dan will be notified of it besides via linux1394-devel.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Ben Collins <ben.collins@ubuntu.com>
Cc: Dan Dennedy <dan@dennedy.org>
Cc: Kristian Hoegsberg <krh@bitplanet.net>
---
 MAINTAINERS |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

Index: linux-2.6.33-rc2/MAINTAINERS
===================================================================
--- linux-2.6.33-rc2.orig/MAINTAINERS
+++ linux-2.6.33-rc2/MAINTAINERS
@@ -2169,10 +2169,9 @@ F:	drivers/hwmon/f75375s.c
 F:	include/linux/f75375s.h
 
 FIREWIRE SUBSYSTEM
-M:	Kristian Hoegsberg <krh@redhat.com>
 M:	Stefan Richter <stefanr@s5r6.in-berlin.de>
 L:	linux1394-devel@lists.sourceforge.net
-W:	http://www.linux1394.org/
+W:	http://ieee1394.wiki.kernel.org/
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git
 S:	Maintained
 F:	drivers/firewire/
@@ -2705,22 +2704,14 @@ S:	Supported
 F:	drivers/idle/i7300_idle.c
 
 IEEE 1394 SUBSYSTEM
-M:	Ben Collins <ben.collins@ubuntu.com>
 M:	Stefan Richter <stefanr@s5r6.in-berlin.de>
 L:	linux1394-devel@lists.sourceforge.net
-W:	http://www.linux1394.org/
+W:	http://ieee1394.wiki.kernel.org/
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git
-S:	Maintained
+S:	Obsolete
 F:	Documentation/debugging-via-ohci1394.txt
 F:	drivers/ieee1394/
 
-IEEE 1394 RAW I/O DRIVER
-M:	Dan Dennedy <dan@dennedy.org>
-M:	Stefan Richter <stefanr@s5r6.in-berlin.de>
-L:	linux1394-devel@lists.sourceforge.net
-S:	Maintained
-F:	drivers/ieee1394/raw1394*
-
 IEEE 802.15.4 SUBSYSTEM
 M:	Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
 M:	Sergey Lapin <slapin@ossfans.org>

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

Re: [PATCH 4/5] firewire, ieee1394: update MAINTAINERS entries

[Dan Dennedy]

Hi

On Fri, Dec 25, 2009 at 4:36 PM, Stefan Richter
<stefanr@s5r6.in-berlin.de> wrote:
> Ben and Kristian have not been involved in maintenance of the IEEE 1394
> drivers for quite some time; submitters are not required to Cc them on
> patches.
>
> The linux1394.org domain has been dead for a while and is no longer
> under control of a Linux developer.  The current web site of the
> Linux 1394 project is http://ieee1394.wiki.kernel.org/.
>
> The classic drivers/ieee1394/ stack is now obsolete from the development
> point of view, though still a useful alternative in productive use.  But
> nobody should attempt to submit style cleanup patches for it or to
> develop new drivers on top of this stack, hence mark its MAINTAINERS
> entry as Obsolete.
>
> drivers/ieee1394/raw1394*, like the rest of the old stack, does not
> receive bigger code changes anymore, hence shrink the MAINTAINERS
> database a bit by dropping raw1394's special entry.  If something
> important and urgent is going to come up for raw1394, I will make sure
> that Dan will be notified of it besides via linux1394-devel.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> Cc: Ben Collins <ben.collins@ubuntu.com>
> Cc: Dan Dennedy <dan@dennedy.org>
> Cc: Kristian Hoegsberg <krh@bitplanet.net>

Signed-off-by: Dan Dennedy <dan@dennedy.org>

> ---
>  MAINTAINERS |   15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> Index: linux-2.6.33-rc2/MAINTAINERS
> ===================================================================
> --- linux-2.6.33-rc2.orig/MAINTAINERS
> +++ linux-2.6.33-rc2/MAINTAINERS
> @@ -2169,10 +2169,9 @@ F:       drivers/hwmon/f75375s.c
>  F:     include/linux/f75375s.h
>
>  FIREWIRE SUBSYSTEM
> -M:     Kristian Hoegsberg <krh@redhat.com>
>  M:     Stefan Richter <stefanr@s5r6.in-berlin.de>
>  L:     linux1394-devel@lists.sourceforge.net
> -W:     http://www.linux1394.org/
> +W:     http://ieee1394.wiki.kernel.org/
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git
>  S:     Maintained
>  F:     drivers/firewire/
> @@ -2705,22 +2704,14 @@ S:      Supported
>  F:     drivers/idle/i7300_idle.c
>
>  IEEE 1394 SUBSYSTEM
> -M:     Ben Collins <ben.collins@ubuntu.com>
>  M:     Stefan Richter <stefanr@s5r6.in-berlin.de>
>  L:     linux1394-devel@lists.sourceforge.net
> -W:     http://www.linux1394.org/
> +W:     http://ieee1394.wiki.kernel.org/
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git
> -S:     Maintained
> +S:     Obsolete
>  F:     Documentation/debugging-via-ohci1394.txt
>  F:     drivers/ieee1394/
>
> -IEEE 1394 RAW I/O DRIVER
> -M:     Dan Dennedy <dan@dennedy.org>
> -M:     Stefan Richter <stefanr@s5r6.in-berlin.de>
> -L:     linux1394-devel@lists.sourceforge.net
> -S:     Maintained
> -F:     drivers/ieee1394/raw1394*
> -
>  IEEE 802.15.4 SUBSYSTEM
>  M:     Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>  M:     Sergey Lapin <slapin@ossfans.org>
>

[PATCH 5/5] firewire, ieee1394: update Kconfig help

[Stefan Richter]

Update the Kconfig help texts of both stacks to encourage a general move
from the older to the newer drivers.  However, do not label ieee1394 as
"Obsolete" yet, as the newer drivers have not been deployed as default
stack in the majority of Linux distributions yet, and those who start
doing so now may still want to install the old drivers as fallback for
unforeseen issues.

Since Linux 2.6.32, FireWire audio devices can be driven by the newer
firewire driver stack too, hence remove an outdated comment about audio
devices.  Also remove comments about library versions since the 2nd
generation of libraw1394 and libdc1394 is now in common use; details on
library versions can be read at the wiki link from the help texts.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/Kconfig          |    2 
 drivers/firewire/Kconfig |   44 +++++++++--------------------
 drivers/ieee1394/Kconfig |   59 ++++++++++++++++++++++++++++-----------
 3 files changed, 56 insertions(+), 49 deletions(-)

Index: linux-2.6.33-rc2/drivers/Kconfig
===================================================================
--- linux-2.6.33-rc2.orig/drivers/Kconfig
+++ linux-2.6.33-rc2/drivers/Kconfig
@@ -28,7 +28,7 @@ source "drivers/md/Kconfig"
 
 source "drivers/message/fusion/Kconfig"
 
-source "drivers/ieee1394/Kconfig"
+source "drivers/firewire/Kconfig"
 
 source "drivers/message/i2o/Kconfig"
 
Index: linux-2.6.33-rc2/drivers/firewire/Kconfig
===================================================================
--- linux-2.6.33-rc2.orig/drivers/firewire/Kconfig
+++ linux-2.6.33-rc2/drivers/firewire/Kconfig
@@ -1,5 +1,10 @@
+menu "IEEE 1394 (FireWire) support"
+	depends on PCI || BROKEN
+	# firewire-core does not depend on PCI but is
+	# not useful without PCI controller driver
+
 comment "You can enable one or both FireWire driver stacks."
-comment "See the help texts for more information."
+comment "The newer stack is recommended."
 
 config FIREWIRE
 	tristate "FireWire driver stack"
@@ -15,16 +20,6 @@ config FIREWIRE
 	  To compile this driver as a module, say M here: the module will be
 	  called firewire-core.
 
-	  This module functionally replaces ieee1394, raw1394, and video1394.
-	  To access it from application programs, you generally need at least
-	  libraw1394 v2.  IIDC/DCAM applications need libdc1394 v2.
-	  No libraries are required to access storage devices through the
-	  firewire-sbp2 driver.
-
-	  NOTE:
-	  FireWire audio devices currently require the old drivers (ieee1394,
-	  ohci1394, raw1394).
-
 config FIREWIRE_OHCI
 	tristate "OHCI-1394 controllers"
 	depends on PCI && FIREWIRE
@@ -34,22 +29,7 @@ config FIREWIRE_OHCI
 	  is the only chipset in use, so say Y here.
 
 	  To compile this driver as a module, say M here:  The module will be
-	  called firewire-ohci.  It replaces ohci1394 of the classic IEEE 1394
-	  stack.
-
-	  NOTE:
-	  If you want to install firewire-ohci and ohci1394 together, you
-	  should configure them only as modules and blacklist the driver(s)
-	  which you don't want to have auto-loaded.  Add either
-
-	      blacklist firewire-ohci
-	  or
-	      blacklist ohci1394
-	      blacklist video1394
-	      blacklist dv1394
-
-	  to /etc/modprobe.conf or /etc/modprobe.d/* and update modprobe.conf
-	  depending on your distribution.
+	  called firewire-ohci.
 
 config FIREWIRE_OHCI_DEBUG
 	bool
@@ -66,8 +46,7 @@ config FIREWIRE_SBP2
 	  like scanners.
 
 	  To compile this driver as a module, say M here:  The module will be
-	  called firewire-sbp2.  It replaces sbp2 of the classic IEEE 1394
-	  stack.
+	  called firewire-sbp2.
 
 	  You should also enable support for disks, CD-ROMs, etc. in the SCSI
 	  configuration section.
@@ -83,5 +62,8 @@ config FIREWIRE_NET
 	  NOTE, this driver is not stable yet!
 
 	  To compile this driver as a module, say M here:  The module will be
-	  called firewire-net.  It replaces eth1394 of the classic IEEE 1394
-	  stack.
+	  called firewire-net.
+
+source "drivers/ieee1394/Kconfig"
+
+endmenu
Index: linux-2.6.33-rc2/drivers/ieee1394/Kconfig
===================================================================
--- linux-2.6.33-rc2.orig/drivers/ieee1394/Kconfig
+++ linux-2.6.33-rc2/drivers/ieee1394/Kconfig
@@ -1,8 +1,3 @@
-menu "IEEE 1394 (FireWire) support"
-	depends on PCI || BROKEN
-
-source "drivers/firewire/Kconfig"
-
 config IEEE1394
 	tristate "Legacy alternative FireWire driver stack"
 	depends on PCI || BROKEN
@@ -16,8 +11,13 @@ config IEEE1394
 	  is the core support only, you will also need to select a driver for
 	  your IEEE 1394 adapter.
 
-	  To compile this driver as a module, say M here: the
-	  module will be called ieee1394.
+	  To compile this driver as a module, say M here: the module will be
+	  called ieee1394.
+
+	  NOTE:
+	  ieee1394 is superseded by the newer firewire-core driver.  See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
 
 config IEEE1394_OHCI1394
 	tristate "OHCI-1394 controllers"
@@ -29,19 +29,23 @@ config IEEE1394_OHCI1394
 	  use one of these chipsets.  It should work with any OHCI-1394
 	  compliant card, however.
 
-	  To compile this driver as a module, say M here: the
-	  module will be called ohci1394.
+	  To compile this driver as a module, say M here: the module will be
+	  called ohci1394.
 
 	  NOTE:
+	  ohci1394 is superseded by the newer firewire-ohci driver.  See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
+
 	  If you want to install firewire-ohci and ohci1394 together, you
 	  should configure them only as modules and blacklist the driver(s)
 	  which you don't want to have auto-loaded.  Add either
 
-	      blacklist firewire-ohci
-	  or
 	      blacklist ohci1394
 	      blacklist video1394
 	      blacklist dv1394
+	  or
+	      blacklist firewire-ohci
 
 	  to /etc/modprobe.conf or /etc/modprobe.d/* and update modprobe.conf
 	  depending on your distribution.
@@ -58,8 +62,8 @@ config IEEE1394_PCILYNX
 	  Instruments PCILynx chip.  Note: this driver is written for revision
 	  2 of this chip and may not work with revision 0.
 
-	  To compile this driver as a module, say M here: the
-	  module will be called pcilynx.
+	  To compile this driver as a module, say M here: the module will be
+	  called pcilynx.
 
 	  Only some old and now very rare PCI and CardBus cards and
 	  PowerMacs G3 B&W contain the PCILynx controller.  Therefore
@@ -79,6 +83,14 @@ config IEEE1394_SBP2
 	  You should also enable support for disks, CD-ROMs, etc. in the SCSI
 	  configuration section.
 
+	  To compile this driver as a module, say M here: the module will be
+	  called sbp2.
+
+	  NOTE:
+	  sbp2 is superseded by the newer firewire-sbp2 driver. See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
+
 config IEEE1394_SBP2_PHYS_DMA
 	bool "Enable replacement for physical DMA in SBP2"
 	depends on IEEE1394_SBP2 && VIRT_TO_BUS && EXPERIMENTAL
@@ -111,6 +123,11 @@ config IEEE1394_ETH1394
 
 	  The module is called eth1394 although it does not emulate Ethernet.
 
+	  NOTE:
+	  eth1394 is superseded by the newer firewire-net driver. See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
+
 config IEEE1394_RAWIO
 	tristate "raw1394 userspace interface"
 	depends on IEEE1394
@@ -123,6 +140,11 @@ config IEEE1394_RAWIO
 	  To compile this driver as a module, say M here: the module will be
 	  called raw1394.
 
+	  NOTE:
+	  raw1394 is superseded by the newer firewire-core driver. See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
+
 config IEEE1394_VIDEO1394
 	tristate "video1394 userspace interface"
 	depends on IEEE1394 && IEEE1394_OHCI1394
@@ -136,13 +158,18 @@ config IEEE1394_VIDEO1394
 	  To compile this driver as a module, say M here: the module will be
 	  called video1394.
 
+	  NOTE:
+	  video1394 is superseded by the newer firewire-core driver. See
+	  http://ieee1394.wiki.kernel.org/index.php/Juju_Migration for
+	  further information on how to switch to the new FireWire drivers.
+
 config IEEE1394_DV1394
 	tristate "dv1394 userspace interface (deprecated)"
 	depends on IEEE1394 && IEEE1394_OHCI1394
 	help
 	  The dv1394 driver is unsupported and may be removed from Linux in a
-	  future release.  Its functionality is now provided by raw1394 together
-	  with libraries such as libiec61883.
+	  future release.  Its functionality is now provided by either
+	  raw1394 or firewire-core together with libraries such as libiec61883.
 
 config IEEE1394_VERBOSEDEBUG
 	bool "Excessive debugging output"
@@ -153,5 +180,3 @@ config IEEE1394_VERBOSEDEBUG
 	  will quickly result in large amounts of data sent to the system log.
 
 	  Say Y if you really need the debugging output.  Everyone else says N.
-
-endmenu

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