Commit e205597d188a9ea69ce43f740a14f07b3f5b996a

Authored by Stefan Richter
1 parent c82f91f266

firewire: cdev: fix ABI for FCP and address range mapping, add fw_cdev_event_request2

The problem:

A target-like userspace driver, e.g. AV/C target or SBP-2/3 target,
needs to be able to act as responder and requester.  In the latter role,
it needs to send requests to nods from which it received requests.  This
is currently impossible because fw_cdev_event_request lacks information
about sender node ID.
Reported-by: Jay Fenlason <fenlason@redhat.com>

Libffado + libraw1394 + firewire-core is currently unable to drive two
or more audio devices on the same bus.
Reported-by: Arnold Krille <arnold@arnoldarts.de>

This is because libffado requires destination node ID of FCP requests
and sender node ID of FCP responses to match.  It even prohibits
libffado from working with a bus on which libraw1394 opens a /dev/fw* as
default ioctl device that does not correspond with the audio device.
This is because libraw1394 does not receive the sender node ID from the
kernel.

Moreover, fw_cdev_event_request makes it impossible to tell unicast and
broadcast write requests apart.

The fix:

Add a replacement of struct fw_cdev_event_request request, boringly
called struct fw_cdev_event_request2.  The new event will be sent to a
userspace client instead of the old one if the client claims
compatibility with <linux/firewire-cdev.h> ABI version 4 or later.

libraw1394 needs to be extended to make use of the new event, in order
to properly support libffado and other FCP or address range mapping
users who require correct sender node IDs.

Further notes:

While we are at it, change back the range of possible values of
fw_cdev_event_request.tcode to 0x0...0xb like in ABI version <= 3.
The preceding change "firewire: expose extended tcode of incoming lock
requests to (userspace) drivers" expanded it to 0x0...0x17 which could
catch sloppily coded clients by surprise.  The extended range of codes
is only used in the new fw_cdev_event_request2.tcode.

Jay and I also suggested an alternative approach to fix the ABI for
incoming requests:  Add an FW_CDEV_IOC_GET_REQUEST_INFO ioctl which can
be called after reception of an fw_cdev_event_request, before issuing of
the closing FW_CDEV_IOC_SEND_RESPONSE ioctl.  The new ioctl would reveal
the vital information about a request that fw_cdev_event_request lacks.
Jay showed an implementation of this approach.

The former event approach adds 27 LOC of rather trivial code to
core-cdev.c, the ioctl approach 34 LOC, some of which is nontrivial.
The ioctl approach would certainly also add more LOC to userspace
programs which require the expanded information on inbound requests.
This approach is probably only on the lighter-weight side in case of
clients that want to be compatible with kernels that lack the new
capability, like libraw1394.  However, the code to be added to such
libraw1394-like clients in case of the event approach is a straight-
forward additional switch () case in its event handler.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Showing 2 changed files with 110 additions and 11 deletions Side-by-side Diff

drivers/firewire/core-cdev.c
... ... @@ -49,7 +49,8 @@
49 49 /*
50 50 * ABI version history is documented in linux/firewire-cdev.h.
51 51 */
52   -#define FW_CDEV_KERNEL_VERSION 3
  52 +#define FW_CDEV_KERNEL_VERSION 4
  53 +#define FW_CDEV_VERSION_EVENT_REQUEST2 4
