From 7e44c0b56b07a5e34de9943cfb2fee72e71a9f0e Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:39:56 +0200 Subject: [PATCH 01/10] firewire: cdev: fix memory leak in an error path If copy_from_user in an FW_CDEV_IOC_SEND_RESPONSE ioctl failed, an inbound_transaction_resource instance is no longer referenced and needs to be freed. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index ced186d7e9a..0516acd55ab 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -698,6 +698,7 @@ static int ioctl_send_response(struct client *client, void *buffer) struct fw_cdev_send_response *request = buffer; struct client_resource *resource; struct inbound_transaction_resource *r; + int ret = 0; if (release_client_resource(client, request->handle, release_request, &resource) < 0) @@ -707,13 +708,17 @@ static int ioctl_send_response(struct client *client, void *buffer) resource); if (request->length < r->length) r->length = request->length; - if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) - return -EFAULT; + + 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); + out: kfree(r); - return 0; + return ret; } static int ioctl_initiate_bus_reset(struct client *client, void *buffer) From 9fb551bf72929b316abb6d96cfb2ec05e896042a Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:41:10 +0200 Subject: [PATCH 02/10] firewire: normalize style of queue_work wrappers A few stylistic changes to unify some code patterns in the subsystem: - The similar queue_delayed_work helpers fw_schedule_bm_work, schedule_iso_resource, and sbp2_queue_work now have the same call convention. - Two conditional calls of schedule_iso_resource are factored into another small helper. - An sbp2_target_get helper is added as counterpart to sbp2_target_put. Object size of firewire-core is decreased a little bit, object size of firewire-sbp2 remains unchanged. Signed-off-by: Stefan Richter --- drivers/firewire/core-card.c | 5 +---- drivers/firewire/core-cdev.c | 38 ++++++++++++++++++------------------ drivers/firewire/sbp2.c | 9 +++++++-- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index e4864e894e4..33898b63cdf 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -211,11 +211,8 @@ static const char gap_count_table[] = { void fw_schedule_bm_work(struct fw_card *card, unsigned long delay) { - int scheduled; - fw_card_get(card); - scheduled = schedule_delayed_work(&card->work, delay); - if (!scheduled) + if (!schedule_delayed_work(&card->work, delay)) fw_card_put(card); } diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 0516acd55ab..c5f63a93965 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -129,9 +129,22 @@ struct iso_resource { struct iso_resource_event *e_alloc, *e_dealloc; }; -static void schedule_iso_resource(struct iso_resource *); static void release_iso_resource(struct client *, struct client_resource *); +static void schedule_iso_resource(struct iso_resource *r, unsigned long delay) +{ + client_get(r->client); + if (!schedule_delayed_work(&r->work, delay)) + client_put(r->client); +} + +static void schedule_if_iso_resource(struct client_resource *resource) +{ + if (resource->release == release_iso_resource) + schedule_iso_resource(container_of(resource, + struct iso_resource, resource), 0); +} + /* * dequeue_event() just kfree()'s the event, so the event has to be * the first field in a struct XYZ_event. @@ -313,11 +326,8 @@ static void for_each_client(struct fw_device *device, static int schedule_reallocations(int id, void *p, void *data) { - struct client_resource *r = p; + schedule_if_iso_resource(p); - if (r->release == release_iso_resource) - schedule_iso_resource(container_of(r, - struct iso_resource, resource)); return 0; } @@ -413,9 +423,7 @@ static int add_client_resource(struct client *client, &resource->handle); if (ret >= 0) { client_get(client); - if (resource->release == release_iso_resource) - schedule_iso_resource(container_of(resource, - struct iso_resource, resource)); + schedule_if_iso_resource(resource); } spin_unlock_irqrestore(&client->lock, flags); @@ -1032,8 +1040,7 @@ static void iso_resource_work(struct work_struct *work) /* Allow 1000ms grace period for other reallocations. */ if (todo == ISO_RES_ALLOC && time_is_after_jiffies(client->device->card->reset_jiffies + HZ)) { - if (schedule_delayed_work(&r->work, DIV_ROUND_UP(HZ, 3))) - client_get(client); + schedule_iso_resource(r, DIV_ROUND_UP(HZ, 3)); skip = true; } else { /* We could be called twice within the same generation. */ @@ -1118,13 +1125,6 @@ static void iso_resource_work(struct work_struct *work) client_put(client); } -static void schedule_iso_resource(struct iso_resource *r) -{ - client_get(r->client); - if (!schedule_delayed_work(&r->work, 0)) - client_put(r->client); -} - static void release_iso_resource(struct client *client, struct client_resource *resource) { @@ -1133,7 +1133,7 @@ static void release_iso_resource(struct client *client, spin_lock_irq(&client->lock); r->todo = ISO_RES_DEALLOC; - schedule_iso_resource(r); + schedule_iso_resource(r, 0); spin_unlock_irq(&client->lock); } @@ -1179,7 +1179,7 @@ static int init_iso_resource(struct client *client, } else { r->resource.release = NULL; r->resource.handle = -1; - schedule_iso_resource(r); + schedule_iso_resource(r, 0); } request->handle = r->resource.handle; diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c index 98dbbda3ad4..d485cdd8cba 100644 --- a/drivers/firewire/sbp2.c +++ b/drivers/firewire/sbp2.c @@ -820,20 +820,25 @@ static void sbp2_release_target(struct kref *kref) fw_device_put(device); } -static struct workqueue_struct *sbp2_wq; +static void sbp2_target_get(struct sbp2_target *tgt) +{ + kref_get(&tgt->kref); +} static void sbp2_target_put(struct sbp2_target *tgt) { kref_put(&tgt->kref, sbp2_release_target); } +static struct workqueue_struct *sbp2_wq; + /* * Always get the target's kref when scheduling work on one its units. * Each workqueue job is responsible to call sbp2_target_put() upon return. */ static void sbp2_queue_work(struct sbp2_logical_unit *lu, unsigned long delay) { - kref_get(&lu->tgt->kref); + sbp2_target_get(lu->tgt); if (!queue_delayed_work(sbp2_wq, &lu->work, delay)) sbp2_target_put(lu->tgt); } From e21fcf798e246202d7b60e864f1d7302ebaaf41c Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:41:38 +0200 Subject: [PATCH 03/10] firewire: cdev: normalize variable names Unify some names: - "e" for pointers to subtypes of struct event, - "event" for struct members and pointers to struct event, - "r" for pointers to subtypes of struct client_resource, - "resource" for struct members and pointers to struct client_resource, - other names for struct members and pointers to other types. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 46 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index c5f63a93965..1accfaf96c6 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -178,7 +178,7 @@ struct iso_interrupt_event { struct iso_resource_event { struct event event; - struct fw_cdev_event_iso_resource resource; + struct fw_cdev_event_iso_resource iso_resource; }; static inline void __user *u64_to_uptr(__u64 value) @@ -435,26 +435,26 @@ static int add_client_resource(struct client *client, static int release_client_resource(struct client *client, u32 handle, client_resource_release_fn_t release, - struct client_resource **resource) + struct client_resource **return_resource) { - struct client_resource *r; + struct client_resource *resource; spin_lock_irq(&client->lock); if (client->in_shutdown) - r = NULL; + resource = NULL; else - r = idr_find(&client->resource_idr, handle); - if (r && r->release == release) + resource = idr_find(&client->resource_idr, handle); + if (resource && resource->release == release) idr_remove(&client->resource_idr, handle); spin_unlock_irq(&client->lock); - if (!(r && r->release == release)) + if (!(resource && resource->release == release)) return -EINVAL; - if (resource) - *resource = r; + if (return_resource) + *return_resource = resource; else - r->release(client, r); + resource->release(client, resource); client_put(client); @@ -1108,12 +1108,12 @@ static void iso_resource_work(struct work_struct *work) e = r->e_dealloc; r->e_dealloc = NULL; } - e->resource.handle = r->resource.handle; - e->resource.channel = channel; - e->resource.bandwidth = bandwidth; + e->iso_resource.handle = r->resource.handle; + e->iso_resource.channel = channel; + e->iso_resource.bandwidth = bandwidth; queue_event(client, &e->event, - &e->resource, sizeof(e->resource), NULL, 0); + &e->iso_resource, sizeof(e->iso_resource), NULL, 0); if (free) { cancel_delayed_work(&r->work); @@ -1166,10 +1166,10 @@ static int init_iso_resource(struct client *client, r->e_alloc = e1; r->e_dealloc = e2; - e1->resource.closure = request->closure; - e1->resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED; - e2->resource.closure = request->closure; - e2->resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; + e1->iso_resource.closure = request->closure; + e1->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED; + e2->iso_resource.closure = request->closure; + e2->iso_resource.type = FW_CDEV_EVENT_ISO_RESOURCE_DEALLOCATED; if (todo == ISO_RES_ALLOC) { r->resource.release = release_iso_resource; @@ -1394,10 +1394,10 @@ static int fw_device_op_mmap(struct file *file, struct vm_area_struct *vma) static int shutdown_resource(int id, void *p, void *data) { - struct client_resource *r = p; + struct client_resource *resource = p; struct client *client = data; - r->release(client, r); + resource->release(client, resource); client_put(client); return 0; @@ -1406,7 +1406,7 @@ static int shutdown_resource(int id, void *p, void *data) static int fw_device_op_release(struct inode *inode, struct file *file) { struct client *client = file->private_data; - struct event *e, *next_e; + struct event *event, *next_event; mutex_lock(&client->device->client_list_mutex); list_del(&client->link); @@ -1427,8 +1427,8 @@ static int fw_device_op_release(struct inode *inode, struct file *file) idr_remove_all(&client->resource_idr); idr_destroy(&client->resource_idr); - list_for_each_entry_safe(e, next_e, &client->event_list, link) - kfree(e); + list_for_each_entry_safe(event, next_event, &client->event_list, link) + kfree(event); client_put(client); From 8e85973efc87dfae8508f1a3440fd44612897458 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:41:59 +0200 Subject: [PATCH 04/10] firewire: optimize config ROM creation The config ROM image of the local node was created in CPU byte order, then a temporary big endian copy was created to compute the CRC, and finally the card driver created its own big endian copy. We now generate it in big endian byte order in the first place to avoid one byte order conversion and the temporary on-stack copy of the ROM image (1000 bytes stack usage in process context). Furthermore, two 1000 bytes memset()s are replaced by one 1000 bytes - ROM length sized memset. The trivial fw_memcpy_{from,to}_be32() helpers are now superfluous and removed. The newly added __compute_block_crc() function will be folded into fw_compute_block_crc() in a subsequent change. Signed-off-by: Stefan Richter --- drivers/firewire/core-card.c | 62 +++++++++++++++++++++--------------- drivers/firewire/core.h | 7 ++-- drivers/firewire/ohci.c | 30 ++++++++++------- include/linux/firewire.h | 14 -------- 4 files changed, 60 insertions(+), 53 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 33898b63cdf..f73e3bdfc84 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -38,6 +38,18 @@ #include "core.h" +static int __compute_block_crc(__be32 *block) +{ + int length; + u16 crc; + + length = (be32_to_cpu(block[0]) >> 16) & 0xff; + crc = crc_itu_t(0, (u8 *)&block[1], length * 4); + *block |= cpu_to_be32(crc); + + return length; +} + int fw_compute_block_crc(u32 *block) { __be32 be32_block[256]; @@ -72,11 +84,11 @@ static int descriptor_count; #define BIB_CMC ((1) << 30) #define BIB_IMC ((1) << 31) -static u32 *generate_config_rom(struct fw_card *card, size_t *config_rom_length) +static __be32 *generate_config_rom(struct fw_card *card, size_t *rom_length) { struct fw_descriptor *desc; - static u32 config_rom[256]; - int i, j, length; + static __be32 config_rom[256]; + int i, j, k, length; /* * Initialize contents of config rom buffer. On the OHCI @@ -87,40 +99,39 @@ static u32 *generate_config_rom(struct fw_card *card, size_t *config_rom_length) * the version stored in the OHCI registers. */ - memset(config_rom, 0, sizeof(config_rom)); - config_rom[0] = BIB_CRC_LENGTH(4) | BIB_INFO_LENGTH(4) | BIB_CRC(0); - config_rom[1] = 0x31333934; - - config_rom[2] = + config_rom[0] = cpu_to_be32( + BIB_CRC_LENGTH(4) | BIB_INFO_LENGTH(4) | BIB_CRC(0)); + config_rom[1] = cpu_to_be32(0x31333934); + config_rom[2] = cpu_to_be32( BIB_LINK_SPEED(card->link_speed) | BIB_GENERATION(card->config_rom_generation++ % 14 + 2) | BIB_MAX_ROM(2) | BIB_MAX_RECEIVE(card->max_receive) | - BIB_BMC | BIB_ISC | BIB_CMC | BIB_IMC; - config_rom[3] = card->guid >> 32; - config_rom[4] = card->guid; + BIB_BMC | BIB_ISC | BIB_CMC | BIB_IMC); + config_rom[3] = cpu_to_be32(card->guid >> 32); + config_rom[4] = cpu_to_be32(card->guid); /* Generate root directory. */ - i = 5; - config_rom[i++] = 0; - config_rom[i++] = 0x0c0083c0; /* node capabilities */ - j = i + descriptor_count; + config_rom[6] = cpu_to_be32(0x0c0083c0); /* node capabilities */ + i = 7; + j = 7 + descriptor_count; /* Generate root directory entries for descriptors. */ list_for_each_entry (desc, &descriptor_list, link) { if (desc->immediate > 0) - config_rom[i++] = desc->immediate; - config_rom[i] = desc->key | (j - i); + config_rom[i++] = cpu_to_be32(desc->immediate); + config_rom[i] = cpu_to_be32(desc->key | (j - i)); i++; j += desc->length; } /* Update root directory length. */ - config_rom[5] = (i - 5 - 1) << 16; + config_rom[5] = cpu_to_be32((i - 5 - 1) << 16); /* End of root directory, now copy in descriptors. */ list_for_each_entry (desc, &descriptor_list, link) { - memcpy(&config_rom[i], desc->data, desc->length * 4); + for (k = 0; k < desc->length; k++) + config_rom[i + k] = cpu_to_be32(desc->data[k]); i += desc->length; } @@ -129,9 +140,9 @@ static u32 *generate_config_rom(struct fw_card *card, size_t *config_rom_length) * the bus info block, which is always the case for this * implementation. */ for (i = 0; i < j; i += length + 1) - length = fw_compute_block_crc(config_rom + i); + length = __compute_block_crc(config_rom + i); - *config_rom_length = j; + *rom_length = j; return config_rom; } @@ -139,7 +150,7 @@ static u32 *generate_config_rom(struct fw_card *card, size_t *config_rom_length) static void update_config_roms(void) { struct fw_card *card; - u32 *config_rom; + __be32 *config_rom; size_t length; list_for_each_entry (card, &card_list, link) { @@ -432,7 +443,7 @@ EXPORT_SYMBOL(fw_card_initialize); int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid) { - u32 *config_rom; + __be32 *config_rom; size_t length; int ret; @@ -462,7 +473,8 @@ EXPORT_SYMBOL(fw_card_add); * shutdown still need to be provided by the card driver. */ -static int dummy_enable(struct fw_card *card, u32 *config_rom, size_t length) +static int dummy_enable(struct fw_card *card, + const __be32 *config_rom, size_t length) { BUG(); return -1; @@ -475,7 +487,7 @@ static int dummy_update_phy_reg(struct fw_card *card, int address, } static int dummy_set_config_rom(struct fw_card *card, - u32 *config_rom, size_t length) + const __be32 *config_rom, size_t length) { /* * We take the card out of card_list before setting the dummy diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 7ff6e758515..7adca7cb9f5 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -40,7 +40,8 @@ struct fw_card_driver { * enable the PHY or set the link_on bit and initiate a bus * reset. */ - int (*enable)(struct fw_card *card, u32 *config_rom, size_t length); + int (*enable)(struct fw_card *card, + const __be32 *config_rom, size_t length); int (*update_phy_reg)(struct fw_card *card, int address, int clear_bits, int set_bits); @@ -48,10 +49,10 @@ struct fw_card_driver { /* * Update the config rom for an enabled card. This function * should change the config rom that is presented on the bus - * an initiate a bus reset. + * and initiate a bus reset. */ int (*set_config_rom)(struct fw_card *card, - u32 *config_rom, size_t length); + const __be32 *config_rom, size_t length); void (*send_request)(struct fw_card *card, struct fw_packet *packet); void (*send_response)(struct fw_card *card, struct fw_packet *packet); diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 5d524254499..41841556479 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -205,7 +205,7 @@ struct fw_ohci { dma_addr_t config_rom_bus; __be32 *next_config_rom; dma_addr_t next_config_rom_bus; - u32 next_header; + __be32 next_header; struct ar_context ar_request_ctx; struct ar_context ar_response_ctx; @@ -1355,8 +1355,9 @@ static void bus_reset_tasklet(unsigned long data) */ reg_write(ohci, OHCI1394_BusOptions, be32_to_cpu(ohci->config_rom[2])); - ohci->config_rom[0] = cpu_to_be32(ohci->next_header); - reg_write(ohci, OHCI1394_ConfigROMhdr, ohci->next_header); + ohci->config_rom[0] = ohci->next_header; + reg_write(ohci, OHCI1394_ConfigROMhdr, + be32_to_cpu(ohci->next_header)); } #ifdef CONFIG_FIREWIRE_OHCI_REMOTE_DMA @@ -1464,7 +1465,17 @@ static int software_reset(struct fw_ohci *ohci) return -EBUSY; } -static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length) +static void copy_config_rom(__be32 *dest, const __be32 *src, size_t length) +{ + size_t size = length * 4; + + memcpy(dest, src, size); + if (size < CONFIG_ROM_SIZE) + memset(&dest[length], 0, CONFIG_ROM_SIZE - size); +} + +static int ohci_enable(struct fw_card *card, + const __be32 *config_rom, size_t length) { struct fw_ohci *ohci = fw_ohci(card); struct pci_dev *dev = to_pci_dev(card->device); @@ -1565,8 +1576,7 @@ static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length) if (ohci->next_config_rom == NULL) return -ENOMEM; - memset(ohci->next_config_rom, 0, CONFIG_ROM_SIZE); - fw_memcpy_to_be32(ohci->next_config_rom, config_rom, length * 4); + copy_config_rom(ohci->next_config_rom, config_rom, length); } else { /* * In the suspend case, config_rom is NULL, which @@ -1576,7 +1586,7 @@ static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length) ohci->next_config_rom_bus = ohci->config_rom_bus; } - ohci->next_header = be32_to_cpu(ohci->next_config_rom[0]); + ohci->next_header = ohci->next_config_rom[0]; ohci->next_config_rom[0] = 0; reg_write(ohci, OHCI1394_ConfigROMhdr, 0); reg_write(ohci, OHCI1394_BusOptions, @@ -1610,7 +1620,7 @@ static int ohci_enable(struct fw_card *card, u32 *config_rom, size_t length) } static int ohci_set_config_rom(struct fw_card *card, - u32 *config_rom, size_t length) + const __be32 *config_rom, size_t length) { struct fw_ohci *ohci; unsigned long flags; @@ -1659,9 +1669,7 @@ static int ohci_set_config_rom(struct fw_card *card, ohci->next_config_rom = next_config_rom; ohci->next_config_rom_bus = next_config_rom_bus; - memset(ohci->next_config_rom, 0, CONFIG_ROM_SIZE); - fw_memcpy_to_be32(ohci->next_config_rom, config_rom, - length * 4); + copy_config_rom(ohci->next_config_rom, config_rom, length); ohci->next_header = config_rom[0]; ohci->next_config_rom[0] = 0; diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 7e1d4dec83e..53b9217de86 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -20,20 +20,6 @@ #define fw_notify(s, args...) printk(KERN_NOTICE KBUILD_MODNAME ": " s, ## args) #define fw_error(s, args...) printk(KERN_ERR KBUILD_MODNAME ": " s, ## args) -static inline void fw_memcpy_from_be32(void *_dst, void *_src, size_t size) -{ - u32 *dst = _dst; - __be32 *src = _src; - int i; - - for (i = 0; i < size / 4; i++) - dst[i] = be32_to_cpu(src[i]); -} - -static inline void fw_memcpy_to_be32(void *_dst, void *_src, size_t size) -{ - fw_memcpy_from_be32(_dst, _src, size); -} #define CSR_REGISTER_BASE 0xfffff0000000ULL /* register offsets are relative to CSR_REGISTER_BASE */ From fe242579e9f33150868f1bb79c7e262ad7953f17 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:42:27 +0200 Subject: [PATCH 05/10] firewire: core: clarify generate_config_rom usage Move the static config ROM buffer into the scope of the two callers of generate_config_rom(). That way the ROM length can be passed over as return value rather than through a pointer argument. It also becomes more obvious that accesses to the config ROM buffer have to be serialized and how this is accomplished. And firewire-core.ko shrinks a bit as well. Signed-off-by: Stefan Richter --- drivers/firewire/core-card.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index f73e3bdfc84..f5813078999 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -69,6 +69,8 @@ static LIST_HEAD(card_list); static LIST_HEAD(descriptor_list); static int descriptor_count; +static __be32 tmp_config_rom[256]; + #define BIB_CRC(v) ((v) << 0) #define BIB_CRC_LENGTH(v) ((v) << 16) #define BIB_INFO_LENGTH(v) ((v) << 24) @@ -84,10 +86,9 @@ static int descriptor_count; #define BIB_CMC ((1) << 30) #define BIB_IMC ((1) << 31) -static __be32 *generate_config_rom(struct fw_card *card, size_t *rom_length) +static size_t generate_config_rom(struct fw_card *card, __be32 *config_rom) { struct fw_descriptor *desc; - static __be32 config_rom[256]; int i, j, k, length; /* @@ -142,20 +143,17 @@ static __be32 *generate_config_rom(struct fw_card *card, size_t *rom_length) for (i = 0; i < j; i += length + 1) length = __compute_block_crc(config_rom + i); - *rom_length = j; - - return config_rom; + return j; } static void update_config_roms(void) { struct fw_card *card; - __be32 *config_rom; size_t length; list_for_each_entry (card, &card_list, link) { - config_rom = generate_config_rom(card, &length); - card->driver->set_config_rom(card, config_rom, length); + length = generate_config_rom(card, tmp_config_rom); + card->driver->set_config_rom(card, tmp_config_rom, length); } } @@ -443,7 +441,6 @@ EXPORT_SYMBOL(fw_card_initialize); int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid) { - __be32 *config_rom; size_t length; int ret; @@ -453,8 +450,8 @@ int fw_card_add(struct fw_card *card, mutex_lock(&card_mutex); - config_rom = generate_config_rom(card, &length); - ret = card->driver->enable(card, config_rom, length); + length = generate_config_rom(card, tmp_config_rom); + ret = card->driver->enable(card, tmp_config_rom, length); if (ret == 0) list_add_tail(&card->link, &card_list); From cb7c96da3651111efbe088fa12f9bed61836ea93 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 8 Oct 2009 00:42:53 +0200 Subject: [PATCH 06/10] firewire: core: optimize Topology Map creation The Topology Map of the local node was created in CPU byte order, then a temporary big endian copy was created to compute the CRC, and when a read request to the Topology Map arrived it had to be converted to big endian byte order again. We now generate it in big endian byte order in the first place. This also rids us of 1000 bytes stack usage in tasklet context. Signed-off-by: Stefan Richter --- drivers/firewire/core-card.c | 17 ++--------------- drivers/firewire/core-topology.c | 17 ++++++++++------- drivers/firewire/core-transaction.c | 9 ++------- drivers/firewire/core.h | 2 +- include/linux/firewire.h | 2 +- 5 files changed, 16 insertions(+), 31 deletions(-) diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index f5813078999..7083bcc1b9c 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -38,7 +38,7 @@ #include "core.h" -static int __compute_block_crc(__be32 *block) +int fw_compute_block_crc(__be32 *block) { int length; u16 crc; @@ -50,19 +50,6 @@ static int __compute_block_crc(__be32 *block) return length; } -int fw_compute_block_crc(u32 *block) -{ - __be32 be32_block[256]; - int i, length; - - length = (*block >> 16) & 0xff; - for (i = 0; i < length; i++) - be32_block[i] = cpu_to_be32(block[i + 1]); - *block |= crc_itu_t(0, (u8 *) be32_block, length * 4); - - return length; -} - static DEFINE_MUTEX(card_mutex); static LIST_HEAD(card_list); @@ -141,7 +128,7 @@ static size_t generate_config_rom(struct fw_card *card, __be32 *config_rom) * the bus info block, which is always the case for this * implementation. */ for (i = 0; i < j; i += length + 1) - length = __compute_block_crc(config_rom + i); + length = fw_compute_block_crc(config_rom + i); return j; } diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index fddf2b35893..9a5f38c80b0 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -28,9 +28,9 @@ #include #include #include -#include #include +#include #include #include "core.h" @@ -510,13 +510,16 @@ static void update_tree(struct fw_card *card, struct fw_node *root) static void update_topology_map(struct fw_card *card, u32 *self_ids, int self_id_count) { - int node_count; + int node_count = (card->root_node->node_id & 0x3f) + 1; + __be32 *map = card->topology_map; + + *map++ = cpu_to_be32((self_id_count + 2) << 16); + *map++ = cpu_to_be32(be32_to_cpu(card->topology_map[1]) + 1); + *map++ = cpu_to_be32((node_count << 16) | self_id_count); + + while (self_id_count--) + *map++ = cpu_to_be32p(self_ids++); - card->topology_map[1]++; - node_count = (card->root_node->node_id & 0x3f) + 1; - card->topology_map[2] = (node_count << 16) | self_id_count; - card->topology_map[0] = (self_id_count + 2) << 16; - memcpy(&card->topology_map[3], self_ids, self_id_count * 4); fw_compute_block_crc(card->topology_map); } diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index da628c72a46..203e6428bad 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -810,8 +810,7 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request int speed, unsigned long long offset, void *payload, size_t length, void *callback_data) { - int i, start, end; - __be32 *map; + int start; if (!TCODE_IS_READ_REQUEST(tcode)) { fw_send_response(card, request, RCODE_TYPE_ERROR); @@ -824,11 +823,7 @@ static void handle_topology_map(struct fw_card *card, struct fw_request *request } start = (offset - topology_map_region.start) / 4; - end = start + length / 4; - map = payload; - - for (i = 0; i < length / 4; i++) - map[i] = cpu_to_be32(card->topology_map[start + i]); + memcpy(payload, &card->topology_map[start], length); fw_send_response(card, request, RCODE_COMPLETE); } diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 7adca7cb9f5..ed3b1a765c0 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -94,7 +94,7 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid); void fw_core_remove_card(struct fw_card *card); int fw_core_initiate_bus_reset(struct fw_card *card, int short_reset); -int fw_compute_block_crc(u32 *block); +int fw_compute_block_crc(__be32 *block); void fw_schedule_bm_work(struct fw_card *card, unsigned long delay); static inline struct fw_card *fw_card_get(struct fw_card *card) diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 53b9217de86..211a5d7d87b 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -117,7 +117,7 @@ struct fw_card { bool broadcast_channel_allocated; u32 broadcast_channel; - u32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4]; + __be32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4]; }; struct fw_attribute_group { From 5b189bf3633c3b73d4f08124a86f3e019953d412 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Wed, 14 Oct 2009 20:37:36 +0200 Subject: [PATCH 07/10] firewire: core: WARN on wrong usage of core transaction functions In the code path which creates request packets, clearly mark a switch branch which must never be reached with a WARN. In the code path which creates response packets, replace a BUG by a friendlier to debug WARN. Signed-off-by: Stefan Richter --- drivers/firewire/core-transaction.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 203e6428bad..66789c3cc56 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -218,6 +218,9 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, packet->header_length = 16; packet->payload_length = 0; break; + + default: + WARN(1, KERN_ERR "wrong tcode %d", tcode); } common: packet->speed = speed; @@ -595,8 +598,7 @@ void fw_fill_response(struct fw_packet *response, u32 *request_header, break; default: - BUG(); - return; + WARN(1, KERN_ERR "wrong tcode %d", tcode); } response->payload_bus = 0; From 19593ffdb6daa6ba691d247a2400cece12687c52 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Wed, 14 Oct 2009 20:40:10 +0200 Subject: [PATCH 08/10] firewire: ohci: 0 may be a valid DMA address I was told that there are obscure architectures with non-coherent DMA which may DMA-map to bus address 0. We shall not use 0 as a magic number of uninitialized bus address variables. The packet->payload_length > 0 test cannot be used either (except in at_context_queue_packet) because local requests are not DMA-mapped regardless of payload_length. Hence add a state flag to struct fw_packet. Signed-off-by: Stefan Richter --- drivers/firewire/core-transaction.c | 4 ++-- drivers/firewire/ohci.c | 9 +++++---- include/linux/firewire.h | 1 + 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index 66789c3cc56..842739df23e 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -226,7 +226,7 @@ static void fw_fill_request(struct fw_packet *packet, int tcode, int tlabel, packet->speed = speed; packet->generation = generation; packet->ack = 0; - packet->payload_bus = 0; + packet->payload_mapped = false; } /** @@ -601,7 +601,7 @@ void fw_fill_response(struct fw_packet *response, u32 *request_header, WARN(1, KERN_ERR "wrong tcode %d", tcode); } - response->payload_bus = 0; + response->payload_mapped = false; } EXPORT_SYMBOL(fw_fill_response); diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 41841556479..a71477541dc 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -995,7 +995,8 @@ static int at_context_queue_packet(struct context *ctx, packet->ack = RCODE_SEND_ERROR; return -1; } - packet->payload_bus = payload_bus; + packet->payload_bus = payload_bus; + packet->payload_mapped = true; d[2].req_count = cpu_to_le16(packet->payload_length); d[2].data_address = cpu_to_le32(payload_bus); @@ -1023,7 +1024,7 @@ static int at_context_queue_packet(struct context *ctx, */ if (ohci->generation != packet->generation || reg_read(ohci, OHCI1394_IntEventSet) & OHCI1394_busReset) { - if (packet->payload_length > 0) + if (packet->payload_mapped) dma_unmap_single(ohci->card.device, payload_bus, packet->payload_length, DMA_TO_DEVICE); packet->ack = RCODE_GENERATION; @@ -1059,7 +1060,7 @@ static int handle_at_packet(struct context *context, /* This packet was cancelled, just continue. */ return 1; - if (packet->payload_bus) + if (packet->payload_mapped) dma_unmap_single(ohci->card.device, packet->payload_bus, packet->payload_length, DMA_TO_DEVICE); @@ -1723,7 +1724,7 @@ static int ohci_cancel_packet(struct fw_card *card, struct fw_packet *packet) if (packet->ack != 0) goto out; - if (packet->payload_bus) + if (packet->payload_mapped) dma_unmap_single(ohci->card.device, packet->payload_bus, packet->payload_length, DMA_TO_DEVICE); diff --git a/include/linux/firewire.h b/include/linux/firewire.h index 211a5d7d87b..9416a461b69 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -267,6 +267,7 @@ struct fw_packet { void *payload; size_t payload_length; dma_addr_t payload_bus; + bool payload_mapped; u32 timestamp; /* From b2c0a2ac3e59f4764c59c23bd90b571e44256030 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Thu, 15 Oct 2009 21:16:53 +0200 Subject: [PATCH 09/10] firewire: cdev: reduce stack usage by ioctl_dispatch Replace a hardcoded buffer size by a sizeof union {}. This shrinks the stack-allocated ioctl argument buffer from 256 to 40 bytes. (This is not much, but subsequent stack usage particularly by the queue_iso ioctl handler adds up.) The new form is also easier to keep up to date than a hardcoded size if more ioctls are added. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 1accfaf96c6..e7290928a90 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1299,7 +1299,23 @@ static int (* const ioctl_handlers[])(struct client *client, void *buffer) = { static int dispatch_ioctl(struct client *client, unsigned int cmd, void __user *arg) { - char buffer[256]; + char buffer[sizeof(union { + struct fw_cdev_get_info _00; + struct fw_cdev_send_request _01; + struct fw_cdev_allocate _02; + struct fw_cdev_deallocate _03; + struct fw_cdev_send_response _04; + struct fw_cdev_initiate_bus_reset _05; + struct fw_cdev_add_descriptor _06; + struct fw_cdev_remove_descriptor _07; + struct fw_cdev_create_iso_context _08; + struct fw_cdev_queue_iso _09; + struct fw_cdev_start_iso _0a; + struct fw_cdev_stop_iso _0b; + struct fw_cdev_get_cycle_timer _0c; + struct fw_cdev_allocate_iso_resource _0d; + struct fw_cdev_send_stream_packet _13; + })]; int ret; if (_IOC_TYPE(cmd) != '#' || From af0940dac37545b1e7900b19c464fb6367d3f82f Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Fri, 13 Nov 2009 12:53:08 +0100 Subject: [PATCH 10/10] ieee1394: Use hweight32 Use hweight32 instead of counting for each bit Signed-off-by: Akinobu Mita Signed-off-by: Stefan Richter (add required include) --- drivers/ieee1394/ohci1394.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/ieee1394/ohci1394.c b/drivers/ieee1394/ohci1394.c index 65c1429e412..d0dc1db80b2 100644 --- a/drivers/ieee1394/ohci1394.c +++ b/drivers/ieee1394/ohci1394.c @@ -82,6 +82,7 @@ * */ +#include #include #include #include @@ -434,7 +435,6 @@ static void initialize_dma_trm_ctx(struct dma_trm_ctx *d) /* Count the number of available iso contexts */ static int get_nb_iso_ctx(struct ti_ohci *ohci, int reg) { - int i,ctx=0; u32 tmp; reg_write(ohci, reg, 0xffffffff); @@ -443,11 +443,7 @@ static int get_nb_iso_ctx(struct ti_ohci *ohci, int reg) DBGMSG("Iso contexts reg: %08x implemented: %08x", reg, tmp); /* Count the number of contexts */ - for (i=0; i<32; i++) { - if (tmp & 1) ctx++; - tmp >>= 1; - } - return ctx; + return hweight32(tmp); } /* Global initialization */