Message-ID: <4B35CACF.2000302@joow.be>
Date:	Sat, 26 Dec 2009 09:35:27 +0100
From:	Pieter Palmers <pieterp@...w.be>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
CC:	linux-kernel@...r.kernel.org,
	Clemens Ladisch <cladisch@...tmail.net>,
	linux1394-devel@...ts.sourceforge.net
Subject: Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow
 multiple FCP listeners

Stefan Richter wrote:
> Date: Thu, 24 Dec 2009 12:05:58 +0100
> From: Clemens Ladisch <cladisch@...tmail.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@...isch.de>
> Signed-off-by: Stefan Richter <stefanr@...6.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 = {
> 

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

Powered by blists - more mailing lists