2009-03-10 20:06:33

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 0/5] firewire: some simplifications in firewire-core + a fix

Next up:

firewire: core: drop unused call parameters of close_transaction
firewire: core: increase bus manager grace period
firewire: core: simplify broadcast channel allocation
firewire: core: optimize propagation of BROADCAST_CHANNEL

drivers/firewire/fw-card.c | 214 +++++-------------------------
drivers/firewire/fw-device.c | 45 +++++-
drivers/firewire/fw-device.h | 5
drivers/firewire/fw-topology.c | 1
drivers/firewire/fw-transaction.c | 17 +-
drivers/firewire/fw-transaction.h | 6
6 files changed, 96 insertions(+), 192 deletions(-)
--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/


2009-03-10 20:07:30

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 1/5] firewire: core: drop unused call parameters of close_transaction

All callers inserted NULL and 0 here.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-transaction.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)

Index: linux/drivers/firewire/fw-transaction.c
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.c
+++ linux/drivers/firewire/fw-transaction.c
@@ -65,8 +65,7 @@
#define PHY_IDENTIFIER(id) ((id) << 30)

static int close_transaction(struct fw_transaction *transaction,
- struct fw_card *card, int rcode,
- u32 *payload, size_t length)
+ struct fw_card *card, int rcode)
{
struct fw_transaction *t;
unsigned long flags;
@@ -82,7 +81,7 @@ static int close_transaction(struct fw_t
spin_unlock_irqrestore(&card->lock, flags);

if (&t->link != &card->transaction_list) {
- t->callback(card, rcode, payload, length, t->callback_data);
+ t->callback(card, rcode, NULL, 0, t->callback_data);
return 0;
}

@@ -110,7 +109,7 @@ int fw_cancel_transaction(struct fw_card
* if the transaction is still pending and remove it in that case.
*/

- return close_transaction(transaction, card, RCODE_CANCELLED, NULL, 0);
+ return close_transaction(transaction, card, RCODE_CANCELLED);
}
EXPORT_SYMBOL(fw_cancel_transaction);

@@ -122,7 +121,7 @@ static void transmit_complete_callback(s

switch (status) {
case ACK_COMPLETE:
- close_transaction(t, card, RCODE_COMPLETE, NULL, 0);
+ close_transaction(t, card, RCODE_COMPLETE);
break;
case ACK_PENDING:
t->timestamp = packet->timestamp;
@@ -130,20 +129,20 @@ static void transmit_complete_callback(s
case ACK_BUSY_X:
case ACK_BUSY_A:
case ACK_BUSY_B:
- close_transaction(t, card, RCODE_BUSY, NULL, 0);
+ close_transaction(t, card, RCODE_BUSY);
break;
case ACK_DATA_ERROR:
- close_transaction(t, card, RCODE_DATA_ERROR, NULL, 0);
+ close_transaction(t, card, RCODE_DATA_ERROR);
break;
case ACK_TYPE_ERROR:
- close_transaction(t, card, RCODE_TYPE_ERROR, NULL, 0);
+ close_transaction(t, card, RCODE_TYPE_ERROR);
break;
default:
/*
* In this case the ack is really a juju specific
* rcode, so just forward that to the callback.
*/
- close_transaction(t, card, status, NULL, 0);
+ close_transaction(t, card, status);
break;
}
}

--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

2009-03-10 20:08:25

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 2/5] firewire: core: increase bus manager grace period

Per IEEE 1394 clause 8.4.2.5, bus manager capable nodes which are not
incumbent shall wait at least 125ms before trying to establish
themselves as bus manager.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-card.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux/drivers/firewire/fw-card.c
===================================================================
--- linux.orig/drivers/firewire/fw-card.c
+++ linux/drivers/firewire/fw-card.c
@@ -368,9 +368,11 @@ static void fw_card_bm_work(struct work_
atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
root_device_is_cmc = root_device && root_device->cmc;
root_id = root_node->node_id;
- grace = time_after(jiffies, card->reset_jiffies + DIV_ROUND_UP(HZ, 10));
irm_device = irm_node->data;
local_device = local_node->data;
+
+ grace = time_after(jiffies, card->reset_jiffies + DIV_ROUND_UP(HZ, 8));
+
if (is_next_generation(generation, card->bm_generation) ||
(card->bm_generation != generation && grace)) {
/*
@@ -434,12 +436,11 @@ static void fw_card_bm_work(struct work_
}
} else if (card->bm_generation != generation) {
/*
- * OK, we weren't BM in the last generation, and it's
- * less than 100ms since last bus reset. Reschedule
- * this task 100ms from now.
+ * We weren't BM in the last generation, and the last
+ * bus reset is less than 125ms ago. Reschedule this job.
*/
spin_unlock_irqrestore(&card->lock, flags);
- fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 10));
+ fw_schedule_bm_work(card, DIV_ROUND_UP(HZ, 8));
goto out;
}


--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

2009-03-10 20:09:01

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 3/5] firewire: core: simplify broadcast channel allocation

