Coming up: Small fixes, reduction in stack usage, code style tweaks.
[PATCH 1/8] firewire: sbp2: provide fallback if mgt_ORB_timeout is missing
[PATCH 2/8] firewire: cdev: fix memory leak in an error path
[PATCH 3/8] firewire: cdev: reduce stack usage by ioctl_dispatch
[PATCH 4/8] firewire: normalize style of queue_work wrappers
[PATCH 5/8] firewire: cdev: normalize variable names
[PATCH 6/8] firewire: optimize config ROM creation
[PATCH 7/8] firewire: core: clarify generate_config_rom usage
[PATCH 8/8] firewire: core: optimize Topology Map creation
drivers/firewire/core-card.c | 75 +++++++++----------
drivers/firewire/core-cdev.c | 136 +++++++++++++++++++++---------------
drivers/firewire/core-topology.c | 17 ++--
drivers/firewire/core-transaction.c | 9 --
drivers/firewire/core.h | 9 +-
drivers/firewire/ohci.c | 30 +++++--
drivers/firewire/sbp2.c | 48 ++++++------
include/linux/firewire.h | 16 ----
8 files changed, 177 insertions(+), 163 deletions(-)
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
The Unit_Characteristics entry of an SBP-2 unit directory is not
mandatory as far as I can tell. If it is missing, we would probably
fail to log in into the target because firewire-sbp2 would not wait for
status after it sent the login request.
The fix moves the cleanup of tgt->mgt_orb_timeout into a place where it
is executed exactly once before login, rather than 0..n times depending
on the target's config ROM. With targets with one or more
Unit_Characteristics entries, the result is the same as before.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/sbp2.c | 39 +++++++++++++++++++--------------------
1 file changed, 19 insertions(+), 20 deletions(-)
Index: linux-2.6.31/drivers/firewire/sbp2.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/sbp2.c
+++ linux-2.6.31/drivers/firewire/sbp2.c
@@ -188,14 +188,7 @@ static struct fw_device *target_device(s
/* Impossible login_id, to detect logout attempt before successful login */
#define INVALID_LOGIN_ID 0x10000
-/*
- * Per section 7.4.8 of the SBP-2 spec, a mgt_ORB_timeout value can be
- * provided in the config rom. Most devices do provide a value, which
- * we'll use for login management orbs, but with some sane limits.
- */
-#define SBP2_MIN_LOGIN_ORB_TIMEOUT 5000U /* Timeout in ms */
-#define SBP2_MAX_LOGIN_ORB_TIMEOUT 40000U /* Timeout in ms */
-#define SBP2_ORB_TIMEOUT 2000U /* Timeout in ms */
+#define SBP2_ORB_TIMEOUT 2000U /* Timeout in ms */
#define SBP2_ORB_NULL 0x80000000
#define SBP2_RETRY_LIMIT 0xf /* 15 retries */
#define SBP2_CYCLE_LIMIT (0xc8 << 12) /* 200 125us cycles */
@@ -1034,7 +1027,6 @@ static int sbp2_scan_unit_dir(struct sbp
{
struct fw_csr_iterator ci;
int key, value;
- unsigned int timeout;
fw_csr_iterator_init(&ci, directory);
while (fw_csr_iterator_next(&ci, &key, &value)) {
@@ -1059,17 +1051,7 @@ static int sbp2_scan_unit_dir(struct sbp
case SBP2_CSR_UNIT_CHARACTERISTICS:
/* the timeout value is stored in 500ms units */
- timeout = ((unsigned int) value >> 8 & 0xff) * 500;
- timeout = max(timeout, SBP2_MIN_LOGIN_ORB_TIMEOUT);
- tgt->mgt_orb_timeout =
- min(timeout, SBP2_MAX_LOGIN_ORB_TIMEOUT);
-
- if (timeout > tgt->mgt_orb_timeout)
- fw_notify("%s: config rom contains %ds "
- "management ORB timeout, limiting "
- "to %ds\n", tgt->bus_id,
- timeout / 1000,
- tgt->mgt_orb_timeout / 1000);
+ tgt->mgt_orb_timeout = (value >> 8 & 0xff) * 500;
break;
case SBP2_CSR_LOGICAL_UNIT_NUMBER:
@@ -1087,6 +1069,22 @@ static int sbp2_scan_unit_dir(struct sbp
return 0;
}
+/*
+ * Per section 7.4.8 of the SBP-2 spec, a mgt_ORB_timeout value can be
+ * provided in the config rom. Most devices do provide a value, which
+ * we'll use for login management orbs, but with some sane limits.
+ */
+static void sbp2_clamp_management_orb_timeout(struct sbp2_target *tgt)
+{
+ unsigned int timeout = tgt->mgt_orb_timeout;
+
+ if (timeout > 40000)
+ fw_notify("%s: %ds mgt_ORB_timeout limited to 40s\n",
+ tgt->bus_id, timeout / 1000);
+
+ tgt->mgt_orb_timeout = clamp_val(timeout, 5000, 40000);
+}
+
static void sbp2_init_workarounds(struct sbp2_target *tgt, u32 model,
u32 firmware_revision)
{
@@ -1171,6 +1169,7 @@ static int sbp2_probe(struct device *dev
&firmware_revision) < 0)
goto fail_tgt_put;
+ sbp2_clamp_management_orb_timeout(tgt);
sbp2_init_workarounds(tgt, model, firmware_revision);
/*
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
drivers/firewire/core-cdev.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Index: linux-2.6.31/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.31/drivers/firewire/core-cdev.c
@@ -698,6 +698,7 @@ static int ioctl_send_response(struct cl
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 cl
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)
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
Shrink the stack-allocated ioctl argument buffer from 256 to 40 bytes.
This is the maximum of what we currently need for all ioctls.
Also, turn runtime checks of the buffer size into compile-time checks.
This involves a few more lines of code but they are all taken care of by
the compiler's dead code elimination, and this way the buffer size
should actually be a little easier to update when new ioctls are added.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/core-cdev.c | 41 ++++++++++++++++++++++++++---------
1 file changed, 30 insertions(+), 11 deletions(-)
Index: linux-2.6.31/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.31/drivers/firewire/core-cdev.c
@@ -1299,28 +1299,47 @@ static int (* const ioctl_handlers[])(st
static int dispatch_ioctl(struct client *client,
unsigned int cmd, void __user *arg)
{
- char buffer[256];
+ char buffer[40];
int ret;
+#define check_ioctl_size(x) BUILD_BUG_ON(_IOC_SIZE(x) > sizeof(buffer))
+
+ check_ioctl_size(FW_CDEV_IOC_GET_INFO);
+ check_ioctl_size(FW_CDEV_IOC_SEND_REQUEST);
+ check_ioctl_size(FW_CDEV_IOC_ALLOCATE);
+ check_ioctl_size(FW_CDEV_IOC_DEALLOCATE);
+ check_ioctl_size(FW_CDEV_IOC_SEND_RESPONSE);
+ check_ioctl_size(FW_CDEV_IOC_INITIATE_BUS_RESET);
+ check_ioctl_size(FW_CDEV_IOC_ADD_DESCRIPTOR);
+ check_ioctl_size(FW_CDEV_IOC_REMOVE_DESCRIPTOR);
+ check_ioctl_size(FW_CDEV_IOC_CREATE_ISO_CONTEXT);
+ check_ioctl_size(FW_CDEV_IOC_QUEUE_ISO);
+ check_ioctl_size(FW_CDEV_IOC_START_ISO);
+ check_ioctl_size(FW_CDEV_IOC_STOP_ISO);
+ check_ioctl_size(FW_CDEV_IOC_GET_CYCLE_TIMER);
+ check_ioctl_size(FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE);
+ check_ioctl_size(FW_CDEV_IOC_DEALLOCATE_ISO_RESOURCE);
+ check_ioctl_size(FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE_ONCE);
+ check_ioctl_size(FW_CDEV_IOC_DEALLOCATE_ISO_RESOURCE_ONCE);
+ check_ioctl_size(FW_CDEV_IOC_GET_SPEED);
+ check_ioctl_size(FW_CDEV_IOC_SEND_BROADCAST_REQUEST);
+ check_ioctl_size(FW_CDEV_IOC_SEND_STREAM_PACKET);
+
if (_IOC_TYPE(cmd) != '#' ||
_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_handlers))
return -EINVAL;
- if (_IOC_DIR(cmd) & _IOC_WRITE) {
- if (_IOC_SIZE(cmd) > sizeof(buffer) ||
- copy_from_user(buffer, arg, _IOC_SIZE(cmd)))
- return -EFAULT;
- }
+ if (_IOC_DIR(cmd) & _IOC_WRITE &&
+ copy_from_user(buffer, arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
ret = ioctl_handlers[_IOC_NR(cmd)](client, buffer);
if (ret < 0)
return ret;
- if (_IOC_DIR(cmd) & _IOC_READ) {
- if (_IOC_SIZE(cmd) > sizeof(buffer) ||
- copy_to_user(arg, buffer, _IOC_SIZE(cmd)))
- return -EFAULT;
- }
+ if (_IOC_DIR(cmd) & _IOC_READ &&
+ copy_to_user(arg, buffer, _IOC_SIZE(cmd)))
+ return -EFAULT;
return ret;
}
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
drivers/firewire/core-card.c | 5 +----
drivers/firewire/core-cdev.c | 38 +++++++++++++++++-----------------
drivers/firewire/sbp2.c | 9 ++++++--
3 files changed, 27 insertions(+), 25 deletions(-)
Index: linux-2.6.31/drivers/firewire/core-card.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-card.c
+++ linux-2.6.31/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);
}
Index: linux-2.6.31/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.31/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_de
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 cl
&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 wor
/* 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 wor
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
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 clie
} else {
r->resource.release = NULL;
r->resource.handle = -1;
- schedule_iso_resource(r);
+ schedule_iso_resource(r, 0);
}
request->handle = r->resource.handle;
Index: linux-2.6.31/drivers/firewire/sbp2.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/sbp2.c
+++ linux-2.6.31/drivers/firewire/sbp2.c
@@ -820,20 +820,25 @@ static void sbp2_release_target(struct k
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);
}
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
drivers/firewire/core-cdev.c | 46 +++++++++++++++++-----------------
1 file changed, 23 insertions(+), 23 deletions(-)
Index: linux-2.6.31/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.31/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 cl
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 wor
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 clie
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;
@@ -1413,10 +1413,10 @@ static int fw_device_op_mmap(struct file
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;
@@ -1425,7 +1425,7 @@ static int shutdown_resource(int id, voi
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);
@@ -1446,8 +1446,8 @@ static int fw_device_op_release(struct i
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);
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
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(-)
Index: linux-2.6.31/drivers/firewire/core-card.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-card.c
+++ linux-2.6.31/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 f
* 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 f
* 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 f
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 f
}
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
Index: linux-2.6.31/drivers/firewire/core.h
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core.h
+++ linux-2.6.31/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);
Index: linux-2.6.31/drivers/firewire/ohci.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/ohci.c
+++ linux-2.6.31/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 l
*/
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
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 *c
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 *c
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 *c
}
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
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;
Index: linux-2.6.31/include/linux/firewire.h
===================================================================
--- linux-2.6.31.orig/include/linux/firewire.h
+++ linux-2.6.31/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 */
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
drivers/firewire/core-card.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
Index: linux-2.6.31/drivers/firewire/core-card.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-card.c
+++ linux-2.6.31/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(struc
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);
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
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 <[email protected]>
---
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(-)
Index: linux-2.6.31/drivers/firewire/core-card.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-card.c
+++ linux-2.6.31/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 *b
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
* 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;
}
Index: linux-2.6.31/drivers/firewire/core-topology.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-topology.c
+++ linux-2.6.31/drivers/firewire/core-topology.c
@@ -28,9 +28,9 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
-#include <linux/string.h>
#include <asm/atomic.h>
+#include <asm/byteorder.h>
#include <asm/system.h>
#include "core.h"
@@ -510,13 +510,16 @@ static void update_tree(struct fw_card *
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);
}
Index: linux-2.6.31/drivers/firewire/core-transaction.c
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core-transaction.c
+++ linux-2.6.31/drivers/firewire/core-transaction.c
@@ -810,8 +810,7 @@ static void handle_topology_map(struct f
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 f
}
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);
}
Index: linux-2.6.31/drivers/firewire/core.h
===================================================================
--- linux-2.6.31.orig/drivers/firewire/core.h
+++ linux-2.6.31/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)
Index: linux-2.6.31/include/linux/firewire.h
===================================================================
--- linux-2.6.31.orig/include/linux/firewire.h
+++ linux-2.6.31/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 {
--
Stefan Richter
-=====-==--= =-=- -=---
http://arcgraph.de/sr/
I wrote:
> --- linux-2.6.31.orig/drivers/firewire/core-cdev.c
> +++ linux-2.6.31/drivers/firewire/core-cdev.c
> @@ -1299,28 +1299,47 @@ static int (* const ioctl_handlers[])(st
> static int dispatch_ioctl(struct client *client,
> unsigned int cmd, void __user *arg)
> {
> - char buffer[256];
> + char buffer[40];
> int ret;
>
> +#define check_ioctl_size(x) BUILD_BUG_ON(_IOC_SIZE(x) > sizeof(buffer))
> +
> + check_ioctl_size(FW_CDEV_IOC_GET_INFO);
> + check_ioctl_size(FW_CDEV_IOC_SEND_REQUEST);
[...]
With linux 2.6.32-rc1's version of BUILD_BUG_ON, this adds sparse
warnings (seen with sparse 0.4.2):
drivers/firewire/core-cdev.c:1308:9: error: bad integer constant expression
drivers/firewire/core-cdev.c:1309:9: error: bad integer constant expression
[...]
Back to the drawing board.
--
Stefan Richter
-=====-==--= =-=- -===-
http://arcgraph.de/sr/
Shrink the stack-allocated ioctl argument buffer from 256 to 40 bytes.
This is the maximum of what we currently need for all ioctls.
Also, turn runtime checks of the buffer size into compile-time checks.
This involves a few more lines of code but they are all taken care of by
the compiler's dead code elimination, and this way the buffer size
should actually be a little easier to update when new ioctls are added.
At first I wanted to define this as
BUILD_BUG_ON(_IOC_SIZE(x) > sizeof(buffer))
but sparse spews "error: bad integer constant expression" then.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/core-cdev.c | 36 ++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
Index: linux-2.6.32-rc1/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.32-rc1.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.32-rc1/drivers/firewire/core-cdev.c
@@ -1300,28 +1300,42 @@ static int (* const ioctl_handlers[])(st
static int dispatch_ioctl(struct client *client,
unsigned int cmd, void __user *arg)
{
- char buffer[256];
+ char buffer[40];
int ret;
+#define check_ioctl_size(x) BUILD_BUG_ON(sizeof(x) > sizeof(buffer))
+
+ check_ioctl_size(struct fw_cdev_get_info);
+ check_ioctl_size(struct fw_cdev_send_request);
+ check_ioctl_size(struct fw_cdev_allocate);
+ check_ioctl_size(struct fw_cdev_deallocate);
+ check_ioctl_size(struct fw_cdev_send_response);
+ check_ioctl_size(struct fw_cdev_initiate_bus_reset);
+ check_ioctl_size(struct fw_cdev_add_descriptor);
+ check_ioctl_size(struct fw_cdev_remove_descriptor);
+ check_ioctl_size(struct fw_cdev_create_iso_context);
+ check_ioctl_size(struct fw_cdev_queue_iso);
+ check_ioctl_size(struct fw_cdev_start_iso);
+ check_ioctl_size(struct fw_cdev_stop_iso);
+ check_ioctl_size(struct fw_cdev_get_cycle_timer);
+ check_ioctl_size(struct fw_cdev_allocate_iso_resource);
+ check_ioctl_size(struct fw_cdev_send_stream_packet);
+
if (_IOC_TYPE(cmd) != '#' ||
_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_handlers))
return -EINVAL;
- if (_IOC_DIR(cmd) & _IOC_WRITE) {
- if (_IOC_SIZE(cmd) > sizeof(buffer) ||
- copy_from_user(buffer, arg, _IOC_SIZE(cmd)))
- return -EFAULT;
- }
+ if (_IOC_DIR(cmd) & _IOC_WRITE &&
+ copy_from_user(buffer, arg, _IOC_SIZE(cmd)))
+ return -EFAULT;
ret = ioctl_handlers[_IOC_NR(cmd)](client, buffer);
if (ret < 0)
return ret;
- if (_IOC_DIR(cmd) & _IOC_READ) {
- if (_IOC_SIZE(cmd) > sizeof(buffer) ||
- copy_to_user(arg, buffer, _IOC_SIZE(cmd)))
- return -EFAULT;
- }
+ if (_IOC_DIR(cmd) & _IOC_READ &&
+ copy_to_user(arg, buffer, _IOC_SIZE(cmd)))
+ return -EFAULT;
return ret;
}
--
Stefan Richter
-=====-==--= =-=- -===-
http://arcgraph.de/sr/