From 792a61021c6043f6c2b24b1cdd42be5753b3e54c Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 12 Jul 2008 14:49:19 +0200 Subject: firewire: fix race of bus reset with request transmission Reported by Jay Fenlason: A bus reset tasklet may call fw_flush_transactions and touch transactions (call their callback which will free them) while the context which submitted the transaction is still inserting it into the transmission queue. A simple solution to this problem is to _not_ "flush" the transactions because of a bus reset (complete the transcations as 'cancelled'). They will now simply time out (completed as 'cancelled' by the split-timeout timer). Jay Fenlason thought of this fix too but I was quicker to type it out. :-) Background: Contexts which access an instance of struct fw_transaction are: 1. the submitter, until it inserted the packet which is embedded in the transaction into the AT req DMA, 2. the AsReqTrContext tasklet when the request packet was acked by the responder node or transmission to the responder failed, 3. the AsRspRcvContext tasklet when it found a request which matched an incoming response, 4. the card->flush_timer when it picks up timed-out transactions to cancel them, 5. the bus reset tasklet when it cancels transactions (this access is eliminated by this patch), 6. a process which shuts down an fw_card (unregisters it from fw-core when the controller is unbound from fw-ohci) --- although in this case there shouldn't really be any transactions anymore because we wait until all card users finished their business with the card. All of these contexts run concurrently (except for the 6th, presumably). The 1st is safe against the 2nd and 3rd because of the way how a request packet is carefully submitted to the hardware. A race between 2nd and 3rd has been fixed a while ago (bug 9617). The 4th is almost safe against 1st, 2nd, 3rd; there are issues with it if huge scheduling latencies occur, to be fixed separately. The 5th looks safe against 2nd, 3rd, and 4th but is unsafe against 1st. Maybe this could be fixed with an explicit state variable in struct fw_transaction. But this would require fw_transaction to be rewritten as only dynamically allocatable object with reference counting --- not a good solution if we also can simply kill this 5th accessing context (replace it by the 4th). Signed-off-by: Stefan Richter --- drivers/firewire/fw-topology.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/firewire/fw-topology.c b/drivers/firewire/fw-topology.c index 213b0ff8f3d..c1b81077c4a 100644 --- a/drivers/firewire/fw-topology.c +++ b/drivers/firewire/fw-topology.c @@ -510,8 +510,6 @@ fw_core_handle_bus_reset(struct fw_card *card, struct fw_node *local_node; unsigned long flags; - fw_flush_transactions(card); - spin_lock_irqsave(&card->lock, flags); /* -- cgit v1.2.3 From e9aeb46c93a8b1b703d00586c05d9a71aa7e0f0c Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 12 Jul 2008 14:50:06 +0200 Subject: firewire: fully initialize fw_transaction before marking it pending In theory, card->flush_timer could already access a transaction between fw_send_request()'s spin_unlock_irqrestore and the rest of what happens in fw_send_request(). This would happen if the process which sends the request is preempted and put to sleep right after spin_unlock_irqrestore for longer than 100ms. Therefore we fill in everything in struct fw_transaction at which the flush_timer might look at before we lift the lock. To do: Ensure that the timer does not pick up the transaction before the time of the AT request event plus split transaction timeout. Signed-off-by: Stefan Richter --- drivers/firewire/fw-transaction.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/firewire/fw-transaction.c b/drivers/firewire/fw-transaction.c index 40db8075227..7addfb3b070 100644 --- a/drivers/firewire/fw-transaction.c +++ b/drivers/firewire/fw-transaction.c @@ -279,11 +279,6 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, card->current_tlabel = (card->current_tlabel + 1) & 0x1f; card->tlabel_mask |= (1 << tlabel); - list_add_tail(&t->link, &card->transaction_list); - - spin_unlock_irqrestore(&card->lock, flags); - - /* Initialize rest of transaction, fill out packet and send it. */ t->node_id = node_id; t->tlabel = tlabel; t->callback = callback; @@ -294,6 +289,10 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, speed, offset, payload, length); t->packet.callback = transmit_complete_callback; + list_add_tail(&t->link, &card->transaction_list); + + spin_unlock_irqrestore(&card->lock, flags); + card->driver->send_request(card, &t->packet); } EXPORT_SYMBOL(fw_send_request); -- cgit v1.2.3 From b9549bc6803d6a16fe6a85b316b742ef82bd3931 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 12 Jul 2008 14:50:42 +0200 Subject: firewire: small fw_fill_request cleanup - better name for a function argument - removal of a local variable which became unnecessary after "fully initialize fw_transaction before marking it pending" Signed-off-by: Stefan Richter --- drivers/firewire/fw-transaction.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/firewire/fw-transaction.c b/drivers/firewire/fw-transaction.c index 7addfb3b070..861dd60de7d 100644 --- a/drivers/firewire/fw-transaction.c +++ b/drivers/firewire/fw-transaction.c @@ -151,7 +151,7 @@ transmit_complete_callback(struct fw_packet *packet, static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, - int node_id, int source_id, int generation, int speed, + int destination_id, int source_id, int generation, int speed, unsigned long long offset, void *payload, size_t length) { int ext_tcode; @@ -166,7 +166,7 @@ fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, HEADER_RETRY(RETRY_X) | HEADER_TLABEL(tlabel) | HEADER_TCODE(tcode) | - HEADER_DESTINATION(node_id); + HEADER_DESTINATION(destination_id); packet->header[1] = HEADER_OFFSET_HIGH(offset >> 32) | HEADER_SOURCE(source_id); packet->header[2] = @@ -252,7 +252,7 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, fw_transaction_callback_t callback, void *callback_data) { unsigned long flags; - int tlabel, source; + int tlabel; /* * Bump the flush timer up 100ms first of all so we @@ -268,7 +268,6 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, spin_lock_irqsave(&card->lock, flags); - source = card->node_id; tlabel = card->current_tlabel; if (card->tlabel_mask & (1 << tlabel)) { spin_unlock_irqrestore(&card->lock, flags); @@ -284,9 +283,8 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, t->callback = callback; t->callback_data = callback_data; - fw_fill_request(&t->packet, tcode, t->tlabel, - node_id, source, generation, - speed, offset, payload, length); + fw_fill_request(&t->packet, tcode, t->tlabel, node_id, card->node_id, + generation, speed, offset, payload, length); t->packet.callback = transmit_complete_callback; list_add_tail(&t->link, &card->transaction_list); -- cgit v1.2.3 From 1e8afea124added6409d5209f90d9949f5a13b32 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 12 Jul 2008 14:51:18 +0200 Subject: firewire: warn on unfinished transactions during card removal After card->done and card->work are completed, any remaining pending request would be a bug. We cannot safely complete a transaction at that point anymore. IOW card users must not drop their last fw_card reference (usually indirect references through fw_device references) before their last outbound transaction through that card was finished. Signed-off-by: Stefan Richter --- drivers/firewire/fw-card.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c index da873d795aa..bbd73a406e5 100644 --- a/drivers/firewire/fw-card.c +++ b/drivers/firewire/fw-card.c @@ -539,7 +539,7 @@ fw_core_remove_card(struct fw_card *card) wait_for_completion(&card->done); cancel_delayed_work_sync(&card->work); - fw_flush_transactions(card); + WARN_ON(!list_empty(&card->transaction_list)); del_timer_sync(&card->flush_timer); } EXPORT_SYMBOL(fw_core_remove_card); -- cgit v1.2.3 From f9543d0ab6392a9a5bff0034622688dc10d9d225 Mon Sep 17 00:00:00 2001 From: JiSheng Zhang Date: Sat, 19 Jul 2008 15:35:41 +0800 Subject: firewire: queue the right number of data There will be 4 padding bytes in struct fw_cdev_event_response on some platforms The member:__u32 data will point to these padding bytes. While queue the response and data in complete_transaction in fw-cdev.c, it will queue like this: |response(excluding padding bytes)|4 padding bytes|4 padding bytes|data. It queue 4 extra bytes. That is to say it use "&response + sizeof(response)" while other place of kernel and userspace library use "&response + offsetof (typeof(response), data)". So it will lost the last 4 bytes of data. This patch can fix it while not changing the struct definition. Signed-off-by: JiSheng Zhang This fixes responses to outbound block read requests on 64bit architectures. Tested on i686, x86-64, and x86-64 with i686 userland, using firecontrol and gscanbus. Signed-off-by: Stefan Richter --- drivers/firewire/fw-cdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/fw-cdev.c b/drivers/firewire/fw-cdev.c index c639915fc3c..bc81d6fcd2f 100644 --- a/drivers/firewire/fw-cdev.c +++ b/drivers/firewire/fw-cdev.c @@ -382,9 +382,9 @@ complete_transaction(struct fw_card *card, int rcode, response->response.type = FW_CDEV_EVENT_RESPONSE; response->response.rcode = rcode; - queue_event(client, &response->event, - &response->response, sizeof(response->response), - response->response.data, response->response.length); + queue_event(client, &response->event, &response->response, + sizeof(response->response) + response->response.length, + NULL, 0); } static int ioctl_send_request(struct client *client, void *buffer) -- cgit v1.2.3 From 95984f62c9b0bf6d89ef4f514b1afe73623481de Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Tue, 22 Jul 2008 18:41:10 +0200 Subject: firewire: fw-ohci: TSB43AB22/A dualbuffer workaround Isochronous reception in dualbuffer mode is reportedly broken with TI TSB43AB22A on x86-64. Descriptor addresses above 2G have been determined as the trigger: https://bugzilla.redhat.com/show_bug.cgi?id=435550 Two fixes are possible: - pci_set_consistent_dma_mask(pdev, DMA_31BIT_MASK); at least when IR descriptors are allocated, or - simply don't use dualbuffer. This fix implements the latter workaround. But we keep using dualbuffer on x86-32 which won't give us highmen (and thus physical addresses outside the 31bit range) in coherent DMA memory allocations. Right now we could for example also whitelist PPC32, but DMA mapping implementation details are expected to change there. Signed-off-by: Stefan Richter Signed-off-by: Jarod Wilson --- drivers/firewire/fw-ohci.c | 37 ++++++++++++++++++++++++------------- include/linux/pci_ids.h | 1 + 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/drivers/firewire/fw-ohci.c b/drivers/firewire/fw-ohci.c index 333b12544dd..a4eff32621b 100644 --- a/drivers/firewire/fw-ohci.c +++ b/drivers/firewire/fw-ohci.c @@ -171,7 +171,6 @@ struct iso_context { struct fw_ohci { struct fw_card card; - u32 version; __iomem char *registers; dma_addr_t self_id_bus; __le32 *self_id_cpu; @@ -180,6 +179,8 @@ struct fw_ohci { int generation; int request_generation; /* for timestamping incoming requests */ u32 bus_seconds; + + bool use_dualbuffer; bool old_uninorth; bool bus_reset_packet_quirk; @@ -1885,7 +1886,7 @@ ohci_allocate_iso_context(struct fw_card *card, int type, size_t header_size) } else { mask = &ohci->ir_context_mask; list = ohci->ir_context_list; - if (ohci->version >= OHCI_VERSION_1_1) + if (ohci->use_dualbuffer) callback = handle_ir_dualbuffer_packet; else callback = handle_ir_packet_per_buffer; @@ -1949,7 +1950,7 @@ static int ohci_start_iso(struct fw_iso_context *base, } else { index = ctx - ohci->ir_context_list; control = IR_CONTEXT_ISOCH_HEADER; - if (ohci->version >= OHCI_VERSION_1_1) + if (ohci->use_dualbuffer) control |= IR_CONTEXT_DUAL_BUFFER_MODE; match = (tags << 28) | (sync << 8) | ctx->base.channel; if (cycle >= 0) { @@ -2279,7 +2280,7 @@ ohci_queue_iso(struct fw_iso_context *base, spin_lock_irqsave(&ctx->context.ohci->lock, flags); if (base->type == FW_ISO_CONTEXT_TRANSMIT) retval = ohci_queue_iso_transmit(base, packet, buffer, payload); - else if (ctx->context.ohci->version >= OHCI_VERSION_1_1) + else if (ctx->context.ohci->use_dualbuffer) retval = ohci_queue_iso_receive_dualbuffer(base, packet, buffer, payload); else @@ -2341,7 +2342,7 @@ static int __devinit pci_probe(struct pci_dev *dev, const struct pci_device_id *ent) { struct fw_ohci *ohci; - u32 bus_options, max_receive, link_speed; + u32 bus_options, max_receive, link_speed, version; u64 guid; int err; size_t size; @@ -2366,12 +2367,6 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent) pci_write_config_dword(dev, OHCI1394_PCI_HCI_Control, 0); pci_set_drvdata(dev, ohci); -#if defined(CONFIG_PPC_PMAC) && defined(CONFIG_PPC32) - ohci->old_uninorth = dev->vendor == PCI_VENDOR_ID_APPLE && - dev->device == PCI_DEVICE_ID_APPLE_UNI_N_FW; -#endif - ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI; - spin_lock_init(&ohci->lock); tasklet_init(&ohci->bus_reset_tasklet, @@ -2390,6 +2385,23 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent) goto fail_iomem; } + version = reg_read(ohci, OHCI1394_Version) & 0x00ff00ff; + ohci->use_dualbuffer = version >= OHCI_VERSION_1_1; + +/* x86-32 currently doesn't use highmem for dma_alloc_coherent */ +#if !defined(CONFIG_X86_32) + /* dual-buffer mode is broken with descriptor addresses above 2G */ + if (dev->vendor == PCI_VENDOR_ID_TI && + dev->device == PCI_DEVICE_ID_TI_TSB43AB22) + ohci->use_dualbuffer = false; +#endif + +#if defined(CONFIG_PPC_PMAC) && defined(CONFIG_PPC32) + ohci->old_uninorth = dev->vendor == PCI_VENDOR_ID_APPLE && + dev->device == PCI_DEVICE_ID_APPLE_UNI_N_FW; +#endif + ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI; + ar_context_init(&ohci->ar_request_ctx, ohci, OHCI1394_AsReqRcvContextControlSet); @@ -2441,9 +2453,8 @@ pci_probe(struct pci_dev *dev, const struct pci_device_id *ent) if (err < 0) goto fail_self_id; - ohci->version = reg_read(ohci, OHCI1394_Version) & 0x00ff00ff; fw_notify("Added fw-ohci device %s, OHCI version %x.%x\n", - dev->dev.bus_id, ohci->version >> 16, ohci->version & 0xff); + dev->dev.bus_id, version >> 16, version & 0xff); return 0; fail_self_id: diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 65953822c9c..720d6755410 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -748,6 +748,7 @@ #define PCI_VENDOR_ID_TI 0x104c #define PCI_DEVICE_ID_TI_TVP4020 0x3d07 #define PCI_DEVICE_ID_TI_4450 0x8011 +#define PCI_DEVICE_ID_TI_TSB43AB22 0x8023 #define PCI_DEVICE_ID_TI_XX21_XX11 0x8031 #define PCI_DEVICE_ID_TI_XX21_XX11_FM 0x8033 #define PCI_DEVICE_ID_TI_XX21_XX11_SD 0x8034 -- cgit v1.2.3 From c0220d686b926a5865a2032c805015758bfdda69 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Tue, 22 Jul 2008 21:35:47 +0200 Subject: firewire: avoid memleak after phy config transmit failure Use only statically allocated data for PHY config packet transmission. With the previous incarnation, some data wouldn't be freed if the packet transmit callback was never called. A theoretical drawback now is that, in PCs with more than one card, card A may complete() for a waiter on card B. But this is highly unlikely and its impact not serious. Bus manager B may reset bus B before the PHY config went out, but the next phy config on B should be fine. However, with a timeout of 100ms, this situation is close to impossible. Signed-off-by: Stefan Richter --- drivers/firewire/fw-transaction.c | 58 ++++++++++++++------------------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/drivers/firewire/fw-transaction.c b/drivers/firewire/fw-transaction.c index 861dd60de7d..e5d1a0b64fc 100644 --- a/drivers/firewire/fw-transaction.c +++ b/drivers/firewire/fw-transaction.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -295,58 +296,41 @@ fw_send_request(struct fw_card *card, struct fw_transaction *t, } EXPORT_SYMBOL(fw_send_request); -struct fw_phy_packet { - struct fw_packet packet; - struct completion done; - struct kref kref; -}; - -static void phy_packet_release(struct kref *kref) -{ - struct fw_phy_packet *p = - container_of(kref, struct fw_phy_packet, kref); - kfree(p); -} +static DEFINE_MUTEX(phy_config_mutex); +static DECLARE_COMPLETION(phy_config_done); static void transmit_phy_packet_callback(struct fw_packet *packet, struct fw_card *card, int status) { - struct fw_phy_packet *p = - container_of(packet, struct fw_phy_packet, packet); - - complete(&p->done); - kref_put(&p->kref, phy_packet_release); + complete(&phy_config_done); } +static struct fw_packet phy_config_packet = { + .header_length = 8, + .payload_length = 0, + .speed = SCODE_100, + .callback = transmit_phy_packet_callback, +}; + void fw_send_phy_config(struct fw_card *card, int node_id, int generation, int gap_count) { - struct fw_phy_packet *p; long timeout = DIV_ROUND_UP(HZ, 10); u32 data = PHY_IDENTIFIER(PHY_PACKET_CONFIG) | PHY_CONFIG_ROOT_ID(node_id) | PHY_CONFIG_GAP_COUNT(gap_count); - p = kmalloc(sizeof(*p), GFP_KERNEL); - if (p == NULL) - return; + mutex_lock(&phy_config_mutex); + + phy_config_packet.header[0] = data; + phy_config_packet.header[1] = ~data; + phy_config_packet.generation = generation; + INIT_COMPLETION(phy_config_done); + + card->driver->send_request(card, &phy_config_packet); + wait_for_completion_timeout(&phy_config_done, timeout); - p->packet.header[0] = data; - p->packet.header[1] = ~data; - p->packet.header_length = 8; - p->packet.payload_length = 0; - p->packet.speed = SCODE_100; - p->packet.generation = generation; - p->packet.callback = transmit_phy_packet_callback; - init_completion(&p->done); - kref_set(&p->kref, 2); - - card->driver->send_request(card, &p->packet); - timeout = wait_for_completion_timeout(&p->done, timeout); - kref_put(&p->kref, phy_packet_release); - - /* will leak p if the callback is never executed */ - WARN_ON(timeout == 0); + mutex_unlock(&phy_config_mutex); } void fw_flush_transactions(struct fw_card *card) -- cgit v1.2.3 From f05e21b39f7dddcebab03ff329fef5783fea58d4 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Fri, 25 Jul 2008 16:24:19 +0200 Subject: firewire: state userland requirements in Kconfig help Signed-off-by: Stefan Richter --- drivers/firewire/Kconfig | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig index 76f26710fc1..fa6d6abefd4 100644 --- a/drivers/firewire/Kconfig +++ b/drivers/firewire/Kconfig @@ -16,8 +16,13 @@ config FIREWIRE enable the new stack. To compile this driver as a module, say M here: the module will be - called firewire-core. It functionally replaces ieee1394, raw1394, - and video1394. + called firewire-core. + + This module functionally replaces ieee1394, raw1394, and video1394. + To access it from application programs, you generally need at least + libraw1394 version 2. IIDC/DCAM applications also need libdc1394 + version 2. No libraries are required to access storage devices + through the firewire-sbp2 driver. config FIREWIRE_OHCI tristate "OHCI-1394 controllers" -- cgit v1.2.3