fw-iso.c has channel allocation code now, use it.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-card.c | 124 +++++++++----------------------------
1 file changed, 33 insertions(+), 91 deletions(-)

Index: linux/drivers/firewire/fw-card.c
===================================================================
--- linux.orig/drivers/firewire/fw-card.c
+++ linux/drivers/firewire/fw-card.c
@@ -181,10 +181,6 @@ void fw_core_remove_descriptor(struct fw
mutex_unlock(&card_mutex);
}

-/* ------------------------------------------------------------------ */
-/* Code to handle 1394a broadcast channel */
-
-#define THIRTY_TWO_CHANNELS (0xFFFFFFFFU)
#define IRM_RETRIES 2

/*
@@ -265,62 +261,18 @@ tryagain_w:
return 0;
}

-static void
-irm_allocate_broadcast(struct fw_device *irm_dev, struct device *locald)
+static void allocate_broadcast_channel(struct fw_card *card, int generation)
{
- u32 generation;
- u32 node_id;
- u32 max_speed;
- u32 retries;
- __be32 old_data;
- __be32 lock_data[2];
- int rcode;
+ int channel, bandwidth = 0;

- /*
- * The device we are updating is the IRM, so we must do
- * some extra work.
- */
- retries = IRM_RETRIES;
- generation = irm_dev->generation;
- /* FIXME: do we need locking here? */
- smp_rmb();
- node_id = irm_dev->node_id;
- max_speed = irm_dev->max_speed;
-
- lock_data[0] = cpu_to_be32(THIRTY_TWO_CHANNELS);
- lock_data[1] = cpu_to_be32(THIRTY_TWO_CHANNELS & ~1);
-tryagain:
- old_data = lock_data[0];
- rcode = fw_run_transaction(irm_dev->card, TCODE_LOCK_COMPARE_SWAP,
- node_id, generation, max_speed,
- CSR_REGISTER_BASE+CSR_CHANNELS_AVAILABLE_HI,
- &lock_data[0], 8);
- switch (rcode) {
- case RCODE_BUSY:
- if (retries--)
- goto tryagain;
- /* fallthrough */
- default:
- fw_error("node %x: allocate broadcast channel failed (%x)\n",
- node_id, rcode);
- return;
-
- case RCODE_COMPLETE:
- if (lock_data[0] == old_data)
- break;
- if (retries--) {
- lock_data[1] = cpu_to_be32(be32_to_cpu(lock_data[0])&~1);
- goto tryagain;
- }
- fw_error("node %x: allocate broadcast channel failed: too many"
- " retries\n", node_id);
- return;
+ fw_iso_resource_manage(card, generation, 1ULL << 31,
+ &channel, &bandwidth, true);
+ if (channel == 31) {
+ card->is_irm = true;
+ device_for_each_child(card->device, NULL,
+ fw_irm_set_broadcast_channel_register);
}
- irm_dev->card->is_irm = true;
- device_for_each_child(locald, NULL, fw_irm_set_broadcast_channel_register);
}
-/* ------------------------------------------------------------------ */
-