53 54  
54 55 struct client {
55 56 u32 version;
... ... @@ -176,7 +177,10 @@
176 177  
177 178 struct inbound_transaction_event {
178 179 struct event event;
179   - struct fw_cdev_event_request request;
  180 + union {
  181 + struct fw_cdev_event_request request;
  182 + struct fw_cdev_event_request2 request2;
  183 + } req;
180 184 };
181 185  
182 186 struct iso_interrupt_event {
... ... @@ -645,6 +649,7 @@
645 649 struct address_handler_resource *handler = callback_data;
646 650 struct inbound_transaction_resource *r;
647 651 struct inbound_transaction_event *e;
  652 + size_t event_size0;
648 653 void *fcp_frame = NULL;
649 654 int ret;
650 655  
651 656  
652 657  
... ... @@ -678,15 +683,37 @@
678 683 if (ret < 0)
679 684 goto failed;
680 685  
681   - e->request.type = FW_CDEV_EVENT_REQUEST;
682   - e->request.tcode = tcode;
683   - e->request.offset = offset;
684   - e->request.length = length;
685   - e->request.handle = r->resource.handle;
686   - e->request.closure = handler->closure;
  686 + if (handler->client->version < FW_CDEV_VERSION_EVENT_REQUEST2) {
  687 + struct fw_cdev_event_request *req = &e->req.request;
687 688  
  689 + if (tcode & 0x10)
  690 + tcode = TCODE_LOCK_REQUEST;
  691 +
  692 + req->type = FW_CDEV_EVENT_REQUEST;
  693 + req->tcode = tcode;
  694 + req->offset = offset;
  695 + req->length = length;
  696 + req->handle = r->resource.handle;
  697 + req->closure = handler->closure;
  698 + event_size0 = sizeof(*req);
  699 + } else {
  700 + struct fw_cdev_event_request2 *req = &e->req.request2;
  701 +
  702 + req->type = FW_CDEV_EVENT_REQUEST2;
  703 + req->tcode = tcode;
  704 + req->offset = offset;
  705 + req->source_node_id = source;
  706 + req->destination_node_id = destination;
  707 + req->card = card->index;
  708 + req->generation = generation;
  709 + req->length = length;
  710 + req->handle = r->resource.handle;
  711 + req->closure = handler->closure;
  712 + event_size0 = sizeof(*req);
  713 + }
  714 +
688 715 queue_event(handler->client, &e->event,
689   - &e->request, sizeof(e->request), r->data, length);
  716 + &e->req, event_size0, r->data, length);
690 717 return;
691 718  
692 719 failed:
include/linux/firewire-cdev.h
... ... @@ -32,6 +32,9 @@
32 32 #define FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED 0x04
33 33 #define FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED 0x05
34 34  
  35 +/* available since kernel version 2.6.36 */
  36 +#define FW_CDEV_EVENT_REQUEST2 0x06
  37 +
35 38 /**
36 39 * struct fw_cdev_event_common - Common part of all fw_cdev_event_ types
37 40 * @closure: For arbitrary use by userspace
38 41  
39 42  
... ... @@ -98,11 +101,46 @@
98 101 };
99 102  
100 103 /**
101   - * struct fw_cdev_event_request - Sent on incoming request to an address region
  104 + * struct fw_cdev_event_request - Old version of &fw_cdev_event_request2
102 105 * @closure: See &fw_cdev_event_common; set by %FW_CDEV_IOC_ALLOCATE ioctl
103 106 * @type: See &fw_cdev_event_common; always %FW_CDEV_EVENT_REQUEST
  107 + * @tcode: See &fw_cdev_event_request2
  108 + * @offset: See &fw_cdev_event_request2
  109 + * @handle: See &fw_cdev_event_request2
  110 + * @length: See &fw_cdev_event_request2
  111 + * @data: See &fw_cdev_event_request2
  112 + *
  113 + * This event is sent instead of &fw_cdev_event_request2 if the kernel or
  114 + * the client implements ABI version <= 3.
  115 + *
  116 + * Unlike &fw_cdev_event_request2, the sender identity cannot be established,
  117 + * broadcast write requests cannot be distinguished from unicast writes, and
  118 + * @tcode of lock requests is %TCODE_LOCK_REQUEST.
  119 + *
  120 + * Requests to the FCP_REQUEST or FCP_RESPONSE register are responded to as
  121 + * with &fw_cdev_event_request2, except in kernel 2.6.32 and older which send
  122 + * the response packet of the client's %FW_CDEV_IOC_SEND_RESPONSE ioctl.
  123 + */
  124 +struct fw_cdev_event_request {
  125 + __u64 closure;
  126 + __u32 type;
  127 + __u32 tcode;
  128 + __u64 offset;
  129 + __u32 handle;
  130 + __u32 length;
  131 + __u32 data[0];
  132 +};
  133 +
  134 +/**
  135 + * struct fw_cdev_event_request2 - Sent on incoming request to an address region
  136 + * @closure: See &fw_cdev_event_common; set by %FW_CDEV_IOC_ALLOCATE ioctl
  137 + * @type: See &fw_cdev_event_common; always %FW_CDEV_EVENT_REQUEST2
104 138 * @tcode: Transaction code of the incoming request
105 139 * @offset: The offset into the 48-bit per-node address space
  140 + * @source_node_id: Sender node ID
  141 + * @destination_node_id: Destination node ID
  142 + * @card: The index of the card from which the request came
  143 + * @generation: Bus generation in which the request is valid
106 144 * @handle: Reference to the kernel-side pending request
107 145 * @length: Data length, i.e. the request's payload size in bytes
108 146 * @data: Incoming data, if any
109 147  
110 148  
... ... @@ -115,12 +153,42 @@
115 153 *
116 154 * The payload data for requests carrying data (write and lock requests)
117 155 * follows immediately and can be accessed through the @data field.
  156 + *
  157 + * Unlike &fw_cdev_event_request, @tcode of lock requests is one of the
  158 + * firewire-core specific %TCODE_LOCK_MASK_SWAP...%TCODE_LOCK_VENDOR_DEPENDENT,
  159 + * i.e. encodes the extended transaction code.
  160 + *
  161 + * @card may differ from &fw_cdev_get_info.card because requests are received
  162 + * from all cards of the Linux host. @source_node_id, @destination_node_id, and
  163 + * @generation pertain to that card. Destination node ID and bus generation may
  164 + * therefore differ from the corresponding fields of the last
  165 + * &fw_cdev_event_bus_reset.
  166 + *
  167 + * @destination_node_id may also differ from the current node ID because of a
  168 + * non-local bus ID part or in case of a broadcast write request. Note, a
  169 + * client must call an %FW_CDEV_IOC_SEND_RESPONSE ioctl even in case of a
  170 + * broadcast write request; the kernel will then release the kernel-side pending
  171 + * request but will not actually send a response packet.
  172 + *
  173 + * In case of a write request to FCP_REQUEST or FCP_RESPONSE, the kernel already
  174 + * sent a write response immediately after the request was received; in this
  175 + * case the client must still call an %FW_CDEV_IOC_SEND_RESPONSE ioctl to
  176 + * release the kernel-side pending request, though another response won't be
  177 + * sent.
  178 + *
  179 + * If the client subsequently needs to initiate requests to the sender node of
  180 + * an &fw_cdev_event_request2, it needs to use a device file with matching
  181 + * card index, node ID, and generation for outbound requests.
118 182 */
119   -struct fw_cdev_event_request {
  183 +struct fw_cdev_event_request2 {
120 184 __u64 closure;
121 185 __u32 type;
122 186 __u32 tcode;
123 187 __u64 offset;
  188 + __u32 source_node_id;
  189 + __u32 destination_node_id;
  190 + __u32 card;
  191 + __u32 generation;
124 192 __u32 handle;
125 193 __u32 length;
126 194 __u32 data[0];
... ... @@ -200,6 +268,7 @@
200 268 * @bus_reset: Valid if @common.type == %FW_CDEV_EVENT_BUS_RESET
201 269 * @response: Valid if @common.type == %FW_CDEV_EVENT_RESPONSE
202 270 * @request: Valid if @common.type == %FW_CDEV_EVENT_REQUEST
  271 + * @request2: Valid if @common.type == %FW_CDEV_EVENT_REQUEST2
203 272 * @iso_interrupt: Valid if @common.type == %FW_CDEV_EVENT_ISO_INTERRUPT
204 273 * @iso_resource: Valid if @common.type ==
205 274 * %FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED or
... ... @@ -218,6 +287,7 @@
218 287 struct fw_cdev_event_bus_reset bus_reset;
219 288 struct fw_cdev_event_response response;
220 289 struct fw_cdev_event_request request;
  290 + struct fw_cdev_event_request2 request2; /* added in 2.6.36 */
221 291 struct fw_cdev_event_iso_interrupt iso_interrupt;
222 292 struct fw_cdev_event_iso_resource iso_resource; /* added in 2.6.30 */
223 293 };
224 294  
... ... @@ -263,8 +333,10 @@
263 333 * (2.6.32) - added time stamp to xmit &fw_cdev_event_iso_interrupt
264 334 * (2.6.33) - IR has always packet-per-buffer semantics now, not one of
265 335 * dual-buffer or packet-per-buffer depending on hardware
  336 + * - shared use and auto-response for FCP registers
266 337 * 3 (2.6.34) - made &fw_cdev_get_cycle_timer reliable
267 338 * - added %FW_CDEV_IOC_GET_CYCLE_TIMER2
  339 + * 4 (2.6.36) - added %FW_CDEV_EVENT_REQUEST2
268 340 */
269 341 #define FW_CDEV_VERSION 3 /* Meaningless; don't use this macro. */
270 342