static const char gap_count_table[] = {
63, 5, 7, 8, 10, 13, 16, 18, 21, 24, 26, 29, 32, 35, 37, 40
@@ -339,10 +291,11 @@ void fw_schedule_bm_work(struct fw_card
static void fw_card_bm_work(struct work_struct *work)
{
struct fw_card *card = container_of(work, struct fw_card, work.work);
- struct fw_device *root_device, *irm_device, *local_device;
- struct fw_node *root_node, *local_node, *irm_node;
+ struct fw_device *root_device;
+ struct fw_node *root_node;
unsigned long flags;
- int root_id, new_root_id, irm_id, gap_count, generation, grace, rcode;
+ int root_id, new_root_id, irm_id, local_id;
+ int gap_count, generation, grace, rcode;
bool do_reset = false;
bool root_device_is_running;
bool root_device_is_cmc;
@@ -350,26 +303,22 @@ static void fw_card_bm_work(struct work_

spin_lock_irqsave(&card->lock, flags);
card->is_irm = false;
- local_node = card->local_node;
- root_node = card->root_node;
- irm_node = card->irm_node;

- if (local_node == NULL) {
+ if (card->local_node == NULL) {
spin_unlock_irqrestore(&card->lock, flags);
goto out_put_card;
}
- fw_node_get(local_node);
- fw_node_get(root_node);
- fw_node_get(irm_node);

generation = card->generation;
+ root_node = card->root_node;
+ fw_node_get(root_node);
root_device = root_node->data;
root_device_is_running = root_device &&
atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
root_device_is_cmc = root_device && root_device->cmc;
- root_id = root_node->node_id;
- irm_device = irm_node->data;
- local_device = local_node->data;
+ root_id = root_node->node_id;
+ irm_id = card->irm_node->node_id;
+ local_id = card->local_node->node_id;

grace = time_after(jiffies, card->reset_jiffies + DIV_ROUND_UP(HZ, 8));

@@ -387,16 +336,15 @@ static void fw_card_bm_work(struct work_
* next generation.
*/

- irm_id = irm_node->node_id;
- if (!irm_node->link_on) {
- new_root_id = local_node->node_id;
+ if (!card->irm_node->link_on) {
+ new_root_id = local_id;
fw_notify("IRM has link off, making local node (%02x) root.\n",
new_root_id);
goto pick_me;
}

lock_data[0] = cpu_to_be32(0x3f);
- lock_data[1] = cpu_to_be32(local_node->node_id);
+ lock_data[1] = cpu_to_be32(local_id);

spin_unlock_irqrestore(&card->lock, flags);

@@ -411,12 +359,11 @@ static void fw_card_bm_work(struct work_

if (rcode == RCODE_COMPLETE &&
lock_data[0] != cpu_to_be32(0x3f)) {
- /* Somebody else is BM, let them do the work. */
- if (irm_id == local_node->node_id) {
- /* But we are IRM, so do irm-y things */
- irm_allocate_broadcast(irm_device,
- card->device);
- }
+
+ /* Somebody else is BM. Only act as IRM. */
+ if (local_id == irm_id)
+ allocate_broadcast_channel(card, generation);
+
goto out;
}

@@ -429,7 +376,7 @@ static void fw_card_bm_work(struct work_
* do a bus reset and pick the local node as
* root, and thus, IRM.
*/
- new_root_id = local_node->node_id;
+ new_root_id = local_id;
fw_notify("BM lock failed, making local node (%02x) root.\n",
new_root_id);
goto pick_me;
@@ -456,7 +403,7 @@ static void fw_card_bm_work(struct work_
* Either link_on is false, or we failed to read the
* config rom. In either case, pick another root.
*/
- new_root_id = local_node->node_id;
+ new_root_id = local_id;
} else if (!root_device_is_running) {
/*
* If we haven't probed this device yet, bail out now
@@ -478,7 +425,7 @@ static void fw_card_bm_work(struct work_
* successfully read the config rom, but it's not
* cycle master capable.
*/
- new_root_id = local_node->node_id;
+ new_root_id = local_id;
}

pick_me:
@@ -509,19 +456,14 @@ static void fw_card_bm_work(struct work_
card->index, new_root_id, gap_count);
fw_send_phy_config(card, new_root_id, generation, gap_count);
fw_core_initiate_bus_reset(card, 1);
- } else if (irm_node->node_id == local_node->node_id) {
- /*
- * We are IRM, so do irm-y things.
- * There's no reason to do this if we're doing a reset. . .
- * We'll be back.
- */
- irm_allocate_broadcast(irm_device, card->device);
+ /* Will allocate broadcast channel after the reset. */
+ } else {
+ if (local_id == irm_id)
+ allocate_broadcast_channel(card, generation);
}

out:
fw_node_put(root_node);
- fw_node_put(local_node);
- fw_node_put(irm_node);
out_put_card:
fw_card_put(card);
}

--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

2009-03-10 20:09:50

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 4/5] firewire: core: optimize propagation of BROADCAST_CHANNEL

Cache the test result of whether a device implements BROADCAST_CHANNEL.
This minimizes traffic on the bus after each bus reset. A majority of
devices does not implement BROADCAST_CHANNEL.

Remove busy retries; just rely on the hardware to retry requests to busy
responders. Remove unnecessary log messages.

Rename the flag is_irm to broadcast_channel_allocated to better reflect
its meaning. Reset the flag earlier in fw_core_handle_bus_reset.

Pass the generation down as a call parameter; that way generation can't
be newer than card->broadcast_channel_allocated and device->node_id.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-card.c | 85 +-----------------------------
drivers/firewire/fw-device.c | 45 ++++++++++++++-
drivers/firewire/fw-device.h | 5 +
drivers/firewire/fw-topology.c | 1
drivers/firewire/fw-transaction.h | 6 --
5 files changed, 52 insertions(+), 90 deletions(-)

Index: linux/drivers/firewire/fw-card.c
===================================================================
--- linux.orig/drivers/firewire/fw-card.c
+++ linux/drivers/firewire/fw-card.c
@@ -181,83 +181,9 @@ void fw_core_remove_descriptor(struct fw
mutex_unlock(&card_mutex);
}

-#define IRM_RETRIES 2
-
-/*
- * The abi is set by device_for_each_child(), even though we have no use
- * for data, nor do we have a meaningful return value.
- */
-int fw_irm_set_broadcast_channel_register(struct device *dev, void *data)
+static int set_broadcast_channel(struct device *dev, void *data)
{
- struct fw_device *d;
- int rcode;
- int node_id;
- int max_speed;
- int retries;
- int generation;
- __be32 regval;
- struct fw_card *card;
-
- d = fw_device(dev);
- /* FIXME: do we need locking here? */
- generation = d->generation;
- smp_rmb(); /* Ensure generation is at least as old as node_id */
- node_id = d->node_id;
- max_speed = d->max_speed;
- retries = IRM_RETRIES;
- card = d->card;
-tryagain_r:
- rcode = fw_run_transaction(card, TCODE_READ_QUADLET_REQUEST,
- node_id, generation, max_speed,
- CSR_REGISTER_BASE + CSR_BROADCAST_CHANNEL,
- &regval, 4);
- switch (rcode) {
- case RCODE_BUSY:
- if (retries--)
- goto tryagain_r;
- fw_notify("node %x read broadcast channel busy\n",
- node_id);
- return 0;
-
- default:
- fw_notify("node %x read broadcast channel failed %x\n",
- node_id, rcode);
- return 0;
-
- case RCODE_COMPLETE:
- /*
- * Paranoid reporting of nonstandard broadcast channel
- * contents goes here
- */
- if (regval != cpu_to_be32(BROADCAST_CHANNEL_INITIAL))
- return 0;
- break;
- }
- retries = IRM_RETRIES;
- regval = cpu_to_be32(BROADCAST_CHANNEL_INITIAL |
- BROADCAST_CHANNEL_VALID);
-tryagain_w:
- rcode = fw_run_transaction(card,
- TCODE_WRITE_QUADLET_REQUEST, node_id,
- generation, max_speed,
- CSR_REGISTER_BASE + CSR_BROADCAST_CHANNEL,
- &regval, 4);
- switch (rcode) {
- case RCODE_BUSY:
- if (retries--)
- goto tryagain_w;
- fw_notify("node %x write broadcast channel busy\n",
- node_id);
- return 0;
-
- default:
- fw_notify("node %x write broadcast channel failed %x\n",
- node_id, rcode);
- return 0;
-
- case RCODE_COMPLETE:
- return 0;
- }
+ fw_device_set_broadcast_channel(fw_device(dev), (long)data);
return 0;
}

@@ -268,9 +194,9 @@ static void allocate_broadcast_channel(s
fw_iso_resource_manage(card, generation, 1ULL << 31,
&channel, &bandwidth, true);
if (channel == 31) {
- card->is_irm = true;
- device_for_each_child(card->device, NULL,
- fw_irm_set_broadcast_channel_register);
+ card->broadcast_channel_allocated = true;
+ device_for_each_child(card->device, (void *)(long)generation,
+ set_broadcast_channel);
}
}

@@ -302,7 +228,6 @@ static void fw_card_bm_work(struct work_
__be32 lock_data[2];

spin_lock_irqsave(&card->lock, flags);
- card->is_irm = false;

if (card->local_node == NULL) {
spin_unlock_irqrestore(&card->lock, flags);
Index: linux/drivers/firewire/fw-device.c
===================================================================
--- linux.orig/drivers/firewire/fw-device.c
+++ linux/drivers/firewire/fw-device.c
@@ -518,7 +518,7 @@ static int read_bus_info_block(struct fw

kfree(old_rom);
ret = 0;
- device->cmc = rom[2] & 1 << 30;
+ device->cmc = rom[2] >> 30 & 1;
out:
kfree(rom);

@@ -756,6 +756,44 @@ static int lookup_existing_device(struct
return match;
}

+enum { BC_UNKNOWN = 0, BC_UNIMPLEMENTED, BC_IMPLEMENTED, };
+
+void fw_device_set_broadcast_channel(struct fw_device *device, int generation)
+{
+ struct fw_card *card = device->card;
+ __be32 data;
+ int rcode;
+
+ if (!card->broadcast_channel_allocated)
+ return;
+
+ if (device->bc_implemented == BC_UNKNOWN) {
+ rcode = fw_run_transaction(card, TCODE_READ_QUADLET_REQUEST,
+ device->node_id, generation, device->max_speed,
+ CSR_REGISTER_BASE + CSR_BROADCAST_CHANNEL,
+ &data, 4);
+ switch (rcode) {
+ case RCODE_COMPLETE:
+ if (data & cpu_to_be32(1 << 31)) {
+ device->bc_implemented = BC_IMPLEMENTED;
+ break;
+ }
+ /* else fall through to case address error */
+ case RCODE_ADDRESS_ERROR:
+ device->bc_implemented = BC_UNIMPLEMENTED;
+ }
+ }
+
+ if (device->bc_implemented == BC_IMPLEMENTED) {
+ data = cpu_to_be32(BROADCAST_CHANNEL_INITIAL |
+ BROADCAST_CHANNEL_VALID);
+ fw_run_transaction(card, TCODE_WRITE_QUADLET_REQUEST,
+ device->node_id, generation, device->max_speed,
+ CSR_REGISTER_BASE + CSR_BROADCAST_CHANNEL,
+ &data, 4);
+ }
+}
+
static void fw_device_init(struct work_struct *work)
{
struct fw_device *device =
@@ -849,9 +887,8 @@ static void fw_device_init(struct work_s
device->config_rom[3], device->config_rom[4],
1 << device->max_speed);
device->config_rom_retries = 0;
- if (device->card->is_irm)
- fw_irm_set_broadcast_channel_register(&device->device,
- NULL);
+
+ fw_device_set_broadcast_channel(device, device->generation);
}

/*
Index: linux/drivers/firewire/fw-device.h
===================================================================
--- linux.orig/drivers/firewire/fw-device.h
+++ linux/drivers/firewire/fw-device.h
@@ -71,7 +71,6 @@ struct fw_device {
int node_id;
int generation;
unsigned max_speed;
- bool cmc;
struct fw_card *card;
struct device device;

@@ -81,6 +80,9 @@ struct fw_device {
u32 *config_rom;
size_t config_rom_length;
int config_rom_retries;
+ unsigned cmc:1;
+ unsigned bc_implemented:2;
+
struct delayed_work work;
struct fw_attribute_group attribute_group;
};
@@ -109,6 +111,7 @@ static inline void fw_device_put(struct

struct fw_device *fw_device_get_by_devt(dev_t devt);
int fw_device_enable_phys_dma(struct fw_device *device);
+void fw_device_set_broadcast_channel(struct fw_device *device, int generation);

void fw_device_cdev_update(struct fw_device *device);
void fw_device_cdev_remove(struct fw_device *device);
Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -526,6 +526,7 @@ void fw_core_handle_bus_reset(struct fw_

spin_lock_irqsave(&card->lock, flags);

+ card->broadcast_channel_allocated = false;
card->node_id = node_id;
/*
* Update node_id before generation to prevent anybody from using
Index: linux/drivers/firewire/fw-transaction.h
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.h
+++ linux/drivers/firewire/fw-transaction.h
@@ -230,11 +230,6 @@ struct fw_card {
u8 color; /* must be u8 to match the definition in struct fw_node */
int gap_count;
bool beta_repeaters_present;
- /*
- * Set if the local device is the IRM and the broadcast channel
- * was allocated.
- */
- bool is_irm;

int index;

@@ -245,6 +240,7 @@ struct fw_card {
int bm_retries;
int bm_generation;

+ bool broadcast_channel_allocated;
u32 broadcast_channel;
u32 topology_map[(CSR_TOPOLOGY_MAP_END - CSR_TOPOLOGY_MAP) / 4];
};

--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

2009-03-10 20:11:11

by Stefan Richter

[permalink] [raw]
Subject: [PATCH 5/5] firewire: some simplifications in firewire-core + a fix

There is no 5th patch. I can't count.
--
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/