[PATCH 0/3] New firewire stack

[PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Hi,

I'm announcing an alternative firewire stack that I've been working on
the last few weeks.  I'm aiming to implement feature parity with the
current firewire stack, but not necessarily interface compatibility.
For now, I have the low-level OHCI driver done, the mid-level
transaction logic done, and the SBP-2 (storage) driver is basically
done.  What's missing is a streaming interface (in progress) to allow
reception and transmission of isochronous data and a userspace
interface for controlling devices (much like raw1394 or libusb for
usb).  I'm working out of this git repository:

  http://gitweb.freedesktop.org/?p=users/krh/juju.git

but I'll be sending 3 patches for review after this mail: first the
core subsystem, then the OHCI driver and finally the SBP-2 (SCSI over
firewire) driver.  For people who want to test this out, the easiest
approach right now is to clone the git repo and run make.  This
requires the kernel-devel RPM on Fedora Core; I'm sure other distros
have a similar package.

Now, I didn't set out to rewrite the entire firewire stack.  At first
I just wanted to fix the OHCI driver.  However any rewrite that
addresses the problems in the driver will shift the code around enough
to invalidate the quirks and workarounds there.  And frankly, I don't
trust most of the workarounds to begin with.  So I decided to write
the OHCI driver from scratch.

The rest of the stack has problems too, there's too many kernel
threads bouncing around, the nodemgr code is racy and doesn't really
consider issues such as hotplug during device probing.  And there is 5
different interfaces for doing isochronous streaming.

The new stack is more compact and I'd like to think it's easier to
follow the code.  Here are the sizes for the three patches that
follow:

[juju:linux-2.6]$ wc -l patches-juju/*.patch
  3983 patches-juju/fw-core.patch
  1510 patches-juju/fw-ohci.patch
  1114 patches-juju/fw-sbp2.patch
  6607 total

Compared to

[krh@dinky ieee1394]$ wc -l *.[ch]
 ...
 30431 total

The new stack can co-exists with the old stack, since it's sitting in
drivers/fw.  So users can pick which stack they want at compile time,
or maybe compile both and switch at run-time using a modprobe
blacklist file.  This allows a transition phase from the old stack to
the new one where interfaces will be awailable.

At this point I'm not proposing the stack for inclusion into mainline
yet, as I'm still developing the streaming interface and the userspace
control interface.  This is just a heads up for now, to announce the
effort and where I'd like to go with this.  It is basically useful
with the storage devices I have available here, though, and ready for
testing for that specific use case.  Once the remaining features land,
I'd like to see this in mainstream linux and I'm interested in hearing
how people feel about this.

cheers,
Kristian

[PATCH 2/3] Import fw-ohci driver.

[Kristian Høgsberg]

Add the OHCI driver to the stack and build system.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---

 drivers/fw/fw-ohci.c | 1334 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/fw/fw-ohci.h |  152 ++++++
 2 files changed, 1486 insertions(+), 0 deletions(-)

diff --git a/drivers/fw/fw-ohci.c b/drivers/fw/fw-ohci.c
new file mode 100644
index 0000000..78e0324
--- /dev/null
+++ b/drivers/fw/fw-ohci.c
@@ -0,0 +1,1334 @@
+/*						-*- c-basic-offset: 8 -*-
+ *
+ * fw-ohci.c - Driver for OHCI 1394 boards
+ * Copyright (C) 2003 Kristian Høgsberg <krh@bitplanet.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include <linux/poll.h>
+#include <asm/uaccess.h>
+#include <asm/semaphore.h>
+
+#include "fw-transaction.h"
+#include "fw-ohci.h"
+
+struct descriptor {
+	u32 req_count:16;
+
+	u32 wait:2;
+	u32 branch:2;
+	u32 irq:2;
+	u32 yy:1;
+	u32 ping:1;
+
+	u32 key:3;
+	u32 status:1;
+	u32 command:4;
+
+	u32 data_address;
+	u32 branch_address;
+
+	u32 res_count:16;
+	u32 transfer_status:16;
+} __attribute__ ((aligned(16)));
+
+#define DESCRIPTOR_OUTPUT_MORE		0
+#define DESCRIPTOR_OUTPUT_LAST		1
+#define DESCRIPTOR_INPUT_MORE		2
+#define DESCRIPTOR_INPUT_LAST		3
+#define DESCRIPTOR_NO_IRQ		0
+#define DESCRIPTOR_IRQ_ERROR		1
+#define DESCRIPTOR_IRQ_ALWAYS		3
+#define DESCRIPTOR_KEY_IMMEDIATE	2
+#define DESCRIPTOR_BRANCH_ALWAYS	3
+
+struct ar_context {
+	struct fw_ohci *ohci;
+	struct descriptor descriptor;
+	u32 buffer[512];
+	dma_addr_t descriptor_bus;
+	dma_addr_t buffer_bus;
+
+	u32 command_ptr;
+	u32 control_set;
+	u32 control_clear;
+
+	struct tasklet_struct tasklet;
+};
+
+struct at_context {
+	struct fw_ohci *ohci;
+	dma_addr_t descriptor_bus;
+	dma_addr_t buffer_bus;
+
+	struct list_head list;
+
+	struct {
+		struct descriptor more;
+		u32 header[4];
+		struct descriptor last;
+	} descriptor;
+
+	u32 command_ptr;
+	u32 control_set;
+	u32 control_clear;
+
+	struct tasklet_struct tasklet;
+};
+
+struct it_header {
+	u32 sy:4;
+	u32 tcode:4;
+	u32 channel:6;
+	u32 tag:2;
+	u32 speed:3;
+	u32 reserved0:13;
+	u32 reserved1:16;
+	u32 data_length:16;
+};
+
+struct iso_context {
+	struct fw_iso_context base;
+	struct tasklet_struct tasklet;
+	u32 control_set;
+	u32 control_clear;
+	u32 command_ptr;
+	u32 context_match;
+
+	struct descriptor *buffer;
+	dma_addr_t buffer_bus;
+	struct descriptor *head_descriptor;
+	struct descriptor *tail_descriptor;
+	struct descriptor *tail_descriptor_last;
+	struct descriptor *prev_descriptor;
+};
+
+#define CONFIG_ROM_SIZE 1024
+
+struct fw_ohci {
+	struct fw_card card;
+
+	struct pci_dev *dev;
+	char *registers;
+	dma_addr_t self_id_bus;
+	u32 *self_id_cpu;
+	struct tasklet_struct bus_reset_tasklet;
+	int generation;
+	int request_generation;
+
+	spinlock_t lock;
+	u32 self_id_buffer[512];
+
+	/* Config rom buffers */
+	u32 *config_rom;
+	dma_addr_t config_rom_bus;
+	u32 *next_config_rom;
+	dma_addr_t next_config_rom_bus;
+
+	struct ar_context ar_request_ctx;
+	struct ar_context ar_response_ctx;
+	struct at_context at_request_ctx;
+	struct at_context at_response_ctx;
+
+	u32 it_context_mask;
+	struct iso_context *it_context_list;
+	u32 ir_context_mask;
+	struct iso_context *ir_context_list;
+};
+
+extern inline struct fw_ohci *fw_ohci(struct fw_card *card)
+{
+	return container_of(card, struct fw_ohci, card);
+}
+
+#define CONTEXT_CYCLE_MATCH_ENABLE	0x80000000
+
+#define CONTEXT_RUN	0x8000
+#define CONTEXT_WAKE	0x1000
+#define CONTEXT_DEAD	0x0800
+#define CONTEXT_ACTIVE	0x0400
+
+#define OHCI1394_MAX_AT_REQ_RETRIES	0x2
+#define OHCI1394_MAX_AT_RESP_RETRIES	0x2
+#define OHCI1394_MAX_PHYS_RESP_RETRIES	0x8
+
+#define FW_OHCI_MAJOR			240
+#define OHCI1394_REGISTER_SIZE		0x800
+#define OHCI_LOOP_COUNT			500
+#define OHCI1394_PCI_HCI_Control	0x40
+#define SELF_ID_BUF_SIZE		0x800
+
+#define PCI_CLASS_FIREWIRE_OHCI ((PCI_CLASS_SERIAL_FIREWIRE << 8) | 0x10)
+
+static char ohci_driver_name[] = KBUILD_MODNAME;
+
+extern inline void reg_write(const struct fw_ohci *ohci, int offset, u32 data)
+{
+	writel(data, ohci->registers + offset);
+}
+
+extern inline u32 reg_read(const struct fw_ohci *ohci, int offset)
+{
+	return readl(ohci->registers + offset);
+}
+
+static int read_phy_reg(struct fw_ohci *ohci, u8 addr)
+{
+	u32 val;
+
+	reg_write(ohci, OHCI1394_PhyControl, OHCI1394_PhyControl_Read(addr));
+	msleep(2);
+	val = reg_read(ohci, OHCI1394_PhyControl);
+	if ((val & OHCI1394_PhyControl_ReadDone) == 0)
+		return -EBUSY;
+
+	return OHCI1394_PhyControl_ReadData(val);
+}
+
+static void write_phy_reg(struct fw_ohci *ohci, u8 addr, u8 data)
+{
+	reg_write(ohci, OHCI1394_PhyControl,
+		  OHCI1394_PhyControl_Write(addr, data));
+}
+
+static int
+ohci_update_phy_reg(struct fw_card *card, int addr,
+		    int clear_bits, int set_bits)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	int old;
+
+	old = read_phy_reg(ohci, addr);
+	if (old < 0) {
+		fw_error("failed to set phy reg bits.\n");
+		return old;
+	}
+	old = (old & ~clear_bits) | set_bits;
+	write_phy_reg(ohci, addr, old);
+
+	return 0;
+}
+
+static void ar_context_run(struct ar_context *ctx)
+{
+	reg_write(ctx->ohci, ctx->command_ptr, ctx->descriptor_bus | 1);
+	reg_write(ctx->ohci, ctx->control_set, CONTEXT_RUN);
+}
+
+static void ar_context_tasklet(unsigned long data)
+{
+	struct ar_context *ctx = (struct ar_context *)data;
+	struct fw_ohci *ohci = ctx->ohci;
+	u32 status;
+	int length, speed, ack, timestamp;
+
+	/* FIXME: What to do about evt_* errors? */
+	length    = ctx->descriptor.req_count - ctx->descriptor.res_count - 4;
+	status    = ctx->buffer[length / 4];
+	ack       = ((status >> 16) & 0x1f) - 16;
+	speed     = (status >> 21) & 0x7;
+	timestamp = status & 0xffff;
+
+	/* The OHCI bus reset handler synthesizes a phy packet with
+	 * the new generation number when a bus reset happens (see
+	 * section 8.4.2.3).  This helps us determine when a request
+	 * was received and make sure we send the response in the same
+	 * generation.  We only need this for requests; for responses
+	 * we use the unique tlabel for finding the matching
+	 * request. */
+	if (ack + 16 == 0x09)
+		ohci->request_generation = (ctx->buffer[2] >> 16) & 0xff;
+	else if (ctx == &ohci->ar_request_ctx)
+		fw_core_handle_request(&ohci->card, speed, ack, timestamp,
+				       ohci->request_generation,
+				       length, ctx->buffer);
+	else
+		fw_core_handle_response(&ohci->card, speed, ack, timestamp,
+					length, ctx->buffer);
+
+	ctx->descriptor.data_address = ctx->buffer_bus;
+	ctx->descriptor.req_count = sizeof ctx->buffer;
+	ctx->descriptor.res_count = ctx->descriptor.req_count;
+
+	dma_sync_single_for_device(&ohci->dev->dev, ctx->descriptor_bus,
+				   sizeof ctx->descriptor_bus, DMA_TO_DEVICE);
+
+	/* FIXME: We stop and restart the ar context here, what if we
+	 * stop while a receive is in progress? Maybe we could just
+	 * loop the context back to itself and use it in buffer fill
+	 * mode as intended... */
+
+	reg_write(ctx->ohci, ctx->control_clear, CONTEXT_RUN);
+	ar_context_run(ctx);
+}
+
+static int
+ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci, u32 control_set)
+{
+	/* FIXME: cpu_to_le32. */
+
+	ctx->descriptor_bus =
+		dma_map_single(&ohci->dev->dev, &ctx->descriptor,
+			       sizeof ctx->descriptor, PCI_DMA_TODEVICE);
+	if (ctx->descriptor_bus == 0)
+		return -ENOMEM;
+
+	if (ctx->descriptor_bus & 0xf)
+		fw_notify("descriptor not 16-byte aligned: 0x%08x\n",
+			  ctx->descriptor_bus);
+
+	ctx->buffer_bus =
+		dma_map_single(&ohci->dev->dev, ctx->buffer,
+			       sizeof ctx->buffer, PCI_DMA_FROMDEVICE);
+
+	if (ctx->buffer_bus == 0)
+		return -ENOMEM;
+
+	memset(&ctx->descriptor, 0, sizeof ctx->descriptor);
+	ctx->descriptor.command      = DESCRIPTOR_INPUT_MORE;
+	ctx->descriptor.status       = 1;
+	ctx->descriptor.irq          = DESCRIPTOR_NO_IRQ;
+	ctx->descriptor.branch       = DESCRIPTOR_BRANCH_ALWAYS;
+	ctx->descriptor.req_count    = sizeof ctx->buffer;
+	ctx->descriptor.data_address = ctx->buffer_bus;
+	ctx->descriptor.res_count    = ctx->descriptor.req_count;
+
+	ctx->control_set   = control_set;
+	ctx->control_clear = control_set + 4;
+	ctx->command_ptr   = control_set + 12;
+	ctx->ohci          = ohci;
+
+	tasklet_init(&ctx->tasklet, ar_context_tasklet, (unsigned long)ctx);
+
+	ar_context_run(ctx);
+
+	return 0;
+}
+
+static void
+complete_transmission(struct fw_packet *packet, struct fw_ohci *ohci,
+		      int status)
+{
+	list_del(&packet->link);
+	packet->callback(packet, &ohci->card, status);
+}
+
+/* This function prepares the first packet in the context queue for
+ * transmission.  Must always be called with the ochi->lock held to
+ * ensure proper generation handling and locking around packet queue
+ * manipulation. */
+static void at_context_setup_packet(struct at_context *ctx)
+{
+	struct fw_packet *packet;
+	struct fw_ohci *ohci = ctx->ohci;
+	int z, i;
+
+	packet = fw_packet(ctx->list.next);
+
+	memset(&ctx->descriptor.more, 0, sizeof ctx->descriptor.more);
+	if (packet->payload_length > 0) {
+		packet->payload_bus = dma_map_single(&ohci->dev->dev,
+						     packet->payload,
+						     packet->payload_length,
+						     PCI_DMA_TODEVICE);
+		if (packet->payload_bus == 0) {
+			complete_transmission(packet, ohci, -ENOMEM);
+			return;
+		}
+
+		ctx->descriptor.more.command      = DESCRIPTOR_OUTPUT_MORE;
+		ctx->descriptor.more.key          = DESCRIPTOR_KEY_IMMEDIATE;
+		ctx->descriptor.more.req_count    = packet->header_length;
+		ctx->descriptor.last.command      = DESCRIPTOR_OUTPUT_LAST;
+		ctx->descriptor.last.irq          = DESCRIPTOR_IRQ_ALWAYS;
+		ctx->descriptor.last.branch       = DESCRIPTOR_BRANCH_ALWAYS;
+		ctx->descriptor.last.req_count    = packet->payload_length;
+		ctx->descriptor.last.data_address = packet->payload_bus;
+		ctx->descriptor.last.res_count    = packet->timestamp;
+		z = 3;
+	} else {
+		ctx->descriptor.more.command   = DESCRIPTOR_OUTPUT_LAST;
+		ctx->descriptor.more.key       = DESCRIPTOR_KEY_IMMEDIATE;
+		ctx->descriptor.more.irq       = DESCRIPTOR_IRQ_ALWAYS;
+		ctx->descriptor.more.branch    = DESCRIPTOR_BRANCH_ALWAYS;
+		ctx->descriptor.more.req_count = packet->header_length;
+		ctx->descriptor.more.res_count = packet->timestamp;
+		z = 2;
+	}
+
+	/* The DMA format for asyncronous link packets is different
+	 * from the IEEE1394 layout, so shift the fields around
+	 * accordingly.  If header_length is 8, it's a PHY packet, to
+	 * which we need to prepend an extra quadlet. */
+	if (packet->header_length > 8) {
+		ctx->descriptor.header[0] =
+			(packet->header[0] & 0xffff) | (packet->speed << 16);
+		ctx->descriptor.header[1] =
+			(packet->header[1] & 0xffff) |
+			(packet-> header[0] & 0xffff0000);
+		for (i = 2; i < packet->header_length / 4; i++)
+			ctx->descriptor.header[i] = packet->header[i];
+	} else {
+		ctx->descriptor.header[0] =
+			(OHCI1394_phy_tcode << 4) | (packet->speed << 16);
+		ctx->descriptor.header[1] = packet->header[0];
+		ctx->descriptor.header[2] = packet->header[1];
+		ctx->descriptor.more.req_count = 12;
+	}
+
+	/* FIXME: Document how the locking works. */
+	if (ohci->generation == packet->generation) {
+		reg_write(ctx->ohci, ctx->command_ptr,
+			  ctx->descriptor_bus | z);
+		reg_write(ctx->ohci, ctx->control_set,
+			  CONTEXT_RUN | CONTEXT_WAKE);
+	} else {
+		/* We dont return error codes from this function; all
+		 * transmission errors are reported through the
+		 * callback. */
+		complete_transmission(packet, ohci, -ESTALE);
+	}
+}
+
+static void at_context_stop(struct at_context *ctx)
+{
+	u32 reg;
+
+	reg_write(ctx->ohci, ctx->control_clear, CONTEXT_RUN);
+
+	reg = reg_read(ctx->ohci, ctx->control_set);
+	if (reg & CONTEXT_ACTIVE)
+		fw_notify("Tried to stop context, but it is still active "
+			  "(0x%08x).\n", reg);
+}
+
+static void at_context_tasklet(unsigned long data)
+{
+	struct at_context *ctx = (struct at_context *)data;
+	struct fw_ohci *ohci = ctx->ohci;
+	struct fw_packet *packet;
+	unsigned long flags;
+	int evt;
+
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	packet = fw_packet(ctx->list.next);
+
+	at_context_stop(ctx);
+
+	if (packet->payload_length > 0) {
+		pci_unmap_single(ohci->dev, packet->payload_bus,
+				 packet->payload_length, PCI_DMA_TODEVICE);
+		evt = ctx->descriptor.last.transfer_status & 0x1f;
+		packet->timestamp = ctx->descriptor.last.res_count;
+	} else {
+		evt = ctx->descriptor.more.transfer_status & 0x1f;
+		packet->timestamp = ctx->descriptor.more.res_count;
+	}
+
+	if (evt < 16) {
+		switch (evt) {
+		case OHCI1394_evt_timeout:
+			/* Async response transmit timed out. */
+			complete_transmission(packet, ohci, -ETIMEDOUT);
+			break;
+
+		case OHCI1394_evt_flushed:
+			/* The packet was flushed should give same
+			 * error as when we try to use a stale
+			 * generation count. */
+			complete_transmission(packet, ohci, -ESTALE);
+			break;
+
+		case OHCI1394_evt_missing_ack:
+			/* This would be a higher level software
+			 * error, it is using a valid (current)
+			 * generation count, but the node is not on
+			 * the bus. */
+			complete_transmission(packet, ohci, -ENODEV);
+			break;
+
+		default:
+			complete_transmission(packet, ohci, -EIO);
+			break;
+		}
+	} else
+		complete_transmission(packet, ohci, evt - 16);
+
+	/* If more packets are queued, set up the next one. */
+	if (!list_empty(&ctx->list))
+		at_context_setup_packet(ctx);
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+}
+
+static int
+at_context_init(struct at_context *ctx, struct fw_ohci *ohci, u32 control_set)
+{
+	INIT_LIST_HEAD(&ctx->list);
+
+	ctx->descriptor_bus =
+	    dma_map_single(&ohci->dev->dev, &ctx->descriptor,
+			   sizeof ctx->descriptor, PCI_DMA_TODEVICE);
+	if (ctx->descriptor_bus == 0)
+		return -ENOMEM;
+
+	ctx->control_set   = control_set;
+	ctx->control_clear = control_set + 4;
+	ctx->command_ptr   = control_set + 12;
+	ctx->ohci          = ohci;
+
+	tasklet_init(&ctx->tasklet, at_context_tasklet, (unsigned long)ctx);
+
+	return 0;
+}
+
+static void
+at_context_transmit(struct at_context *ctx, struct fw_packet *packet)
+{
+	unsigned long flags;
+	int was_empty;
+
+	spin_lock_irqsave(&ctx->ohci->lock, flags);
+
+	was_empty = list_empty(&ctx->list);
+	list_add_tail(&packet->link, &ctx->list);
+	if (was_empty)
+		at_context_setup_packet(ctx);
+
+	spin_unlock_irqrestore(&ctx->ohci->lock, flags);
+}
+
+static void bus_reset_tasklet(unsigned long data)
+{
+	struct fw_ohci *ohci = (struct fw_ohci *)data;
+	int self_id_count, i, j, reg, node_id;
+	int generation, new_generation;
+	unsigned long flags;
+
+	reg = reg_read(ohci, OHCI1394_NodeID);
+	if (!(reg & OHCI1394_NodeID_idValid)) {
+		fw_error("node ID not valid, new bus reset in progress\n");
+		return;
+	}
+	node_id = reg & 0xffff;
+
+	/* The count in the SelfIDCount register is the number of
+	 * bytes in the self ID receive buffer.  Since we also receive
+	 * the inverted quadlets and a header quadlet, we shift one
+	 * bit extra to get the actual number of self IDs. */
+
+	self_id_count = (reg_read(ohci, OHCI1394_SelfIDCount) >> 3) & 0x3ff;
+	generation = (ohci->self_id_cpu[0] >> 16) & 0xff;
+
+	for (i = 1, j = 0; j < self_id_count; i += 2, j++) {
+		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1])
+			fw_error("inconsistent self IDs\n");
+		ohci->self_id_buffer[j] = ohci->self_id_cpu[i];
+	}
+
+	/* Check the consistency of the self IDs we just read.  The
+	 * problem we face is that a new bus reset can start while we
+	 * read out the self IDs from the DMA buffer. If this happens,
+	 * the DMA buffer will be overwritten with new self IDs and we
+	 * will read out inconsistent data.  The OHCI specification
+	 * (section 11.2) recommends a technique similar to
+	 * linux/seqlock.h, where we remember the generation of the
+	 * self IDs in the buffer before reading them out and compare
+	 * it to the current generation after reading them out.  If
+	 * the two generations match we know we have a consistent set
+	 * of self IDs. */
+
+	new_generation = (reg_read(ohci, OHCI1394_SelfIDCount) >> 16) & 0xff;
+	if (new_generation != generation) {
+		fw_notify("recursive bus reset detected, "
+			  "discarding self ids\n");
+		return;
+	}
+
+	/* FIXME: Document how the locking works. */
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	ohci->generation = generation;
+	at_context_stop(&ohci->at_request_ctx);
+	at_context_stop(&ohci->at_response_ctx);
+	reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
+
+	/* This next bit is unrelated to the AT context stuff but we
+	 * have to do it under the spinlock also.  If a new config rom
+	 * was set up before this reset, the old one is now no longer
+	 * in use and we can free it. Update the config rom pointers
+	 * to point to the current config rom and clear the
+	 * next_config_rom pointer so a new udpate can take place. */
+
+	if (ohci->next_config_rom != NULL) {
+		pci_free_consistent(ohci->dev, CONFIG_ROM_SIZE,
+				    ohci->config_rom, ohci->config_rom_bus);
+		ohci->config_rom      = ohci->next_config_rom;
+		ohci->config_rom_bus  = ohci->next_config_rom_bus;
+		ohci->next_config_rom = NULL;
+	}
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+
+	fw_core_handle_bus_reset(&ohci->card, node_id, generation,
+				 self_id_count, ohci->self_id_buffer);
+}
+
+static irqreturn_t irq_handler(int irq, void *data, struct pt_regs *unused)
+{
+	struct fw_ohci *ohci = data;
+	u32 event, iso_event;
+	int i;
+
+	event = reg_read(ohci, OHCI1394_IntEventClear);
+
+	if (!event)
+		return IRQ_NONE;
+
+	reg_write(ohci, OHCI1394_IntEventClear, event);
+
+	if (event & OHCI1394_selfIDComplete)
+		tasklet_schedule(&ohci->bus_reset_tasklet);
+
+	if (event & OHCI1394_RQPkt)
+		tasklet_schedule(&ohci->ar_request_ctx.tasklet);
+
+	if (event & OHCI1394_RSPkt)
+		tasklet_schedule(&ohci->ar_response_ctx.tasklet);
+
+	if (event & OHCI1394_reqTxComplete)
+		tasklet_schedule(&ohci->at_request_ctx.tasklet);
+
+	if (event & OHCI1394_respTxComplete)
+		tasklet_schedule(&ohci->at_response_ctx.tasklet);
+
+	iso_event = reg_read(ohci, OHCI1394_IsoRecvIntEventSet);
+	reg_write(ohci, OHCI1394_IsoRecvIntEventClear, iso_event);
+
+	while (iso_event) {
+		i = ffs(iso_event) - 1;
+		tasklet_schedule(&ohci->ir_context_list[i].tasklet);
+		iso_event &= ~(1 << i);
+	}
+
+	iso_event = reg_read(ohci, OHCI1394_IsoXmitIntEventSet);
+	reg_write(ohci, OHCI1394_IsoXmitIntEventClear, iso_event);
+
+	while (iso_event) {
+		i = ffs(iso_event) - 1;
+		tasklet_schedule(&ohci->it_context_list[i].tasklet);
+		iso_event &= ~(1 << i);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ohci_enable(struct fw_card *card, u32 * config_rom, size_t length)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+
+	/* When the link is not yet enabled, the atomic config rom
+	 * described above is not active.  We have to update
+	 * ConfigRomHeader and BusOptions manually, and the write to
+	 * ConfigROMmap takes effect immediately.  We tie this to the
+	 * enabling of the link, so we ensure that we have a valid
+	 * config rom before enabling - the OHCI requires that
+	 * ConfigROMhdr and BusOptions have valid values before
+	 * enabling.
+	 */
+
+	ohci->config_rom = pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
+						&ohci->config_rom_bus);
+	if (ohci->config_rom == NULL)
+		return -ENOMEM;
+
+	memset(ohci->config_rom, 0, CONFIG_ROM_SIZE);
+	fw_memcpy_to_be32(ohci->config_rom, config_rom, length * 4);
+
+	reg_write(ohci, OHCI1394_ConfigROMmap, ohci->config_rom_bus);
+	reg_write(ohci, OHCI1394_ConfigROMhdr, config_rom[0]);
+	reg_write(ohci, OHCI1394_BusOptions, config_rom[2]);
+
+	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
+
+	if (request_irq(ohci->dev->irq, irq_handler,
+			SA_SHIRQ, ohci_driver_name, ohci)) {
+		fw_error("Failed to allocate shared interrupt %d.\n",
+			 ohci->dev->irq);
+		return -EIO;
+	}
+
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_linkEnable |
+		  OHCI1394_HCControl_BIBimageValid);
+
+	/* We are ready to go, initiate bus reset to finish the
+	 * initialization. */
+
+	fw_core_initiate_bus_reset(&ohci->card, 1);
+
+	return 0;
+}
+
+static int
+ohci_set_config_rom(struct fw_card *card, u32 * config_rom, size_t length)
+{
+	struct fw_ohci *ohci;
+	unsigned long flags;
+	int retval = 0;
+
+	ohci = fw_ohci(card);
+
+	/* When the OHCI controller is enabled, the config rom update
+	 * mechanism is a bit tricky, but easy enough to use.  See
+	 * section 5.5.6 in the OHCI specification.
+	 *
+	 * The OHCI controller caches the new config rom address in a
+	 * shadow register (ConfigROMmapNext) and needs a bus reset
+	 * for the changes to take place.  When the bus reset is
+	 * detected, the controller loads the new values for the
+	 * ConfigRomHeader and BusOptions registers from the specified
+	 * config rom and loads ConfigROMmap from the ConfigROMmapNext
+	 * shadow register. All automatically and atomically.
+	 *
+	 * We use ohci->lock to avoid racing with the code that sets
+	 * ohci->next_config_rom to NULL (see bus_reset_tasklet).
+	 */
+
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	if (ohci->next_config_rom == NULL) {
+		ohci->next_config_rom =
+			pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
+					     &ohci->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);
+		reg_write(ohci, OHCI1394_ConfigROMmap,
+			  ohci->next_config_rom_bus);
+
+		/* Now initiate a bus reset to have the changes take
+		 * effect. We clean up the old config rom memory and
+		 * DMA mappings in the bus reset tasklet, since the
+		 * OHCI controller could need to access it before the
+		 * bus reset takes effect.
+		 */
+
+		fw_core_initiate_bus_reset(&ohci->card, 1);
+	} else
+		retval = -EBUSY;
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+
+	return retval;
+}
+
+static void ohci_send_request(struct fw_card *card, struct fw_packet *packet)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+
+	at_context_transmit(&ohci->at_request_ctx, packet);
+}
+
+static void ohci_send_response(struct fw_card *card, struct fw_packet *packet)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+
+	at_context_transmit(&ohci->at_response_ctx, packet);
+}
+
+static int
+ohci_enable_phys_dma(struct fw_card *card, int node_id, int generation)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	unsigned long flags;
+	int retval = 0;
+
+	/* FIXME: make sure this bitmask is cleared when we clear the
+	 * busReset interrupt bit. */
+
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	if (ohci->generation != generation) {
+		retval = -ESTALE;
+		goto out;
+	}
+
+	if (node_id < 32) {
+		reg_write(ohci, OHCI1394_PhyReqFilterLoSet, 1 << node_id);
+	} else {
+		reg_write(ohci, OHCI1394_PhyReqFilterHiSet,
+			  1 << (node_id - 32));
+	}
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+
+ out:
+	return retval;
+}
+
+static void ir_context_tasklet(unsigned long data)
+{
+	struct iso_context *ctx = (struct iso_context *)data;
+
+	(void)ctx;
+}
+
+#define ISO_BUFFER_SIZE (64 * 1024)
+
+static void flush_iso_context(struct iso_context *ctx)
+{
+	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
+	struct descriptor *d, *last;
+	int z;
+
+	dma_sync_single_for_cpu(ohci->card.device, ctx->buffer_bus,
+				ISO_BUFFER_SIZE, DMA_TO_DEVICE);
+
+	d    = ctx->tail_descriptor;
+	last = ctx->tail_descriptor_last;
+
+	while (last->branch_address != 0 && last->transfer_status != 0) {
+		z = last->branch_address & 0xf;
+		d = ctx->buffer + (last->branch_address -
+				   ctx->buffer_bus) / sizeof *d;
+
+		if (z == 2)
+			last = d;
+		else
+			last = d + z - 1;
+
+		if (last->irq)
+			ctx->base.callback(&ctx->base,
+					   0, last->res_count,
+					   ctx->base.callback_data);
+	}
+
+	ctx->tail_descriptor      = d;
+	ctx->tail_descriptor_last = last;
+}
+
+static void it_context_tasklet(unsigned long data)
+{
+	struct iso_context *ctx = (struct iso_context *)data;
+
+	flush_iso_context(ctx);
+}
+
+static struct fw_iso_context *ohci_allocate_iso_context(struct fw_card *card,
+							int type)
+{
+	struct fw_ohci *ohci = fw_ohci(card);
+	struct iso_context *ctx, *list;
+	void (*tasklet) (unsigned long data);
+	u32 *mask;
+	unsigned long flags;
+	int index;
+
+	if (type == FW_ISO_CONTEXT_TRANSMIT) {
+		mask = &ohci->it_context_mask;
+		list = ohci->it_context_list;
+		tasklet = it_context_tasklet;
+	} else {
+		mask = &ohci->ir_context_mask;
+		list = ohci->ir_context_list;
+		tasklet = ir_context_tasklet;
+	}
+
+	spin_lock_irqsave(&ohci->lock, flags);
+	index = ffs(*mask) - 1;
+	if (index >= 0)
+		*mask &= ~(1 << index);
+	spin_unlock_irqrestore(&ohci->lock, flags);
+
+	if (index < 0)
+		return ERR_PTR(-EBUSY);
+
+	ctx = &list[index];
+	memset(ctx, 0, sizeof *ctx);
+	tasklet_init(&ctx->tasklet, tasklet, (unsigned long)ctx);
+
+	ctx->buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
+	if (ctx->buffer == NULL) {
+		spin_lock_irqsave(&ohci->lock, flags);
+		*mask |= 1 << index;
+		spin_unlock_irqrestore(&ohci->lock, flags);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	ctx->buffer_bus =
+	    dma_map_single(card->device, ctx->buffer,
+			   ISO_BUFFER_SIZE, PCI_DMA_TODEVICE);
+
+	ctx->head_descriptor      = ctx->buffer;
+	ctx->prev_descriptor      = ctx->buffer;
+	ctx->tail_descriptor      = ctx->buffer;
+	ctx->tail_descriptor_last = ctx->buffer;
+
+	/* We put a dummy descriptor in the buffer that has a NULL
+	 * branch address and looks like it's been sent.  That way we
+	 * have a descriptor to append DMA programs to.  Also, the
+	 * ring buffer invariant is that it always has at least one
+	 * element so that head == tail means buffer full. */
+
+	memset(ctx->head_descriptor, 0, sizeof *ctx->head_descriptor);
+	ctx->head_descriptor->command         = DESCRIPTOR_OUTPUT_LAST;
+	ctx->head_descriptor->transfer_status = 0x8011;
+	ctx->head_descriptor++;
+
+	return &ctx->base;
+}
+
+static int ohci_send_iso(struct fw_iso_context *base, s32 cycle)
+{
+	struct iso_context *ctx = (struct iso_context *)base;
+	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
+	u32 cycle_match = 0;
+	int index;
+
+	index = ctx - ohci->it_context_list;
+	if (cycle > 0)
+		cycle_match = CONTEXT_CYCLE_MATCH_ENABLE |
+			(cycle & 0x7fff) << 16;
+
+	reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, 1 << index);
+	reg_write(ohci, OHCI1394_IsoXmitCommandPtr(index),
+		  ctx->tail_descriptor_last->branch_address);
+	reg_write(ohci, OHCI1394_IsoXmitContextControlClear(index), ~0);
+	reg_write(ohci, OHCI1394_IsoXmitContextControlSet(index),
+		  CONTEXT_RUN | cycle_match);
+
+	return 0;
+}
+
+static void ohci_free_iso_context(struct fw_iso_context *base)
+{
+	struct fw_ohci *ohci = fw_ohci(base->card);
+	struct iso_context *ctx = (struct iso_context *)base;
+	unsigned long flags;
+	int index;
+
+	flush_iso_context(ctx);
+
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	if (ctx->base.type == FW_ISO_CONTEXT_TRANSMIT) {
+		index = ctx - ohci->it_context_list;
+		reg_write(ohci, OHCI1394_IsoXmitContextControlClear(index), ~0);
+		reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, 1 << index);
+		ohci->it_context_mask |= 1 << index;
+	} else {
+		index = ctx - ohci->ir_context_list;
+		reg_write(ohci, OHCI1394_IsoRcvContextControlClear(index), ~0);
+		reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, 1 << index);
+		ohci->ir_context_mask |= 1 << index;
+	}
+
+	dma_unmap_single(ohci->card.device, ctx->buffer_bus,
+			 ISO_BUFFER_SIZE, DMA_TO_DEVICE);
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+}
+
+static int
+ohci_queue_iso(struct fw_iso_context *base,
+	       struct fw_iso_packet *packet, void *payload)
+{
+	struct iso_context *ctx = (struct iso_context *)base;
+	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
+	struct descriptor *d, *end, *last, *tail, *pd;
+	struct fw_iso_packet *p;
+	struct it_header *header;
+	dma_addr_t d_bus;
+	u32 z, header_z, payload_z;
+	u32 payload_index, payload_end_index, next_page_index;
+	int index, page, end_page, i, length, offset;
+
+	/* FIXME: Cycle lost behavior should be configurable: lose
+	 * packet, retransmit or terminate.. */
+
+	p = packet;
+	payload_index = payload - ctx->base.buffer;
+	d = ctx->head_descriptor;
+	tail = ctx->tail_descriptor;
+	end = ctx->buffer + ISO_BUFFER_SIZE / sizeof(struct descriptor);
+
+	if (p->skip)
+		z = 1;
+	else
+		z = 2;
+	if (p->header_length > 0)
+		z++;
+
+	/* Determine the first page the payload isn't contained in. */
+	end_page = PAGE_ALIGN(payload_index + p->payload_length) >> PAGE_SHIFT;
+	if (p->payload_length > 0)
+		payload_z = end_page - (payload_index >> PAGE_SHIFT);
+	else
+		payload_z = 0;
+
+	z += payload_z;
+
+	/* Get header size in number of descriptors. */
+	header_z = DIV_ROUND_UP(p->header_length, sizeof *d);
+
+	if (d + z + header_z <= tail) {
+		goto has_space;
+	} else if (d > tail && d + z + header_z <= end) {
+		goto has_space;
+	} else if (d > tail && ctx->buffer + z + header_z <= tail) {
+		d = ctx->buffer;
+		goto has_space;
+	}
+
+	/* No space in buffer */
+	return -1;
+
+ has_space:
+	memset(d, 0, (z + header_z) * sizeof *d);
+	d_bus = ctx->buffer_bus + (d - ctx->buffer) * sizeof *d;
+
+	if (p->skip) {
+		d[0].key = 0;
+		d[0].req_count = 0;
+	} else {
+		d[0].key = DESCRIPTOR_KEY_IMMEDIATE;
+		d[0].req_count = 8;
+
+		header = (struct it_header *)&d[1];
+		header->sy          = p->sy;
+		header->tag         = p->tag;
+		header->tcode       = TCODE_STREAM_DATA;
+		header->channel     = ctx->base.channel;
+		header->speed       = ctx->base.speed;
+		header->data_length = p->header_length + p->payload_length;
+	}
+
+	if (p->header_length > 0) {
+		d[2].req_count    = p->header_length;
+		d[2].data_address = d_bus + z * sizeof *d;
+		memcpy(&d[z], p->header, p->header_length);
+	}
+
+	pd = d + z - payload_z;
+	payload_end_index = payload_index + p->payload_length;
+	for (i = 0; i < payload_z; i++) {
+		page               = payload_index >> PAGE_SHIFT;
+		offset             = payload_index & ~PAGE_MASK;
+		next_page_index    = (page + 1) << PAGE_SHIFT;
+		length             =
+			min(next_page_index, payload_end_index) - payload_index;
+		pd[i].req_count    = length;
+		pd[i].data_address = ctx->base.pages[page] + offset;
+
+		payload_index += length;
+	}
+
+	if (z == 2)
+		last = d;
+	else
+		last = d + z - 1;
+
+	last->command = DESCRIPTOR_OUTPUT_LAST;
+	last->status  = 1;
+	last->branch  = DESCRIPTOR_BRANCH_ALWAYS;
+	if (p->interrupt)
+		last->irq = DESCRIPTOR_IRQ_ALWAYS;
+	else
+		last->irq = DESCRIPTOR_NO_IRQ;
+
+	dma_sync_single_for_device(ohci->card.device, ctx->buffer_bus,
+				   ISO_BUFFER_SIZE, DMA_TO_DEVICE);
+
+	ctx->head_descriptor = d + z + header_z;
+	ctx->prev_descriptor->branch_address = d_bus | z;
+	ctx->prev_descriptor = last;
+
+	index = ctx - ohci->it_context_list;
+	reg_write(ohci, OHCI1394_IsoXmitContextControlSet(index), CONTEXT_WAKE);
+
+	return 0;
+}
+
+static struct fw_card_driver ohci_driver = {
+	.name			= ohci_driver_name,
+	.enable			= ohci_enable,
+	.update_phy_reg		= ohci_update_phy_reg,
+	.set_config_rom		= ohci_set_config_rom,
+	.send_request		= ohci_send_request,
+	.send_response		= ohci_send_response,
+	.enable_phys_dma	= ohci_enable_phys_dma,
+
+	.allocate_iso_context	= ohci_allocate_iso_context,
+	.free_iso_context	= ohci_free_iso_context,
+	.queue_iso		= ohci_queue_iso,
+	.send_iso		= ohci_send_iso
+};
+
+static int software_reset(struct fw_ohci *ohci)
+{
+	int i;
+
+	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
+
+	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
+		if ((reg_read(ohci, OHCI1394_HCControlSet) &
+		     OHCI1394_HCControl_softReset) == 0)
+			return 0;
+		msleep(1);
+	}
+
+	return -EBUSY;
+}
+
+/* ---------- pci subsystem interface ---------- */
+
+enum {
+	CLEANUP_IRQ,
+	CLEANUP_SELF_ID,
+	CLEANUP_REGISTERS,
+	CLEANUP_IOMEM,
+	CLEANUP_OHCI
+};
+
+static int cleanup(struct fw_ohci *ohci, int stage, int code)
+{
+	switch (stage) {
+	case CLEANUP_SELF_ID:
+		pci_free_consistent(ohci->dev, SELF_ID_BUF_SIZE,
+				    ohci->self_id_cpu, ohci->self_id_bus);
+	case CLEANUP_REGISTERS:
+		kfree(ohci->it_context_list);
+		kfree(ohci->ir_context_list);
+		iounmap(ohci->registers);
+	case CLEANUP_IOMEM:
+		release_mem_region(pci_resource_start(ohci->dev, 0),
+				   OHCI1394_REGISTER_SIZE);
+	case CLEANUP_OHCI:
+		fw_card_put(&ohci->card);
+	}
+
+	return code;
+}
+
+static int __devinit
+pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
+{
+	struct fw_ohci *ohci;
+	u32 base, bus_options, max_receive, link_speed;
+	u64 guid;
+	int error_code;
+	size_t size;
+
+	if (pci_enable_device(dev)) {
+		fw_error("Failed to enable OHCI hardware.\n");
+		return -ENXIO;
+	}
+
+	ohci = kzalloc(sizeof *ohci, SLAB_KERNEL);
+	if (ohci == NULL) {
+		fw_error("Could not malloc fw_ohci data.\n");
+		return -ENOMEM;
+	}
+
+	pci_set_master(dev);
+	pci_write_config_dword(dev, OHCI1394_PCI_HCI_Control, 0);
+	pci_set_drvdata(dev, ohci);
+
+	ohci->dev = dev;
+	spin_lock_init(&ohci->lock);
+
+	tasklet_init(&ohci->bus_reset_tasklet,
+		     bus_reset_tasklet, (unsigned long)ohci);
+
+	/* We hardcode the register set size, since some cards get
+	 * this wrong and others have some extra registers after the
+	 * OHCI range.  We only use the OHCI registers, so there's no
+	 * need to be clever.  */
+	base = pci_resource_start(dev, 0);
+	if (!request_mem_region(base, OHCI1394_REGISTER_SIZE, ohci_driver_name)) {
+		fw_error("MMIO resource (0x%x - 0x%x) unavailable\n",
+			 base, base + OHCI1394_REGISTER_SIZE);
+		return cleanup(ohci, CLEANUP_OHCI, -EBUSY);
+	}
+
+	ohci->registers = ioremap(base, OHCI1394_REGISTER_SIZE);
+	if (ohci->registers == NULL) {
+		fw_error("Failed to remap registers\n");
+		return cleanup(ohci, CLEANUP_IOMEM, -ENXIO);
+	}
+
+	if (software_reset(ohci)) {
+		fw_error("Failed to reset ohci card.\n");
+		return cleanup(ohci, CLEANUP_REGISTERS, -EBUSY);
+	}
+
+	/* Now enable LPS, which we need in order to start accessing
+	 * most of the registers.  In fact, on some cards (ALI M5251),
+	 * accessing registers in the SClk domain without LPS enabled
+	 * will lock up the machine.  Wait 50msec to make sure we have
+	 * full link enabled.  */
+	reg_write(ohci, OHCI1394_HCControlSet,
+		  OHCI1394_HCControl_LPS |
+		  OHCI1394_HCControl_postedWriteEnable);
+	msleep(50);
+
+	/* self-id dma buffer allocation */
+	ohci->self_id_cpu = pci_alloc_consistent(ohci->dev, SELF_ID_BUF_SIZE,
+						 &ohci->self_id_bus);
+	if (ohci->self_id_cpu == NULL) {
+		fw_error("Out of memory for self ID buffer.\n");
+		return cleanup(ohci, CLEANUP_REGISTERS, -ENOMEM);
+	}
+	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
+	reg_write(ohci, OHCI1394_LinkControlSet,
+		  OHCI1394_LinkControl_rcvSelfID |
+		  OHCI1394_LinkControl_cycleTimerEnable |
+		  OHCI1394_LinkControl_cycleMaster);
+
+	ar_context_init(&ohci->ar_request_ctx, ohci,
+			OHCI1394_AsReqRcvContextControlSet);
+
+	ar_context_init(&ohci->ar_response_ctx, ohci,
+			OHCI1394_AsRspRcvContextControlSet);
+
+	at_context_init(&ohci->at_request_ctx, ohci,
+			OHCI1394_AsReqTrContextControlSet);
+
+	at_context_init(&ohci->at_response_ctx, ohci,
+			OHCI1394_AsRspTrContextControlSet);
+
+	reg_write(ohci, OHCI1394_ATRetries,
+		  OHCI1394_MAX_AT_REQ_RETRIES |
+		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
+		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
+
+	reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0);
+	ohci->it_context_mask = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet);
+	reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0);
+	size = sizeof(struct iso_context) * hweight32(ohci->it_context_mask);
+	ohci->it_context_list = kzalloc(size, GFP_KERNEL);
+
+	reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, ~0);
+	ohci->ir_context_mask = reg_read(ohci, OHCI1394_IsoXmitIntMaskSet);
+	reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, ~0);
+	size = sizeof(struct iso_context) * hweight32(ohci->ir_context_mask);
+	ohci->ir_context_list = kzalloc(size, GFP_KERNEL);
+
+	if (ohci->it_context_list == NULL || ohci->ir_context_list == NULL) {
+		fw_error("Out of memory for it/ir contexts.\n");
+		return cleanup(ohci, CLEANUP_REGISTERS, -ENOMEM);
+	}
+
+	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
+	reg_write(ohci, OHCI1394_IntEventClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
+	reg_write(ohci, OHCI1394_IntMaskSet,
+		  OHCI1394_selfIDComplete |
+		  OHCI1394_RQPkt | OHCI1394_RSPkt |
+		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
+		  OHCI1394_isochRx | OHCI1394_isochTx |
+		  OHCI1394_masterIntEnable);
+
+	bus_options = reg_read(ohci, OHCI1394_BusOptions);
+	max_receive = (bus_options >> 12) & 0xf;
+	link_speed = bus_options & 0x7;
+	guid = (__u64) reg_read(ohci, OHCI1394_GUIDHi) << 32 |
+		reg_read(ohci, OHCI1394_GUIDLo);
+
+	error_code = fw_core_add_card(&ohci->card, &ohci_driver, &dev->dev,
+				      max_receive, link_speed, guid);
+	if (error_code < 0)
+		return cleanup(ohci, CLEANUP_SELF_ID, error_code);
+
+	fw_notify("Added fw-ohci device %s.\n", dev->dev.bus_id);
+
+	return 0;
+}
+
+static void pci_remove(struct pci_dev *dev)
+{
+	struct fw_ohci *ohci;
+
+	ohci = pci_get_drvdata(dev);
+	if (ohci == NULL)
+		return;
+
+	free_irq(ohci->dev->irq, ohci);
+	fw_core_remove_card(&ohci->card);
+
+	/* FIXME: Fail all pending packets here, now that the upper
+	 * layers can't queue any more. */
+
+	software_reset(ohci);
+	cleanup(ohci, CLEANUP_SELF_ID, 0);
+
+	fw_notify("Removed fw-ohci device.\n");
+}
+
+static struct pci_device_id pci_table[] = {
+	{
+		.class		= PCI_CLASS_FIREWIRE_OHCI,
+		.class_mask	= PCI_ANY_ID,
+		.vendor		= PCI_ANY_ID,
+		.device		= PCI_ANY_ID,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+	},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(pci, pci_table);
+
+static struct pci_driver fw_ohci_pci_driver = {
+	.name		= ohci_driver_name,
+	.id_table	= pci_table,
+	.probe		= pci_probe,
+	.remove		= pci_remove,
+};
+
+MODULE_AUTHOR("Kristian Høgsberg <krh@bitplanet.net>");
+MODULE_DESCRIPTION("Driver for PCI OHCI IEEE1394 controllers");
+MODULE_LICENSE("GPL");
+
+static int __init fw_ohci_init(void)
+{
+	if (pci_register_driver(&fw_ohci_pci_driver)) {
+		fw_error("Failed to register PCI driver.\n");
+		return -EBUSY;
+	}
+
+	fw_notify("Loaded fw-ohci driver.\n");
+
+	return 0;
+}
+
+static void __exit fw_ohci_cleanup(void)
+{
+	pci_unregister_driver(&fw_ohci_pci_driver);
+	fw_notify("Unloaded fw-ohci driver.\n");
+}
+
+module_init(fw_ohci_init);
+module_exit(fw_ohci_cleanup);
diff --git a/drivers/fw/fw-ohci.h b/drivers/fw/fw-ohci.h
new file mode 100644
index 0000000..e200b1b
--- /dev/null
+++ b/drivers/fw/fw-ohci.h
@@ -0,0 +1,152 @@
+#ifndef __fw_ohci_h
+#define __fw_ohci_h
+
+/* OHCI register map */
+
+#define OHCI1394_Version                      0x000
+#define OHCI1394_GUID_ROM                     0x004
+#define OHCI1394_ATRetries                    0x008
+#define OHCI1394_CSRData                      0x00C
+#define OHCI1394_CSRCompareData               0x010
+#define OHCI1394_CSRControl                   0x014
+#define OHCI1394_ConfigROMhdr                 0x018
+#define OHCI1394_BusID                        0x01C
+#define OHCI1394_BusOptions                   0x020
+#define OHCI1394_GUIDHi                       0x024
+#define OHCI1394_GUIDLo                       0x028
+#define OHCI1394_ConfigROMmap                 0x034
+#define OHCI1394_PostedWriteAddressLo         0x038
+#define OHCI1394_PostedWriteAddressHi         0x03C
+#define OHCI1394_VendorID                     0x040
+#define OHCI1394_HCControlSet                 0x050
+#define OHCI1394_HCControlClear               0x054
+#define  OHCI1394_HCControl_BIBimageValid	0x80000000
+#define  OHCI1394_HCControl_noByteSwap		0x40000000
+#define  OHCI1394_HCControl_programPhyEnable	0x00800000
+#define  OHCI1394_HCControl_aPhyEnhanceEnable	0x00400000
+#define  OHCI1394_HCControl_LPS			0x00080000
+#define  OHCI1394_HCControl_postedWriteEnable	0x00040000
+#define  OHCI1394_HCControl_linkEnable		0x00020000
+#define  OHCI1394_HCControl_softReset		0x00010000
+#define OHCI1394_SelfIDBuffer                 0x064
+#define OHCI1394_SelfIDCount                  0x068
+#define OHCI1394_IRMultiChanMaskHiSet         0x070
+#define OHCI1394_IRMultiChanMaskHiClear       0x074
+#define OHCI1394_IRMultiChanMaskLoSet         0x078
+#define OHCI1394_IRMultiChanMaskLoClear       0x07C
+#define OHCI1394_IntEventSet                  0x080
+#define OHCI1394_IntEventClear                0x084
+#define OHCI1394_IntMaskSet                   0x088
+#define OHCI1394_IntMaskClear                 0x08C
+#define OHCI1394_IsoXmitIntEventSet           0x090
+#define OHCI1394_IsoXmitIntEventClear         0x094
+#define OHCI1394_IsoXmitIntMaskSet            0x098
+#define OHCI1394_IsoXmitIntMaskClear          0x09C
+#define OHCI1394_IsoRecvIntEventSet           0x0A0
+#define OHCI1394_IsoRecvIntEventClear         0x0A4
+#define OHCI1394_IsoRecvIntMaskSet            0x0A8
+#define OHCI1394_IsoRecvIntMaskClear          0x0AC
+#define OHCI1394_InitialBandwidthAvailable    0x0B0
+#define OHCI1394_InitialChannelsAvailableHi   0x0B4
+#define OHCI1394_InitialChannelsAvailableLo   0x0B8
+#define OHCI1394_FairnessControl              0x0DC
+#define OHCI1394_LinkControlSet               0x0E0
+#define OHCI1394_LinkControlClear             0x0E4
+#define   OHCI1394_LinkControl_rcvSelfID	(1 << 9)
+#define   OHCI1394_LinkControl_rcvPhyPkt	(1 << 10)
+#define   OHCI1394_LinkControl_cycleTimerEnable	(1 << 20)
+#define   OHCI1394_LinkControl_cycleMaster	(1 << 21)
+#define   OHCI1394_LinkControl_cycleSource	(1 << 22)
+#define OHCI1394_NodeID                       0x0E8
+#define   OHCI1394_NodeID_idValid             0x80000000
+#define OHCI1394_PhyControl                   0x0EC
+#define   OHCI1394_PhyControl_Read(addr)	(((addr) << 8) | 0x00008000)
+#define   OHCI1394_PhyControl_ReadDone		0x80000000
+#define   OHCI1394_PhyControl_ReadData(r)	(((r) & 0x00ff0000) >> 16)
+#define   OHCI1394_PhyControl_Write(addr, data) (((addr) << 8) | (data) | 0x00004000)
+#define   OHCI1394_PhyControl_WriteDone		0x00004000
+#define OHCI1394_IsochronousCycleTimer        0x0F0
+#define OHCI1394_AsReqFilterHiSet             0x100
+#define OHCI1394_AsReqFilterHiClear           0x104
+#define OHCI1394_AsReqFilterLoSet             0x108
+#define OHCI1394_AsReqFilterLoClear           0x10C
+#define OHCI1394_PhyReqFilterHiSet            0x110
+#define OHCI1394_PhyReqFilterHiClear          0x114
+#define OHCI1394_PhyReqFilterLoSet            0x118
+#define OHCI1394_PhyReqFilterLoClear          0x11C
+#define OHCI1394_PhyUpperBound                0x120
+
+#define OHCI1394_AsReqTrContextBase           0x180
+#define OHCI1394_AsReqTrContextControlSet     0x180
+#define OHCI1394_AsReqTrContextControlClear   0x184
+#define OHCI1394_AsReqTrCommandPtr            0x18C
+
+#define OHCI1394_AsRspTrContextBase           0x1A0
+#define OHCI1394_AsRspTrContextControlSet     0x1A0
+#define OHCI1394_AsRspTrContextControlClear   0x1A4
+#define OHCI1394_AsRspTrCommandPtr            0x1AC
+
+#define OHCI1394_AsReqRcvContextBase          0x1C0
+#define OHCI1394_AsReqRcvContextControlSet    0x1C0
+#define OHCI1394_AsReqRcvContextControlClear  0x1C4
+#define OHCI1394_AsReqRcvCommandPtr           0x1CC
+
+#define OHCI1394_AsRspRcvContextBase          0x1E0
+#define OHCI1394_AsRspRcvContextControlSet    0x1E0
+#define OHCI1394_AsRspRcvContextControlClear  0x1E4
+#define OHCI1394_AsRspRcvCommandPtr           0x1EC
+
+/* Isochronous transmit registers */
+#define OHCI1394_IsoXmitContextBase(n)           (0x200 + 16 * (n))
+#define OHCI1394_IsoXmitContextControlSet(n)     (0x200 + 16 * (n))
+#define OHCI1394_IsoXmitContextControlClear(n)   (0x204 + 16 * (n))
+#define OHCI1394_IsoXmitCommandPtr(n)            (0x20C + 16 * (n))
+
+/* Isochronous receive registers */
+#define OHCI1394_IsoRcvContextControlSet(n)   (0x400 + 32 * (n))
+#define OHCI1394_IsoRcvContextControlClear(n) (0x404 + 32 * (n))
+#define OHCI1394_IsoRcvCommandPtr(n)          (0x40C + 32 * (n))
+#define OHCI1394_IsoRcvContextMatch(n)        (0x410 + 32 * (n))
+
+/* Interrupts Mask/Events */
+#define OHCI1394_reqTxComplete           0x00000001
+#define OHCI1394_respTxComplete          0x00000002
+#define OHCI1394_ARRQ                    0x00000004
+#define OHCI1394_ARRS                    0x00000008
+#define OHCI1394_RQPkt                   0x00000010
+#define OHCI1394_RSPkt                   0x00000020
+#define OHCI1394_isochTx                 0x00000040
+#define OHCI1394_isochRx                 0x00000080
+#define OHCI1394_postedWriteErr          0x00000100
+#define OHCI1394_lockRespErr             0x00000200
+#define OHCI1394_selfIDComplete          0x00010000
+#define OHCI1394_busReset                0x00020000
+#define OHCI1394_phy                     0x00080000
+#define OHCI1394_cycleSynch              0x00100000
+#define OHCI1394_cycle64Seconds          0x00200000
+#define OHCI1394_cycleLost               0x00400000
+#define OHCI1394_cycleInconsistent       0x00800000
+#define OHCI1394_unrecoverableError      0x01000000
+#define OHCI1394_cycleTooLong            0x02000000
+#define OHCI1394_phyRegRcvd              0x04000000
+#define OHCI1394_masterIntEnable         0x80000000
+
+#define OHCI1394_evt_no_status		0x0
+#define OHCI1394_evt_long_packet	0x2
+#define OHCI1394_evt_missing_ack	0x3
+#define OHCI1394_evt_underrun		0x4
+#define OHCI1394_evt_overrun		0x5
+#define OHCI1394_evt_descriptor_read	0x6
+#define OHCI1394_evt_data_read		0x7
+#define OHCI1394_evt_data_write		0x8
+#define OHCI1394_evt_bus_reset		0x9
+#define OHCI1394_evt_timeout		0xa
+#define OHCI1394_evt_tcode_err		0xb
+#define OHCI1394_evt_reserved_b		0xc
+#define OHCI1394_evt_reserved_c		0xd
+#define OHCI1394_evt_unknown		0xe
+#define OHCI1394_evt_flushed		0xf
+
+#define OHCI1394_phy_tcode		0xe
+
+#endif /* __fw_ohci_h */

Re: [PATCH 2/3] Import fw-ohci driver.

[Pete Zaitcev]

On Tue, 05 Dec 2006 00:22:45 -0500, "Kristian Høgsberg" <krh@redhat.com> wrote:

Your wonderful crossed-o might be ok here (or it would've been if only
your mailer worked -- notice that the name is right in the "On" tag
line above):

> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---

But it's very much NOT OK here:

> + * fw-ohci.c - Driver for OHCI 1394 boards
> + * Copyright (C) 2003 Kristian Høgsberg <krh@bitplanet.net>

And you know why? Because the C code in kernel has no character set.

Even if the holy penguin declares "we use UTF-8 now", it's still not ok,
because people routinely send patches in e-mail.

So, please don't do that. I do not put "Copyright (c) 2006 Петр Зайцев"
in my patches out of courtesy to you. You might not even have fonts
for that. Now I expect the same from you.

-- Pete

Re: [PATCH 2/3] Import fw-ohci driver.

[Jeff Garzik]

Kristian Høgsberg wrote:
> Add the OHCI driver to the stack and build system.
> 
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
> 
>  drivers/fw/fw-ohci.c | 1334 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/fw/fw-ohci.h |  152 ++++++
>  2 files changed, 1486 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/fw/fw-ohci.c b/drivers/fw/fw-ohci.c
> new file mode 100644
> index 0000000..78e0324
> --- /dev/null
> +++ b/drivers/fw/fw-ohci.c
> @@ -0,0 +1,1334 @@
> +/*						-*- c-basic-offset: 8 -*-
> + *
> + * fw-ohci.c - Driver for OHCI 1394 boards
> + * Copyright (C) 2003 Kristian Høgsberg <krh@bitplanet.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +#include <linux/poll.h>
> +#include <asm/uaccess.h>
> +#include <asm/semaphore.h>
> +
> +#include "fw-transaction.h"
> +#include "fw-ohci.h"
> +
> +struct descriptor {
> +	u32 req_count:16;
> +
> +	u32 wait:2;
> +	u32 branch:2;
> +	u32 irq:2;
> +	u32 yy:1;
> +	u32 ping:1;
> +
> +	u32 key:3;
> +	u32 status:1;
> +	u32 command:4;
> +
> +	u32 data_address;
> +	u32 branch_address;
> +
> +	u32 res_count:16;
> +	u32 transfer_status:16;
> +} __attribute__ ((aligned(16)));

you probably want __le32 annotations for sparse, right?


> +#define DESCRIPTOR_OUTPUT_MORE		0
> +#define DESCRIPTOR_OUTPUT_LAST		1
> +#define DESCRIPTOR_INPUT_MORE		2
> +#define DESCRIPTOR_INPUT_LAST		3
> +#define DESCRIPTOR_NO_IRQ		0
> +#define DESCRIPTOR_IRQ_ERROR		1
> +#define DESCRIPTOR_IRQ_ALWAYS		3
> +#define DESCRIPTOR_KEY_IMMEDIATE	2
> +#define DESCRIPTOR_BRANCH_ALWAYS	3
> +
> +struct ar_context {
> +	struct fw_ohci *ohci;
> +	struct descriptor descriptor;
> +	u32 buffer[512];
> +	dma_addr_t descriptor_bus;
> +	dma_addr_t buffer_bus;
> +
> +	u32 command_ptr;
> +	u32 control_set;
> +	u32 control_clear;
> +
> +	struct tasklet_struct tasklet;
> +};
> +
> +struct at_context {
> +	struct fw_ohci *ohci;
> +	dma_addr_t descriptor_bus;
> +	dma_addr_t buffer_bus;
> +
> +	struct list_head list;
> +
> +	struct {
> +		struct descriptor more;
> +		u32 header[4];
> +		struct descriptor last;
> +	} descriptor;
> +
> +	u32 command_ptr;
> +	u32 control_set;
> +	u32 control_clear;
> +
> +	struct tasklet_struct tasklet;
> +};
> +
> +struct it_header {
> +	u32 sy:4;
> +	u32 tcode:4;
> +	u32 channel:6;
> +	u32 tag:2;
> +	u32 speed:3;
> +	u32 reserved0:13;
> +	u32 reserved1:16;
> +	u32 data_length:16;
> +};

ditto.

And for the last two fields, I bet that using the normal 'u16' type 
(__le16?) would generate much better code than a bitfield:16 ever would.


> +struct iso_context {
> +	struct fw_iso_context base;
> +	struct tasklet_struct tasklet;
> +	u32 control_set;
> +	u32 control_clear;
> +	u32 command_ptr;
> +	u32 context_match;
> +
> +	struct descriptor *buffer;
> +	dma_addr_t buffer_bus;
> +	struct descriptor *head_descriptor;
> +	struct descriptor *tail_descriptor;
> +	struct descriptor *tail_descriptor_last;
> +	struct descriptor *prev_descriptor;
> +};
> +
> +#define CONFIG_ROM_SIZE 1024
> +
> +struct fw_ohci {
> +	struct fw_card card;
> +
> +	struct pci_dev *dev;

struct device* instead?  grep for to_pci_dev() to see how to get pci-dev 
from device.


> +	char *registers;

should be 'void __iomem *'

See Documentation/sparse.txt for more info on checking your code with 
sparse.


> +	dma_addr_t self_id_bus;
> +	u32 *self_id_cpu;
> +	struct tasklet_struct bus_reset_tasklet;
> +	int generation;
> +	int request_generation;
> +
> +	spinlock_t lock;
> +	u32 self_id_buffer[512];
> +
> +	/* Config rom buffers */
> +	u32 *config_rom;
> +	dma_addr_t config_rom_bus;
> +	u32 *next_config_rom;
> +	dma_addr_t next_config_rom_bus;
> +
> +	struct ar_context ar_request_ctx;
> +	struct ar_context ar_response_ctx;
> +	struct at_context at_request_ctx;
> +	struct at_context at_response_ctx;
> +
> +	u32 it_context_mask;
> +	struct iso_context *it_context_list;
> +	u32 ir_context_mask;
> +	struct iso_context *ir_context_list;
> +};
> +
> +extern inline struct fw_ohci *fw_ohci(struct fw_card *card)
> +{
> +	return container_of(card, struct fw_ohci, card);
> +}
> +
> +#define CONTEXT_CYCLE_MATCH_ENABLE	0x80000000
> +
> +#define CONTEXT_RUN	0x8000
> +#define CONTEXT_WAKE	0x1000
> +#define CONTEXT_DEAD	0x0800
> +#define CONTEXT_ACTIVE	0x0400
> +
> +#define OHCI1394_MAX_AT_REQ_RETRIES	0x2
> +#define OHCI1394_MAX_AT_RESP_RETRIES	0x2
> +#define OHCI1394_MAX_PHYS_RESP_RETRIES	0x8
> +
> +#define FW_OHCI_MAJOR			240
> +#define OHCI1394_REGISTER_SIZE		0x800
> +#define OHCI_LOOP_COUNT			500
> +#define OHCI1394_PCI_HCI_Control	0x40
> +#define SELF_ID_BUF_SIZE		0x800

consider using

	enum {
		constant1	= 1234,
		constant2	= 5678,
	};

for constants.  These actually have some type information attached by 
the compiler, and they show up as symbols in the debugger since they are 
not stripped out by the C pre-processor.


> +#define PCI_CLASS_FIREWIRE_OHCI ((PCI_CLASS_SERIAL_FIREWIRE << 8) | 0x10)
> +
> +static char ohci_driver_name[] = KBUILD_MODNAME;
> +
> +extern inline void reg_write(const struct fw_ohci *ohci, int offset, u32 data)
> +{
> +	writel(data, ohci->registers + offset);
> +}
> +
> +extern inline u32 reg_read(const struct fw_ohci *ohci, int offset)
> +{
> +	return readl(ohci->registers + offset);
> +}
> +
> +static int read_phy_reg(struct fw_ohci *ohci, u8 addr)
> +{
> +	u32 val;
> +
> +	reg_write(ohci, OHCI1394_PhyControl, OHCI1394_PhyControl_Read(addr));
> +	msleep(2);
> +	val = reg_read(ohci, OHCI1394_PhyControl);
> +	if ((val & OHCI1394_PhyControl_ReadDone) == 0)
> +		return -EBUSY;
> +
> +	return OHCI1394_PhyControl_ReadData(val);
> +}
> +
> +static void write_phy_reg(struct fw_ohci *ohci, u8 addr, u8 data)
> +{
> +	reg_write(ohci, OHCI1394_PhyControl,
> +		  OHCI1394_PhyControl_Write(addr, data));
> +}
> +
> +static int
> +ohci_update_phy_reg(struct fw_card *card, int addr,
> +		    int clear_bits, int set_bits)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +	int old;
> +
> +	old = read_phy_reg(ohci, addr);
> +	if (old < 0) {
> +		fw_error("failed to set phy reg bits.\n");
> +		return old;
> +	}
> +	old = (old & ~clear_bits) | set_bits;
> +	write_phy_reg(ohci, addr, old);
> +
> +	return 0;
> +}
> +
> +static void ar_context_run(struct ar_context *ctx)
> +{
> +	reg_write(ctx->ohci, ctx->command_ptr, ctx->descriptor_bus | 1);
> +	reg_write(ctx->ohci, ctx->control_set, CONTEXT_RUN);

PCI posting?


> +static void ar_context_tasklet(unsigned long data)
> +{
> +	struct ar_context *ctx = (struct ar_context *)data;
> +	struct fw_ohci *ohci = ctx->ohci;
> +	u32 status;
> +	int length, speed, ack, timestamp;
> +
> +	/* FIXME: What to do about evt_* errors? */
> +	length    = ctx->descriptor.req_count - ctx->descriptor.res_count - 4;
> +	status    = ctx->buffer[length / 4];
> +	ack       = ((status >> 16) & 0x1f) - 16;
> +	speed     = (status >> 21) & 0x7;
> +	timestamp = status & 0xffff;
> +
> +	/* The OHCI bus reset handler synthesizes a phy packet with
> +	 * the new generation number when a bus reset happens (see
> +	 * section 8.4.2.3).  This helps us determine when a request
> +	 * was received and make sure we send the response in the same
> +	 * generation.  We only need this for requests; for responses
> +	 * we use the unique tlabel for finding the matching
> +	 * request. */
> +	if (ack + 16 == 0x09)
> +		ohci->request_generation = (ctx->buffer[2] >> 16) & 0xff;
> +	else if (ctx == &ohci->ar_request_ctx)
> +		fw_core_handle_request(&ohci->card, speed, ack, timestamp,
> +				       ohci->request_generation,
> +				       length, ctx->buffer);
> +	else
> +		fw_core_handle_response(&ohci->card, speed, ack, timestamp,
> +					length, ctx->buffer);
> +
> +	ctx->descriptor.data_address = ctx->buffer_bus;
> +	ctx->descriptor.req_count = sizeof ctx->buffer;
> +	ctx->descriptor.res_count = ctx->descriptor.req_count;
> +
> +	dma_sync_single_for_device(&ohci->dev->dev, ctx->descriptor_bus,
> +				   sizeof ctx->descriptor_bus, DMA_TO_DEVICE);
> +
> +	/* FIXME: We stop and restart the ar context here, what if we
> +	 * stop while a receive is in progress? Maybe we could just
> +	 * loop the context back to itself and use it in buffer fill
> +	 * mode as intended... */
> +
> +	reg_write(ctx->ohci, ctx->control_clear, CONTEXT_RUN);
> +	ar_context_run(ctx);
> +}
> +
> +static int
> +ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci, u32 control_set)
> +{
> +	/* FIXME: cpu_to_le32. */
> +
> +	ctx->descriptor_bus =
> +		dma_map_single(&ohci->dev->dev, &ctx->descriptor,
> +			       sizeof ctx->descriptor, PCI_DMA_TODEVICE);
> +	if (ctx->descriptor_bus == 0)
> +		return -ENOMEM;
> +
> +	if (ctx->descriptor_bus & 0xf)
> +		fw_notify("descriptor not 16-byte aligned: 0x%08x\n",
> +			  ctx->descriptor_bus);
> +
> +	ctx->buffer_bus =
> +		dma_map_single(&ohci->dev->dev, ctx->buffer,
> +			       sizeof ctx->buffer, PCI_DMA_FROMDEVICE);
> +
> +	if (ctx->buffer_bus == 0)
> +		return -ENOMEM;

leak on error


> +	memset(&ctx->descriptor, 0, sizeof ctx->descriptor);
> +	ctx->descriptor.command      = DESCRIPTOR_INPUT_MORE;
> +	ctx->descriptor.status       = 1;
> +	ctx->descriptor.irq          = DESCRIPTOR_NO_IRQ;
> +	ctx->descriptor.branch       = DESCRIPTOR_BRANCH_ALWAYS;
> +	ctx->descriptor.req_count    = sizeof ctx->buffer;
> +	ctx->descriptor.data_address = ctx->buffer_bus;
> +	ctx->descriptor.res_count    = ctx->descriptor.req_count;
> +
> +	ctx->control_set   = control_set;
> +	ctx->control_clear = control_set + 4;
> +	ctx->command_ptr   = control_set + 12;
> +	ctx->ohci          = ohci;
> +
> +	tasklet_init(&ctx->tasklet, ar_context_tasklet, (unsigned long)ctx);
> +
> +	ar_context_run(ctx);
> +
> +	return 0;
> +}
> +
> +		ohci->next_config_rom = NULL;
> +	}
> +
> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> +	fw_core_handle_bus_reset(&ohci->card, node_id, generation,
> +				 self_id_count, ohci->self_id_buffer);
> +}
> +
> +static irqreturn_t irq_handler(int irq, void *data, struct pt_regs *unused)
> +{
> +	struct fw_ohci *ohci = data;
> +	u32 event, iso_event;
> +	int i;
> +
> +	event = reg_read(ohci, OHCI1394_IntEventClear);
> +
> +	if (!event)
> +		return IRQ_NONE;

check for 0xffffffff also


> +	reg_write(ohci, OHCI1394_IntEventClear, event);
> +
> +	if (event & OHCI1394_selfIDComplete)
> +		tasklet_schedule(&ohci->bus_reset_tasklet);
> +
> +	if (event & OHCI1394_RQPkt)
> +		tasklet_schedule(&ohci->ar_request_ctx.tasklet);
> +
> +	if (event & OHCI1394_RSPkt)
> +		tasklet_schedule(&ohci->ar_response_ctx.tasklet);
> +
> +	if (event & OHCI1394_reqTxComplete)
> +		tasklet_schedule(&ohci->at_request_ctx.tasklet);
> +
> +	if (event & OHCI1394_respTxComplete)
> +		tasklet_schedule(&ohci->at_response_ctx.tasklet);
> +
> +	iso_event = reg_read(ohci, OHCI1394_IsoRecvIntEventSet);
> +	reg_write(ohci, OHCI1394_IsoRecvIntEventClear, iso_event);
> +
> +	while (iso_event) {
> +		i = ffs(iso_event) - 1;
> +		tasklet_schedule(&ohci->ir_context_list[i].tasklet);
> +		iso_event &= ~(1 << i);
> +	}
> +
> +	iso_event = reg_read(ohci, OHCI1394_IsoXmitIntEventSet);
> +	reg_write(ohci, OHCI1394_IsoXmitIntEventClear, iso_event);
> +
> +	while (iso_event) {
> +		i = ffs(iso_event) - 1;
> +		tasklet_schedule(&ohci->it_context_list[i].tasklet);
> +		iso_event &= ~(1 << i);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ohci_enable(struct fw_card *card, u32 * config_rom, size_t length)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +
> +	/* When the link is not yet enabled, the atomic config rom
> +	 * described above is not active.  We have to update
> +	 * ConfigRomHeader and BusOptions manually, and the write to
> +	 * ConfigROMmap takes effect immediately.  We tie this to the
> +	 * enabling of the link, so we ensure that we have a valid
> +	 * config rom before enabling - the OHCI requires that
> +	 * ConfigROMhdr and BusOptions have valid values before
> +	 * enabling.
> +	 */
> +
> +	ohci->config_rom = pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
> +						&ohci->config_rom_bus);
> +	if (ohci->config_rom == NULL)
> +		return -ENOMEM;
> +
> +	memset(ohci->config_rom, 0, CONFIG_ROM_SIZE);
> +	fw_memcpy_to_be32(ohci->config_rom, config_rom, length * 4);
> +
> +	reg_write(ohci, OHCI1394_ConfigROMmap, ohci->config_rom_bus);
> +	reg_write(ohci, OHCI1394_ConfigROMhdr, config_rom[0]);
> +	reg_write(ohci, OHCI1394_BusOptions, config_rom[2]);
> +
> +	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
> +
> +	if (request_irq(ohci->dev->irq, irq_handler,
> +			SA_SHIRQ, ohci_driver_name, ohci)) {
> +		fw_error("Failed to allocate shared interrupt %d.\n",
> +			 ohci->dev->irq);
> +		return -EIO;

leak on error


> +	reg_write(ohci, OHCI1394_HCControlSet,
> +		  OHCI1394_HCControl_linkEnable |
> +		  OHCI1394_HCControl_BIBimageValid);

PCI posting


> +	/* We are ready to go, initiate bus reset to finish the
> +	 * initialization. */
> +
> +	fw_core_initiate_bus_reset(&ohci->card, 1);
> +
> +	return 0;
> +}
> +
> +static int
> +ohci_set_config_rom(struct fw_card *card, u32 * config_rom, size_t length)
> +{
> +	struct fw_ohci *ohci;
> +	unsigned long flags;
> +	int retval = 0;
> +
> +	ohci = fw_ohci(card);
> +
> +	/* When the OHCI controller is enabled, the config rom update
> +	 * mechanism is a bit tricky, but easy enough to use.  See
> +	 * section 5.5.6 in the OHCI specification.
> +	 *
> +	 * The OHCI controller caches the new config rom address in a
> +	 * shadow register (ConfigROMmapNext) and needs a bus reset
> +	 * for the changes to take place.  When the bus reset is
> +	 * detected, the controller loads the new values for the
> +	 * ConfigRomHeader and BusOptions registers from the specified
> +	 * config rom and loads ConfigROMmap from the ConfigROMmapNext
> +	 * shadow register. All automatically and atomically.
> +	 *
> +	 * We use ohci->lock to avoid racing with the code that sets
> +	 * ohci->next_config_rom to NULL (see bus_reset_tasklet).
> +	 */
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +
> +	if (ohci->next_config_rom == NULL) {
> +		ohci->next_config_rom =
> +			pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
> +					     &ohci->next_config_rom_bus);
> +
> +		memset(ohci->next_config_rom, 0, CONFIG_ROM_SIZE);

next_config_rom could be NULL.  you have got to check allocations inside 
spinlocks (GFP_ATOMIC), they are more likely to fail than GFP_KERNEL.


> +		fw_memcpy_to_be32(ohci->next_config_rom, config_rom,
> +				  length * 4);
> +		reg_write(ohci, OHCI1394_ConfigROMmap,
> +			  ohci->next_config_rom_bus);
> +
> +		/* Now initiate a bus reset to have the changes take
> +		 * effect. We clean up the old config rom memory and
> +		 * DMA mappings in the bus reset tasklet, since the
> +		 * OHCI controller could need to access it before the
> +		 * bus reset takes effect.
> +		 */
> +
> +		fw_core_initiate_bus_reset(&ohci->card, 1);
> +	} else
> +		retval = -EBUSY;
> +
> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> +	return retval;
> +}
> +
> +static void ohci_send_request(struct fw_card *card, struct fw_packet *packet)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +
> +	at_context_transmit(&ohci->at_request_ctx, packet);
> +}
> +
> +static void ohci_send_response(struct fw_card *card, struct fw_packet *packet)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +
> +	at_context_transmit(&ohci->at_response_ctx, packet);
> +}
> +
> +static int
> +ohci_enable_phys_dma(struct fw_card *card, int node_id, int generation)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +	unsigned long flags;
> +	int retval = 0;
> +
> +	/* FIXME: make sure this bitmask is cleared when we clear the
> +	 * busReset interrupt bit. */
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +
> +	if (ohci->generation != generation) {
> +		retval = -ESTALE;
> +		goto out;
> +	}
> +
> +	if (node_id < 32) {
> +		reg_write(ohci, OHCI1394_PhyReqFilterLoSet, 1 << node_id);
> +	} else {
> +		reg_write(ohci, OHCI1394_PhyReqFilterHiSet,
> +			  1 << (node_id - 32));
> +	}

PCI posting


> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> + out:
> +	return retval;
> +}
> +
> +static void ir_context_tasklet(unsigned long data)
> +{
> +	struct iso_context *ctx = (struct iso_context *)data;
> +
> +	(void)ctx;
> +}
> +
> +#define ISO_BUFFER_SIZE (64 * 1024)
> +
> +static void flush_iso_context(struct iso_context *ctx)
> +{
> +	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
> +	struct descriptor *d, *last;
> +	int z;
> +
> +	dma_sync_single_for_cpu(ohci->card.device, ctx->buffer_bus,
> +				ISO_BUFFER_SIZE, DMA_TO_DEVICE);
> +
> +	d    = ctx->tail_descriptor;
> +	last = ctx->tail_descriptor_last;
> +
> +	while (last->branch_address != 0 && last->transfer_status != 0) {
> +		z = last->branch_address & 0xf;
> +		d = ctx->buffer + (last->branch_address -
> +				   ctx->buffer_bus) / sizeof *d;
> +
> +		if (z == 2)
> +			last = d;
> +		else
> +			last = d + z - 1;
> +
> +		if (last->irq)
> +			ctx->base.callback(&ctx->base,
> +					   0, last->res_count,
> +					   ctx->base.callback_data);
> +	}
> +
> +	ctx->tail_descriptor      = d;
> +	ctx->tail_descriptor_last = last;
> +}
> +
> +static void it_context_tasklet(unsigned long data)
> +{
> +	struct iso_context *ctx = (struct iso_context *)data;
> +
> +	flush_iso_context(ctx);
> +}
> +
> +static struct fw_iso_context *ohci_allocate_iso_context(struct fw_card *card,
> +							int type)
> +{
> +	struct fw_ohci *ohci = fw_ohci(card);
> +	struct iso_context *ctx, *list;
> +	void (*tasklet) (unsigned long data);
> +	u32 *mask;
> +	unsigned long flags;
> +	int index;
> +
> +	if (type == FW_ISO_CONTEXT_TRANSMIT) {
> +		mask = &ohci->it_context_mask;
> +		list = ohci->it_context_list;
> +		tasklet = it_context_tasklet;
> +	} else {
> +		mask = &ohci->ir_context_mask;
> +		list = ohci->ir_context_list;
> +		tasklet = ir_context_tasklet;
> +	}
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +	index = ffs(*mask) - 1;
> +	if (index >= 0)
> +		*mask &= ~(1 << index);
> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> +	if (index < 0)
> +		return ERR_PTR(-EBUSY);
> +
> +	ctx = &list[index];
> +	memset(ctx, 0, sizeof *ctx);
> +	tasklet_init(&ctx->tasklet, tasklet, (unsigned long)ctx);
> +
> +	ctx->buffer = kmalloc(ISO_BUFFER_SIZE, GFP_KERNEL);
> +	if (ctx->buffer == NULL) {
> +		spin_lock_irqsave(&ohci->lock, flags);
> +		*mask |= 1 << index;
> +		spin_unlock_irqrestore(&ohci->lock, flags);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	ctx->buffer_bus =
> +	    dma_map_single(card->device, ctx->buffer,
> +			   ISO_BUFFER_SIZE, PCI_DMA_TODEVICE);
> +
> +	ctx->head_descriptor      = ctx->buffer;
> +	ctx->prev_descriptor      = ctx->buffer;
> +	ctx->tail_descriptor      = ctx->buffer;
> +	ctx->tail_descriptor_last = ctx->buffer;
> +
> +	/* We put a dummy descriptor in the buffer that has a NULL
> +	 * branch address and looks like it's been sent.  That way we
> +	 * have a descriptor to append DMA programs to.  Also, the
> +	 * ring buffer invariant is that it always has at least one
> +	 * element so that head == tail means buffer full. */
> +
> +	memset(ctx->head_descriptor, 0, sizeof *ctx->head_descriptor);
> +	ctx->head_descriptor->command         = DESCRIPTOR_OUTPUT_LAST;
> +	ctx->head_descriptor->transfer_status = 0x8011;
> +	ctx->head_descriptor++;
> +
> +	return &ctx->base;
> +}
> +
> +static int ohci_send_iso(struct fw_iso_context *base, s32 cycle)
> +{
> +	struct iso_context *ctx = (struct iso_context *)base;
> +	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
> +	u32 cycle_match = 0;
> +	int index;
> +
> +	index = ctx - ohci->it_context_list;
> +	if (cycle > 0)
> +		cycle_match = CONTEXT_CYCLE_MATCH_ENABLE |
> +			(cycle & 0x7fff) << 16;
> +
> +	reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, 1 << index);
> +	reg_write(ohci, OHCI1394_IsoXmitCommandPtr(index),
> +		  ctx->tail_descriptor_last->branch_address);
> +	reg_write(ohci, OHCI1394_IsoXmitContextControlClear(index), ~0);
> +	reg_write(ohci, OHCI1394_IsoXmitContextControlSet(index),
> +		  CONTEXT_RUN | cycle_match);

PCI posting?


> +static void ohci_free_iso_context(struct fw_iso_context *base)
> +{
> +	struct fw_ohci *ohci = fw_ohci(base->card);
> +	struct iso_context *ctx = (struct iso_context *)base;
> +	unsigned long flags;
> +	int index;
> +
> +	flush_iso_context(ctx);
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +
> +	if (ctx->base.type == FW_ISO_CONTEXT_TRANSMIT) {
> +		index = ctx - ohci->it_context_list;
> +		reg_write(ohci, OHCI1394_IsoXmitContextControlClear(index), ~0);
> +		reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, 1 << index);
> +		ohci->it_context_mask |= 1 << index;
> +	} else {
> +		index = ctx - ohci->ir_context_list;
> +		reg_write(ohci, OHCI1394_IsoRcvContextControlClear(index), ~0);
> +		reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, 1 << index);
> +		ohci->ir_context_mask |= 1 << index;
> +	}
> +
> +	dma_unmap_single(ohci->card.device, ctx->buffer_bus,
> +			 ISO_BUFFER_SIZE, DMA_TO_DEVICE);

PCI posting?


> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +}
> +
> +static int
> +ohci_queue_iso(struct fw_iso_context *base,
> +	       struct fw_iso_packet *packet, void *payload)
> +{
> +	struct iso_context *ctx = (struct iso_context *)base;
> +	struct fw_ohci *ohci = fw_ohci(ctx->base.card);
> +	struct descriptor *d, *end, *last, *tail, *pd;
> +	struct fw_iso_packet *p;
> +	struct it_header *header;
> +	dma_addr_t d_bus;
> +	u32 z, header_z, payload_z;
> +	u32 payload_index, payload_end_index, next_page_index;
> +	int index, page, end_page, i, length, offset;
> +
> +	/* FIXME: Cycle lost behavior should be configurable: lose
> +	 * packet, retransmit or terminate.. */
> +
> +	p = packet;
> +	payload_index = payload - ctx->base.buffer;
> +	d = ctx->head_descriptor;
> +	tail = ctx->tail_descriptor;
> +	end = ctx->buffer + ISO_BUFFER_SIZE / sizeof(struct descriptor);
> +
> +	if (p->skip)
> +		z = 1;
> +	else
> +		z = 2;
> +	if (p->header_length > 0)
> +		z++;
> +
> +	/* Determine the first page the payload isn't contained in. */
> +	end_page = PAGE_ALIGN(payload_index + p->payload_length) >> PAGE_SHIFT;
> +	if (p->payload_length > 0)
> +		payload_z = end_page - (payload_index >> PAGE_SHIFT);
> +	else
> +		payload_z = 0;
> +
> +	z += payload_z;
> +
> +	/* Get header size in number of descriptors. */
> +	header_z = DIV_ROUND_UP(p->header_length, sizeof *d);
> +
> +	if (d + z + header_z <= tail) {
> +		goto has_space;
> +	} else if (d > tail && d + z + header_z <= end) {
> +		goto has_space;
> +	} else if (d > tail && ctx->buffer + z + header_z <= tail) {
> +		d = ctx->buffer;
> +		goto has_space;
> +	}
> +
> +	/* No space in buffer */
> +	return -1;
> +
> + has_space:
> +	memset(d, 0, (z + header_z) * sizeof *d);
> +	d_bus = ctx->buffer_bus + (d - ctx->buffer) * sizeof *d;
> +
> +	if (p->skip) {
> +		d[0].key = 0;
> +		d[0].req_count = 0;
> +	} else {
> +		d[0].key = DESCRIPTOR_KEY_IMMEDIATE;
> +		d[0].req_count = 8;
> +
> +		header = (struct it_header *)&d[1];
> +		header->sy          = p->sy;
> +		header->tag         = p->tag;
> +		header->tcode       = TCODE_STREAM_DATA;
> +		header->channel     = ctx->base.channel;
> +		header->speed       = ctx->base.speed;
> +		header->data_length = p->header_length + p->payload_length;
> +	}
> +
> +	if (p->header_length > 0) {
> +		d[2].req_count    = p->header_length;
> +		d[2].data_address = d_bus + z * sizeof *d;
> +		memcpy(&d[z], p->header, p->header_length);
> +	}
> +
> +	pd = d + z - payload_z;
> +	payload_end_index = payload_index + p->payload_length;
> +	for (i = 0; i < payload_z; i++) {
> +		page               = payload_index >> PAGE_SHIFT;
> +		offset             = payload_index & ~PAGE_MASK;
> +		next_page_index    = (page + 1) << PAGE_SHIFT;
> +		length             =
> +			min(next_page_index, payload_end_index) - payload_index;
> +		pd[i].req_count    = length;
> +		pd[i].data_address = ctx->base.pages[page] + offset;
> +
> +		payload_index += length;
> +	}
> +
> +	if (z == 2)
> +		last = d;
> +	else
> +		last = d + z - 1;
> +
> +	last->command = DESCRIPTOR_OUTPUT_LAST;
> +	last->status  = 1;
> +	last->branch  = DESCRIPTOR_BRANCH_ALWAYS;
> +	if (p->interrupt)
> +		last->irq = DESCRIPTOR_IRQ_ALWAYS;
> +	else
> +		last->irq = DESCRIPTOR_NO_IRQ;
> +
> +	dma_sync_single_for_device(ohci->card.device, ctx->buffer_bus,
> +				   ISO_BUFFER_SIZE, DMA_TO_DEVICE);
> +
> +	ctx->head_descriptor = d + z + header_z;
> +	ctx->prev_descriptor->branch_address = d_bus | z;
> +	ctx->prev_descriptor = last;
> +
> +	index = ctx - ohci->it_context_list;
> +	reg_write(ohci, OHCI1394_IsoXmitContextControlSet(index), CONTEXT_WAKE);

PCI posting?


> +static struct fw_card_driver ohci_driver = {
> +	.name			= ohci_driver_name,
> +	.enable			= ohci_enable,
> +	.update_phy_reg		= ohci_update_phy_reg,
> +	.set_config_rom		= ohci_set_config_rom,
> +	.send_request		= ohci_send_request,
> +	.send_response		= ohci_send_response,
> +	.enable_phys_dma	= ohci_enable_phys_dma,
> +
> +	.allocate_iso_context	= ohci_allocate_iso_context,
> +	.free_iso_context	= ohci_free_iso_context,
> +	.queue_iso		= ohci_queue_iso,
> +	.send_iso		= ohci_send_iso
> +};
> +
> +static int software_reset(struct fw_ohci *ohci)
> +{
> +	int i;
> +
> +	reg_write(ohci, OHCI1394_HCControlSet, OHCI1394_HCControl_softReset);
> +
> +	for (i = 0; i < OHCI_LOOP_COUNT; i++) {
> +		if ((reg_read(ohci, OHCI1394_HCControlSet) &
> +		     OHCI1394_HCControl_softReset) == 0)
> +			return 0;
> +		msleep(1);
> +	}
> +
> +	return -EBUSY;
> +}
> +
> +/* ---------- pci subsystem interface ---------- */
> +
> +enum {
> +	CLEANUP_IRQ,
> +	CLEANUP_SELF_ID,
> +	CLEANUP_REGISTERS,
> +	CLEANUP_IOMEM,
> +	CLEANUP_OHCI
> +};
> +
> +static int cleanup(struct fw_ohci *ohci, int stage, int code)
> +{
> +	switch (stage) {
> +	case CLEANUP_SELF_ID:
> +		pci_free_consistent(ohci->dev, SELF_ID_BUF_SIZE,
> +				    ohci->self_id_cpu, ohci->self_id_bus);
> +	case CLEANUP_REGISTERS:
> +		kfree(ohci->it_context_list);
> +		kfree(ohci->ir_context_list);
> +		iounmap(ohci->registers);
> +	case CLEANUP_IOMEM:
> +		release_mem_region(pci_resource_start(ohci->dev, 0),
> +				   OHCI1394_REGISTER_SIZE);
> +	case CLEANUP_OHCI:
> +		fw_card_put(&ohci->card);
> +	}
> +
> +	return code;
> +}
> +
> +static int __devinit
> +pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
> +{
> +	struct fw_ohci *ohci;
> +	u32 base, bus_options, max_receive, link_speed;
> +	u64 guid;
> +	int error_code;
> +	size_t size;
> +
> +	if (pci_enable_device(dev)) {
> +		fw_error("Failed to enable OHCI hardware.\n");
> +		return -ENXIO;
> +	}
> +
> +	ohci = kzalloc(sizeof *ohci, SLAB_KERNEL);

GFP_KERNEL


> +	if (ohci == NULL) {
> +		fw_error("Could not malloc fw_ohci data.\n");
> +		return -ENOMEM;
> +	}
> +
> +	pci_set_master(dev);
> +	pci_write_config_dword(dev, OHCI1394_PCI_HCI_Control, 0);
> +	pci_set_drvdata(dev, ohci);
> +
> +	ohci->dev = dev;
> +	spin_lock_init(&ohci->lock);
> +
> +	tasklet_init(&ohci->bus_reset_tasklet,
> +		     bus_reset_tasklet, (unsigned long)ohci);
> +
> +	/* We hardcode the register set size, since some cards get
> +	 * this wrong and others have some extra registers after the
> +	 * OHCI range.  We only use the OHCI registers, so there's no
> +	 * need to be clever.  */
> +	base = pci_resource_start(dev, 0);
> +	if (!request_mem_region(base, OHCI1394_REGISTER_SIZE, ohci_driver_name)) {

ugh!  use pci_request_regions(), not request_mem_region()


> +		fw_error("MMIO resource (0x%x - 0x%x) unavailable\n",
> +			 base, base + OHCI1394_REGISTER_SIZE);
> +		return cleanup(ohci, CLEANUP_OHCI, -EBUSY);
> +	}
> +
> +	ohci->registers = ioremap(base, OHCI1394_REGISTER_SIZE);
> +	if (ohci->registers == NULL) {
> +		fw_error("Failed to remap registers\n");
> +		return cleanup(ohci, CLEANUP_IOMEM, -ENXIO);
> +	}

pci_iomap() does a lot of this


> +	if (software_reset(ohci)) {
> +		fw_error("Failed to reset ohci card.\n");
> +		return cleanup(ohci, CLEANUP_REGISTERS, -EBUSY);
> +	}
> +
> +	/* Now enable LPS, which we need in order to start accessing
> +	 * most of the registers.  In fact, on some cards (ALI M5251),
> +	 * accessing registers in the SClk domain without LPS enabled
> +	 * will lock up the machine.  Wait 50msec to make sure we have
> +	 * full link enabled.  */
> +	reg_write(ohci, OHCI1394_HCControlSet,
> +		  OHCI1394_HCControl_LPS |
> +		  OHCI1394_HCControl_postedWriteEnable);
> +	msleep(50);

PCI posting


> +	/* self-id dma buffer allocation */
> +	ohci->self_id_cpu = pci_alloc_consistent(ohci->dev, SELF_ID_BUF_SIZE,
> +						 &ohci->self_id_bus);
> +	if (ohci->self_id_cpu == NULL) {
> +		fw_error("Out of memory for self ID buffer.\n");
> +		return cleanup(ohci, CLEANUP_REGISTERS, -ENOMEM);
> +	}
> +	reg_write(ohci, OHCI1394_SelfIDBuffer, ohci->self_id_bus);
> +	reg_write(ohci, OHCI1394_LinkControlSet,
> +		  OHCI1394_LinkControl_rcvSelfID |
> +		  OHCI1394_LinkControl_cycleTimerEnable |
> +		  OHCI1394_LinkControl_cycleMaster);
> +
> +	ar_context_init(&ohci->ar_request_ctx, ohci,
> +			OHCI1394_AsReqRcvContextControlSet);
> +
> +	ar_context_init(&ohci->ar_response_ctx, ohci,
> +			OHCI1394_AsRspRcvContextControlSet);
> +
> +	at_context_init(&ohci->at_request_ctx, ohci,
> +			OHCI1394_AsReqTrContextControlSet);
> +
> +	at_context_init(&ohci->at_response_ctx, ohci,
> +			OHCI1394_AsRspTrContextControlSet);
> +
> +	reg_write(ohci, OHCI1394_ATRetries,
> +		  OHCI1394_MAX_AT_REQ_RETRIES |
> +		  (OHCI1394_MAX_AT_RESP_RETRIES << 4) |
> +		  (OHCI1394_MAX_PHYS_RESP_RETRIES << 8));
> +
> +	reg_write(ohci, OHCI1394_IsoRecvIntMaskSet, ~0);
> +	ohci->it_context_mask = reg_read(ohci, OHCI1394_IsoRecvIntMaskSet);
> +	reg_write(ohci, OHCI1394_IsoRecvIntMaskClear, ~0);
> +	size = sizeof(struct iso_context) * hweight32(ohci->it_context_mask);
> +	ohci->it_context_list = kzalloc(size, GFP_KERNEL);
> +
> +	reg_write(ohci, OHCI1394_IsoXmitIntMaskSet, ~0);
> +	ohci->ir_context_mask = reg_read(ohci, OHCI1394_IsoXmitIntMaskSet);
> +	reg_write(ohci, OHCI1394_IsoXmitIntMaskClear, ~0);
> +	size = sizeof(struct iso_context) * hweight32(ohci->ir_context_mask);
> +	ohci->ir_context_list = kzalloc(size, GFP_KERNEL);
> +
> +	if (ohci->it_context_list == NULL || ohci->ir_context_list == NULL) {
> +		fw_error("Out of memory for it/ir contexts.\n");
> +		return cleanup(ohci, CLEANUP_REGISTERS, -ENOMEM);
> +	}
> +
> +	reg_write(ohci, OHCI1394_PhyUpperBound, 0x00010000);
> +	reg_write(ohci, OHCI1394_IntEventClear, ~0);
> +	reg_write(ohci, OHCI1394_IntMaskClear, ~0);
> +	reg_write(ohci, OHCI1394_IntMaskSet,
> +		  OHCI1394_selfIDComplete |
> +		  OHCI1394_RQPkt | OHCI1394_RSPkt |
> +		  OHCI1394_reqTxComplete | OHCI1394_respTxComplete |
> +		  OHCI1394_isochRx | OHCI1394_isochTx |
> +		  OHCI1394_masterIntEnable);
> +
> +	bus_options = reg_read(ohci, OHCI1394_BusOptions);
> +	max_receive = (bus_options >> 12) & 0xf;
> +	link_speed = bus_options & 0x7;
> +	guid = (__u64) reg_read(ohci, OHCI1394_GUIDHi) << 32 |
> +		reg_read(ohci, OHCI1394_GUIDLo);
> +
> +	error_code = fw_core_add_card(&ohci->card, &ohci_driver, &dev->dev,
> +				      max_receive, link_speed, guid);
> +	if (error_code < 0)
> +		return cleanup(ohci, CLEANUP_SELF_ID, error_code);
> +
> +	fw_notify("Added fw-ohci device %s.\n", dev->dev.bus_id);
> +
> +	return 0;
> +}
> +
> +static void pci_remove(struct pci_dev *dev)
> +{
> +	struct fw_ohci *ohci;
> +
> +	ohci = pci_get_drvdata(dev);
> +	if (ohci == NULL)
> +		return;

test for event that will never occur


> +	free_irq(ohci->dev->irq, ohci);
> +	fw_core_remove_card(&ohci->card);
> +
> +	/* FIXME: Fail all pending packets here, now that the upper
> +	 * layers can't queue any more. */
> +
> +	software_reset(ohci);
> +	cleanup(ohci, CLEANUP_SELF_ID, 0);
> +
> +	fw_notify("Removed fw-ohci device.\n");

need to call pci_disable_device(), pci_release_regions()



> +static struct pci_device_id pci_table[] = {
> +	{
> +		.class		= PCI_CLASS_FIREWIRE_OHCI,
> +		.class_mask	= PCI_ANY_ID,

I'm not sure this is a proper class_mask?


> +		.vendor		= PCI_ANY_ID,
> +		.device		= PCI_ANY_ID,
> +		.subvendor	= PCI_ANY_ID,
> +		.subdevice	= PCI_ANY_ID,
> +	},
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, pci_table);
> +
> +static struct pci_driver fw_ohci_pci_driver = {
> +	.name		= ohci_driver_name,
> +	.id_table	= pci_table,
> +	.probe		= pci_probe,
> +	.remove		= pci_remove,
> +};
> +
> +MODULE_AUTHOR("Kristian Høgsberg <krh@bitplanet.net>");
> +MODULE_DESCRIPTION("Driver for PCI OHCI IEEE1394 controllers");
> +MODULE_LICENSE("GPL");
> +
> +static int __init fw_ohci_init(void)
> +{
> +	if (pci_register_driver(&fw_ohci_pci_driver)) {
> +		fw_error("Failed to register PCI driver.\n");
> +		return -EBUSY;
> +	}
> +
> +	fw_notify("Loaded fw-ohci driver.\n");
> +
> +	return 0;

just return pci_register_driver() return value directly, don't invent 
your own error when pci_register_driver() already returned a 
more-correct error code



> +static void __exit fw_ohci_cleanup(void)
> +{
> +	pci_unregister_driver(&fw_ohci_pci_driver);
> +	fw_notify("Unloaded fw-ohci driver.\n");
> +}
> +
> +module_init(fw_ohci_init);
> +module_exit(fw_ohci_cleanup);
> diff --git a/drivers/fw/fw-ohci.h b/drivers/fw/fw-ohci.h
> new file mode 100644
> index 0000000..e200b1b
> --- /dev/null
> +++ b/drivers/fw/fw-ohci.h
> @@ -0,0 +1,152 @@
> +#ifndef __fw_ohci_h
> +#define __fw_ohci_h
> +
> +/* OHCI register map */
> +
> +#define OHCI1394_Version                      0x000
> +#define OHCI1394_GUID_ROM                     0x004
> +#define OHCI1394_ATRetries                    0x008
> +#define OHCI1394_CSRData                      0x00C
> +#define OHCI1394_CSRCompareData               0x010
> +#define OHCI1394_CSRControl                   0x014
> +#define OHCI1394_ConfigROMhdr                 0x018
> +#define OHCI1394_BusID                        0x01C
> +#define OHCI1394_BusOptions                   0x020
> +#define OHCI1394_GUIDHi                       0x024
> +#define OHCI1394_GUIDLo                       0x028
> +#define OHCI1394_ConfigROMmap                 0x034
> +#define OHCI1394_PostedWriteAddressLo         0x038
> +#define OHCI1394_PostedWriteAddressHi         0x03C
> +#define OHCI1394_VendorID                     0x040
> +#define OHCI1394_HCControlSet                 0x050
> +#define OHCI1394_HCControlClear               0x054
> +#define  OHCI1394_HCControl_BIBimageValid	0x80000000
> +#define  OHCI1394_HCControl_noByteSwap		0x40000000
> +#define  OHCI1394_HCControl_programPhyEnable	0x00800000
> +#define  OHCI1394_HCControl_aPhyEnhanceEnable	0x00400000
> +#define  OHCI1394_HCControl_LPS			0x00080000
> +#define  OHCI1394_HCControl_postedWriteEnable	0x00040000
> +#define  OHCI1394_HCControl_linkEnable		0x00020000
> +#define  OHCI1394_HCControl_softReset		0x00010000
> +#define OHCI1394_SelfIDBuffer                 0x064
> +#define OHCI1394_SelfIDCount                  0x068
> +#define OHCI1394_IRMultiChanMaskHiSet         0x070
> +#define OHCI1394_IRMultiChanMaskHiClear       0x074
> +#define OHCI1394_IRMultiChanMaskLoSet         0x078
> +#define OHCI1394_IRMultiChanMaskLoClear       0x07C
> +#define OHCI1394_IntEventSet                  0x080
> +#define OHCI1394_IntEventClear                0x084
> +#define OHCI1394_IntMaskSet                   0x088
> +#define OHCI1394_IntMaskClear                 0x08C
> +#define OHCI1394_IsoXmitIntEventSet           0x090
> +#define OHCI1394_IsoXmitIntEventClear         0x094
> +#define OHCI1394_IsoXmitIntMaskSet            0x098
> +#define OHCI1394_IsoXmitIntMaskClear          0x09C
> +#define OHCI1394_IsoRecvIntEventSet           0x0A0
> +#define OHCI1394_IsoRecvIntEventClear         0x0A4
> +#define OHCI1394_IsoRecvIntMaskSet            0x0A8
> +#define OHCI1394_IsoRecvIntMaskClear          0x0AC
> +#define OHCI1394_InitialBandwidthAvailable    0x0B0
> +#define OHCI1394_InitialChannelsAvailableHi   0x0B4
> +#define OHCI1394_InitialChannelsAvailableLo   0x0B8
> +#define OHCI1394_FairnessControl              0x0DC
> +#define OHCI1394_LinkControlSet               0x0E0
> +#define OHCI1394_LinkControlClear             0x0E4
> +#define   OHCI1394_LinkControl_rcvSelfID	(1 << 9)
> +#define   OHCI1394_LinkControl_rcvPhyPkt	(1 << 10)
> +#define   OHCI1394_LinkControl_cycleTimerEnable	(1 << 20)
> +#define   OHCI1394_LinkControl_cycleMaster	(1 << 21)
> +#define   OHCI1394_LinkControl_cycleSource	(1 << 22)
> +#define OHCI1394_NodeID                       0x0E8
> +#define   OHCI1394_NodeID_idValid             0x80000000
> +#define OHCI1394_PhyControl                   0x0EC
> +#define   OHCI1394_PhyControl_Read(addr)	(((addr) << 8) | 0x00008000)
> +#define   OHCI1394_PhyControl_ReadDone		0x80000000
> +#define   OHCI1394_PhyControl_ReadData(r)	(((r) & 0x00ff0000) >> 16)
> +#define   OHCI1394_PhyControl_Write(addr, data) (((addr) << 8) | (data) | 0x00004000)
> +#define   OHCI1394_PhyControl_WriteDone		0x00004000
> +#define OHCI1394_IsochronousCycleTimer        0x0F0
> +#define OHCI1394_AsReqFilterHiSet             0x100
> +#define OHCI1394_AsReqFilterHiClear           0x104
> +#define OHCI1394_AsReqFilterLoSet             0x108
> +#define OHCI1394_AsReqFilterLoClear           0x10C
> +#define OHCI1394_PhyReqFilterHiSet            0x110
> +#define OHCI1394_PhyReqFilterHiClear          0x114
> +#define OHCI1394_PhyReqFilterLoSet            0x118
> +#define OHCI1394_PhyReqFilterLoClear          0x11C
> +#define OHCI1394_PhyUpperBound                0x120
> +
> +#define OHCI1394_AsReqTrContextBase           0x180
> +#define OHCI1394_AsReqTrContextControlSet     0x180
> +#define OHCI1394_AsReqTrContextControlClear   0x184
> +#define OHCI1394_AsReqTrCommandPtr            0x18C
> +
> +#define OHCI1394_AsRspTrContextBase           0x1A0
> +#define OHCI1394_AsRspTrContextControlSet     0x1A0
> +#define OHCI1394_AsRspTrContextControlClear   0x1A4
> +#define OHCI1394_AsRspTrCommandPtr            0x1AC
> +
> +#define OHCI1394_AsReqRcvContextBase          0x1C0
> +#define OHCI1394_AsReqRcvContextControlSet    0x1C0
> +#define OHCI1394_AsReqRcvContextControlClear  0x1C4
> +#define OHCI1394_AsReqRcvCommandPtr           0x1CC
> +
> +#define OHCI1394_AsRspRcvContextBase          0x1E0
> +#define OHCI1394_AsRspRcvContextControlSet    0x1E0
> +#define OHCI1394_AsRspRcvContextControlClear  0x1E4
> +#define OHCI1394_AsRspRcvCommandPtr           0x1EC
> +
> +/* Isochronous transmit registers */
> +#define OHCI1394_IsoXmitContextBase(n)           (0x200 + 16 * (n))
> +#define OHCI1394_IsoXmitContextControlSet(n)     (0x200 + 16 * (n))
> +#define OHCI1394_IsoXmitContextControlClear(n)   (0x204 + 16 * (n))
> +#define OHCI1394_IsoXmitCommandPtr(n)            (0x20C + 16 * (n))
> +
> +/* Isochronous receive registers */
> +#define OHCI1394_IsoRcvContextControlSet(n)   (0x400 + 32 * (n))
> +#define OHCI1394_IsoRcvContextControlClear(n) (0x404 + 32 * (n))
> +#define OHCI1394_IsoRcvCommandPtr(n)          (0x40C + 32 * (n))
> +#define OHCI1394_IsoRcvContextMatch(n)        (0x410 + 32 * (n))
> +
> +/* Interrupts Mask/Events */
> +#define OHCI1394_reqTxComplete           0x00000001
> +#define OHCI1394_respTxComplete          0x00000002
> +#define OHCI1394_ARRQ                    0x00000004
> +#define OHCI1394_ARRS                    0x00000008
> +#define OHCI1394_RQPkt                   0x00000010
> +#define OHCI1394_RSPkt                   0x00000020
> +#define OHCI1394_isochTx                 0x00000040
> +#define OHCI1394_isochRx                 0x00000080
> +#define OHCI1394_postedWriteErr          0x00000100
> +#define OHCI1394_lockRespErr             0x00000200
> +#define OHCI1394_selfIDComplete          0x00010000
> +#define OHCI1394_busReset                0x00020000
> +#define OHCI1394_phy                     0x00080000
> +#define OHCI1394_cycleSynch              0x00100000
> +#define OHCI1394_cycle64Seconds          0x00200000
> +#define OHCI1394_cycleLost               0x00400000
> +#define OHCI1394_cycleInconsistent       0x00800000
> +#define OHCI1394_unrecoverableError      0x01000000
> +#define OHCI1394_cycleTooLong            0x02000000
> +#define OHCI1394_phyRegRcvd              0x04000000
> +#define OHCI1394_masterIntEnable         0x80000000
> +
> +#define OHCI1394_evt_no_status		0x0
> +#define OHCI1394_evt_long_packet	0x2
> +#define OHCI1394_evt_missing_ack	0x3
> +#define OHCI1394_evt_underrun		0x4
> +#define OHCI1394_evt_overrun		0x5
> +#define OHCI1394_evt_descriptor_read	0x6
> +#define OHCI1394_evt_data_read		0x7
> +#define OHCI1394_evt_data_write		0x8
> +#define OHCI1394_evt_bus_reset		0x9
> +#define OHCI1394_evt_timeout		0xa
> +#define OHCI1394_evt_tcode_err		0xb
> +#define OHCI1394_evt_reserved_b		0xc
> +#define OHCI1394_evt_reserved_c		0xd
> +#define OHCI1394_evt_unknown		0xe
> +#define OHCI1394_evt_flushed		0xf
> +
> +#define OHCI1394_phy_tcode		0xe

consider enum{} for reasons mentioned above

Re: [PATCH 2/3] Import fw-ohci driver.

[Benjamin Herrenschmidt]

> > +struct descriptor {
> > +	u32 req_count:16;
> > +
> > +	u32 wait:2;
> > +	u32 branch:2;
> > +	u32 irq:2;
> > +	u32 yy:1;
> > +	u32 ping:1;
> > +
> > +	u32 key:3;
> > +	u32 status:1;
> > +	u32 command:4;
> > +
> > +	u32 data_address;
> > +	u32 branch_address;
> > +
> > +	u32 res_count:16;
> > +	u32 transfer_status:16;
> > +} __attribute__ ((aligned(16)));
> 
> you probably want __le32 annotations for sparse, right?

More than that... he wants no bitfields ! Right now, this code is broken
on some endians (I suspect big, dunno on what Kristian tested).

> 
> And for the last two fields, I bet that using the normal 'u16' type 
> (__le16?) would generate much better code than a bitfield:16 ever would.

Bah, it's endian broken anyway due to bitfield (ab)use.

> 	enum {
> 		constant1	= 1234,
> 		constant2	= 5678,
> 	};
> 
> for constants.  These actually have some type information attached by 
> the compiler, and they show up as symbols in the debugger since they are 
> not stripped out by the C pre-processor.

Note that while this is true for small constants, beware of the fact
that this is highly unrecommended for anything that doesn't fit in a
signed int. gcc has some dodgy extensions allowing non-int enums but you
don't want to go near them. Read a recent discussion with Linus & Viro
(a few days ago iirc) on lkml about it.

Since some of his constants have values up to 0x80000000, I'm not 100%
confident enum is the way to go, but if you are careful with sign, it
could still be.

> > +static void ar_context_run(struct ar_context *ctx)
> > +{
> > +	reg_write(ctx->ohci, ctx->command_ptr, ctx->descriptor_bus | 1);
> > +	reg_write(ctx->ohci, ctx->control_set, CONTEXT_RUN);
> 
> PCI posting?

In that specific case (kicking the context), it doesn't matter much.
It's not a bug per-se not to do it, though you might get better
performances by making sure it's kicked right away.

Ben.

Re: [PATCH 2/3] Import fw-ohci driver.

[Kristian Høgsberg]

Jeff Garzik wrote:

Thanks for reviewing this.  I'm updating my git repo with your changes now, 
will send an updated patch set in a few days.

>> +struct descriptor {
>> +    u32 req_count:16;
>> +
>> +    u32 wait:2;
>> +    u32 branch:2;
>> +    u32 irq:2;
>> +    u32 yy:1;
>> +    u32 ping:1;
>> +
>> +    u32 key:3;
>> +    u32 status:1;
>> +    u32 command:4;
>> +
>> +    u32 data_address;
>> +    u32 branch_address;
>> +
>> +    u32 res_count:16;
>> +    u32 transfer_status:16;
>> +} __attribute__ ((aligned(16)));
> 
> you probably want __le32 annotations for sparse, right?

Yup, I've done away with the bitfields and switched to a mix of __le16 and 
__le32 struct fields.

>> +struct it_header {
>> +    u32 sy:4;
>> +    u32 tcode:4;
>> +    u32 channel:6;
>> +    u32 tag:2;
>> +    u32 speed:3;
>> +    u32 reserved0:13;
>> +    u32 reserved1:16;
>> +    u32 data_length:16;
>> +};
> 
> ditto.
> 
> And for the last two fields, I bet that using the normal 'u16' type 
> (__le16?) would generate much better code than a bitfield:16 ever would.

Yeah, as for struct descriptor, this is now just accessed as an __le32 array.

>> +struct fw_ohci {
>> +    struct fw_card card;
>> +
>> +    struct pci_dev *dev;
> 
> struct device* instead?  grep for to_pci_dev() to see how to get pci-dev 
> from device.

Right, and I have the struct device pointer in struct fw_card so I just 
dropped this field.

>> +    char *registers;
> 
> should be 'void __iomem *'

Ok.

>> +#define FW_OHCI_MAJOR            240
>> +#define OHCI1394_REGISTER_SIZE        0x800
>> +#define OHCI_LOOP_COUNT            500
>> +#define OHCI1394_PCI_HCI_Control    0x40
>> +#define SELF_ID_BUF_SIZE        0x800
> 
> consider using
> 
>     enum {
>         constant1    = 1234,
>         constant2    = 5678,
>     };
> 
> for constants.  These actually have some type information attached by 
> the compiler, and they show up as symbols in the debugger since they are 
> not stripped out by the C pre-processor.

All those are basically one-off constants, so maybe a static const u32 would 
be better?  As for the OHCI register map, I'd like to make a struct with a 
bunch of __le32 fields.

>> +static void ar_context_run(struct ar_context *ctx)
>> +{
>> +    reg_write(ctx->ohci, ctx->command_ptr, ctx->descriptor_bus | 1);
>> +    reg_write(ctx->ohci, ctx->control_set, CONTEXT_RUN);
> 
> PCI posting?

Added here, and the other places you pointed out.

>> +static int
>> +ar_context_init(struct ar_context *ctx, struct fw_ohci *ohci, u32 
>> control_set)
>> +{
>> +    /* FIXME: cpu_to_le32. */
>> +
>> +    ctx->descriptor_bus =
>> +        dma_map_single(&ohci->dev->dev, &ctx->descriptor,
>> +                   sizeof ctx->descriptor, PCI_DMA_TODEVICE);
>> +    if (ctx->descriptor_bus == 0)
>> +        return -ENOMEM;
>> +
>> +    if (ctx->descriptor_bus & 0xf)
>> +        fw_notify("descriptor not 16-byte aligned: 0x%08x\n",
>> +              ctx->descriptor_bus);
>> +
>> +    ctx->buffer_bus =
>> +        dma_map_single(&ohci->dev->dev, ctx->buffer,
>> +                   sizeof ctx->buffer, PCI_DMA_FROMDEVICE);
>> +
>> +    if (ctx->buffer_bus == 0)
>> +        return -ENOMEM;
> 
> leak on error

Fixed.

>> +static irqreturn_t irq_handler(int irq, void *data, struct pt_regs 
>> *unused)
>> +{
>> +    struct fw_ohci *ohci = data;
>> +    u32 event, iso_event;
>> +    int i;
>> +
>> +    event = reg_read(ohci, OHCI1394_IntEventClear);
>> +
>> +    if (!event)
>> +        return IRQ_NONE;
> 
> check for 0xffffffff also

Ok.
...
>> +static int ohci_enable(struct fw_card *card, u32 * config_rom, size_t 
>> length)
>> +{
>> +    struct fw_ohci *ohci = fw_ohci(card);
>> +
>> +    /* When the link is not yet enabled, the atomic config rom
>> +     * described above is not active.  We have to update
>> +     * ConfigRomHeader and BusOptions manually, and the write to
>> +     * ConfigROMmap takes effect immediately.  We tie this to the
>> +     * enabling of the link, so we ensure that we have a valid
>> +     * config rom before enabling - the OHCI requires that
>> +     * ConfigROMhdr and BusOptions have valid values before
>> +     * enabling.
>> +     */
>> +
>> +    ohci->config_rom = pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
>> +                        &ohci->config_rom_bus);
>> +    if (ohci->config_rom == NULL)
>> +        return -ENOMEM;
>> +
>> +    memset(ohci->config_rom, 0, CONFIG_ROM_SIZE);
>> +    fw_memcpy_to_be32(ohci->config_rom, config_rom, length * 4);
>> +
>> +    reg_write(ohci, OHCI1394_ConfigROMmap, ohci->config_rom_bus);
>> +    reg_write(ohci, OHCI1394_ConfigROMhdr, config_rom[0]);
>> +    reg_write(ohci, OHCI1394_BusOptions, config_rom[2]);
>> +
>> +    reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
>> +
>> +    if (request_irq(ohci->dev->irq, irq_handler,
>> +            SA_SHIRQ, ohci_driver_name, ohci)) {
>> +        fw_error("Failed to allocate shared interrupt %d.\n",
>> +             ohci->dev->irq);
>> +        return -EIO;
> 
> leak on error

Fixed.

>> +static int
>> +ohci_set_config_rom(struct fw_card *card, u32 * config_rom, size_t 
>> length)
>> +{
>> +    struct fw_ohci *ohci;
>> +    unsigned long flags;
>> +    int retval = 0;
>> +
>> +    ohci = fw_ohci(card);
>> +
>> +    /* When the OHCI controller is enabled, the config rom update
>> +     * mechanism is a bit tricky, but easy enough to use.  See
>> +     * section 5.5.6 in the OHCI specification.
>> +     *
>> +     * The OHCI controller caches the new config rom address in a
>> +     * shadow register (ConfigROMmapNext) and needs a bus reset
>> +     * for the changes to take place.  When the bus reset is
>> +     * detected, the controller loads the new values for the
>> +     * ConfigRomHeader and BusOptions registers from the specified
>> +     * config rom and loads ConfigROMmap from the ConfigROMmapNext
>> +     * shadow register. All automatically and atomically.
>> +     *
>> +     * We use ohci->lock to avoid racing with the code that sets
>> +     * ohci->next_config_rom to NULL (see bus_reset_tasklet).
>> +     */
>> +
>> +    spin_lock_irqsave(&ohci->lock, flags);
>> +
>> +    if (ohci->next_config_rom == NULL) {
>> +        ohci->next_config_rom =
>> +            pci_alloc_consistent(ohci->dev, CONFIG_ROM_SIZE,
>> +                         &ohci->next_config_rom_bus);
>> +
>> +        memset(ohci->next_config_rom, 0, CONFIG_ROM_SIZE);
> 
> next_config_rom could be NULL.  you have got to check allocations inside 
> spinlocks (GFP_ATOMIC), they are more likely to fail than GFP_KERNEL.

Yup, fixed.  For some reason this one slipped my attention.  I moved the alloc 
outside the spinlock so I don't have to alloc GFP_ATOMC and to ease error 
handling.

>> +static int __devinit
>> +pci_probe(struct pci_dev *dev, const struct pci_device_id *ent)
>> +{
>> +    struct fw_ohci *ohci;
>> +    u32 base, bus_options, max_receive, link_speed;
>> +    u64 guid;
>> +    int error_code;
>> +    size_t size;
>> +
>> +    if (pci_enable_device(dev)) {
>> +        fw_error("Failed to enable OHCI hardware.\n");
>> +        return -ENXIO;
>> +    }
>> +
>> +    ohci = kzalloc(sizeof *ohci, SLAB_KERNEL);
> 
> GFP_KERNEL

Yup.

>> +    if (ohci == NULL) {
>> +        fw_error("Could not malloc fw_ohci data.\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    pci_set_master(dev);
>> +    pci_write_config_dword(dev, OHCI1394_PCI_HCI_Control, 0);
>> +    pci_set_drvdata(dev, ohci);
>> +
>> +    ohci->dev = dev;
>> +    spin_lock_init(&ohci->lock);
>> +
>> +    tasklet_init(&ohci->bus_reset_tasklet,
>> +             bus_reset_tasklet, (unsigned long)ohci);
>> +
>> +    /* We hardcode the register set size, since some cards get
>> +     * this wrong and others have some extra registers after the
>> +     * OHCI range.  We only use the OHCI registers, so there's no
>> +     * need to be clever.  */
>> +    base = pci_resource_start(dev, 0);
>> +    if (!request_mem_region(base, OHCI1394_REGISTER_SIZE, 
>> ohci_driver_name)) {
> 
> ugh!  use pci_request_regions(), not request_mem_region()

Fixed.

>> +        fw_error("MMIO resource (0x%x - 0x%x) unavailable\n",
>> +             base, base + OHCI1394_REGISTER_SIZE);
>> +        return cleanup(ohci, CLEANUP_OHCI, -EBUSY);
>> +    }
>> +
>> +    ohci->registers = ioremap(base, OHCI1394_REGISTER_SIZE);
>> +    if (ohci->registers == NULL) {
>> +        fw_error("Failed to remap registers\n");
>> +        return cleanup(ohci, CLEANUP_IOMEM, -ENXIO);
>> +    }
> 
> pci_iomap() does a lot of this

Oh yeah, nice.

>> +static void pci_remove(struct pci_dev *dev)
>> +{
>> +    struct fw_ohci *ohci;
>> +
>> +    ohci = pci_get_drvdata(dev);
>> +    if (ohci == NULL)
>> +        return;
> 
> test for event that will never occur

True, that's pretty stupid.

>> +    free_irq(ohci->dev->irq, ohci);
>> +    fw_core_remove_card(&ohci->card);
>> +
>> +    /* FIXME: Fail all pending packets here, now that the upper
>> +     * layers can't queue any more. */
>> +
>> +    software_reset(ohci);
>> +    cleanup(ohci, CLEANUP_SELF_ID, 0);
>> +
>> +    fw_notify("Removed fw-ohci device.\n");
> 
> need to call pci_disable_device(), pci_release_regions()

Yup, added that.

>> +static struct pci_device_id pci_table[] = {
>> +    {
>> +        .class        = PCI_CLASS_FIREWIRE_OHCI,
>> +        .class_mask    = PCI_ANY_ID,
> 
> I'm not sure this is a proper class_mask?

Huh, yeah... looks like ~0 should work.  And I changed this to use the 
PCI_DEVICE_CLASS macro.

>> +        .vendor        = PCI_ANY_ID,
>> +        .device        = PCI_ANY_ID,
>> +        .subvendor    = PCI_ANY_ID,
>> +        .subdevice    = PCI_ANY_ID,
>> +    },
>> +    { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, pci_table);
>> +
>> +static struct pci_driver fw_ohci_pci_driver = {
>> +    .name        = ohci_driver_name,
>> +    .id_table    = pci_table,
>> +    .probe        = pci_probe,
>> +    .remove        = pci_remove,
>> +};
>> +
>> +MODULE_AUTHOR("Kristian Høgsberg <krh@bitplanet.net>");
>> +MODULE_DESCRIPTION("Driver for PCI OHCI IEEE1394 controllers");
>> +MODULE_LICENSE("GPL");
>> +
>> +static int __init fw_ohci_init(void)
>> +{
>> +    if (pci_register_driver(&fw_ohci_pci_driver)) {
>> +        fw_error("Failed to register PCI driver.\n");
>> +        return -EBUSY;
>> +    }
>> +
>> +    fw_notify("Loaded fw-ohci driver.\n");
>> +
>> +    return 0;
> 
> just return pci_register_driver() return value directly, don't invent 
> your own error when pci_register_driver() already returned a 
> more-correct error code

Ok.

Re: [PATCH 2/3] Import fw-ohci driver.

[Stefan Richter]

Kristian Høgsberg wrote:
> Yup, I've done away with the bitfields and switched to a mix of __le16
> and __le32 struct fields.

I suppose the struct should get __attribute__((packed)) then.

But is the order of two adjacent __le16 fields (i.e. two halves of a
quadlet) independent of host byte order?
-- 
Stefan Richter
-=====-=-==- ==-- -=--=
http://arcgraph.de/sr/

Re: [PATCH 2/3] Import fw-ohci driver.

[Kristian Høgsberg]

Stefan Richter wrote:
> Kristian Høgsberg wrote:
>> Yup, I've done away with the bitfields and switched to a mix of __le16
>> and __le32 struct fields.
> 
> I suppose the struct should get __attribute__((packed)) then.

I guess it wouldn't harm, but is it really necessary?  Would gcc ever insert 
padding here, all the 32 bit fields a 32 bit aligned, and so are the 16 bit 
fields.

> But is the order of two adjacent __le16 fields (i.e. two halves of a
> quadlet) independent of host byte order?

Yeah, it works for both be and le cpus.  The layout is le specific, which is 
how the host controller wants it.

cheers,
Kristian

Re: [PATCH 2/3] Import fw-ohci driver.

[Stefan Richter]

Kristian Høgsberg wrote:
> Stefan Richter wrote:
>> Kristian Høgsberg wrote:
>>> Yup, I've done away with the bitfields and switched to a mix of __le16
>>> and __le32 struct fields.
>>
>> I suppose the struct should get __attribute__((packed)) then.
> 
> I guess it wouldn't harm, but is it really necessary?  Would gcc ever
> insert padding here, all the 32 bit fields a 32 bit aligned, and so are
> the 16 bit fields.

Is 2-byte alignment of 16bit struct members guaranteed on all platforms?

A related question:
If I specify a struct which, among else, contains 32bit quantities, then
any variable of this struct type is supposed to be at least 4-byte-aligned.
No if I specifiy this struct as packed, will variables of this type still
be aligned on 4 byte boundaries or will the compiler assume no alignment?
In other words, should it be __attribute__((packed,aligned(4))) then?
I'm speaking about situations where I not only wish to avoid unnecessarily
bad machine code due to unaligned access but where the device requires
4-byte alignment too.
-- 
Stefan Richter
-=====-=-==- ==-- -=-=-
http://arcgraph.de/sr/

alignment and packing of struct types (was Re: [PATCH 2/3] Import fw-ohci driver.)

[Stefan Richter]

I wrote:
> Kristian Høgsberg wrote:
>> Stefan Richter wrote:
>>> Kristian Høgsberg wrote:
>>>> Yup, I've done away with the bitfields and switched to a mix of __le16
>>>> and __le32 struct fields.
>>> I suppose the struct should get __attribute__((packed)) then.
>> I guess it wouldn't harm, but is it really necessary?  Would gcc ever
>> insert padding here, all the 32 bit fields a 32 bit aligned, and so are
>> the 16 bit fields.
> 
> Is 2-byte alignment of 16bit struct members guaranteed on all platforms?
> 
> A related question:
> If I specify a struct which, among else, contains 32bit quantities, then
> any variable of this struct type is supposed to be at least 4-byte-aligned.
> No if I specifiy this struct as packed, will variables of this type still
> be aligned on 4 byte boundaries or will the compiler assume no alignment?
> In other words, should it be __attribute__((packed,aligned(4))) then?
> I'm speaking about situations where I not only wish to avoid unnecessarily
> bad machine code due to unaligned access but where the device requires
> 4-byte alignment too.

After sending this, I realized I should have changed the subject. :-)
-- 
Stefan Richter
-=====-=-==- ==-- -=-=-
http://arcgraph.de/sr/

[PATCH 3/3] Import fw-sbp2 driver.

[Kristian Høgsberg]

Pull in the fw-sbp2 driver for firewire storage devices.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---

 drivers/fw/fw-ohci.c |    2 
 drivers/fw/fw-sbp2.c | 1083 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 1084 insertions(+), 1 deletions(-)

diff --git a/drivers/fw/fw-ohci.c b/drivers/fw/fw-ohci.c
index 78e0324..444e8f0 100644
--- a/drivers/fw/fw-ohci.c
+++ b/drivers/fw/fw-ohci.c
@@ -594,7 +594,7 @@ static void bus_reset_tasklet(unsigned l
 				 self_id_count, ohci->self_id_buffer);
 }
 
-static irqreturn_t irq_handler(int irq, void *data, struct pt_regs *unused)
+static irqreturn_t irq_handler(int irq, void *data)
 {
 	struct fw_ohci *ohci = data;
 	u32 event, iso_event;
diff --git a/drivers/fw/fw-sbp2.c b/drivers/fw/fw-sbp2.c
new file mode 100644
index 0000000..e0e7590
--- /dev/null
+++ b/drivers/fw/fw-sbp2.c
@@ -0,0 +1,1083 @@
+/*						-*- c-basic-offset: 8 -*-
+ * fw-sbp2.c -- SBP2 driver (SCSI over IEEE1394)
+ *
+ * Copyright © 2005  Kristian Høgsberg <krh@bitplanet.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+
+#include "fw-transaction.h"
+#include "fw-topology.h"
+#include "fw-device.h"
+
+/* I don't know why the SCSI stack doesn't define something like this... */
+typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);
+
+static const char sbp2_driver_name[] = "sbp2";
+
+struct sbp2_device {
+	struct fw_address_handler address_handler;
+	struct list_head orb_list;
+	u64 management_agent_address;
+	u64 command_block_agent_address;
+	u32 workarounds;
+	int login_id;
+
+	/* We cache these addresses and only update them once we've
+	 * logged in or reconnected to the sbp2 device.  That way, any
+	 * IO to the device will automatically fail and get retried if
+	 * it happens in a window where the device is not ready to
+	 * handle it (e.g. after a bus reset but before we reconnect). */
+	int node_id;
+	int address_high;
+	int generation;
+
+	struct work_struct work;
+	struct Scsi_Host *scsi_host;
+};
+
+#define SBP2_MAX_SG_ELEMENT_LENGTH	0xf000
+#define SBP2_MAX_SECTORS		255	/* Max sectors supported */
+#define SBP2_MAX_CMDS			8	/* This should be safe */
+
+#define SBP2_ORB_NULL			0x80000000
+
+#define SBP2_DIRECTION_TO_MEDIA		0x0
+#define SBP2_DIRECTION_FROM_MEDIA	0x1
+
+/* Unit directory keys */
+#define SBP2_COMMAND_SET_SPECIFIER	0x38
+#define SBP2_COMMAND_SET		0x39
+#define SBP2_COMMAND_SET_REVISION	0x3b
+#define SBP2_FIRMWARE_REVISION		0x3c
+
+/* Flags for detected oddities and brokeness */
+#define SBP2_WORKAROUND_128K_MAX_TRANS	0x1
+#define SBP2_WORKAROUND_INQUIRY_36	0x2
+#define SBP2_WORKAROUND_MODE_SENSE_8	0x4
+#define SBP2_WORKAROUND_FIX_CAPACITY	0x8
+#define SBP2_WORKAROUND_OVERRIDE	0x100
+
+/* Management orb opcodes */
+#define SBP2_LOGIN_REQUEST		0x0
+#define SBP2_QUERY_LOGINS_REQUEST	0x1
+#define SBP2_RECONNECT_REQUEST		0x3
+#define SBP2_SET_PASSWORD_REQUEST	0x4
+#define SBP2_LOGOUT_REQUEST		0x7
+#define SBP2_ABORT_TASK_REQUEST		0xb
+#define SBP2_ABORT_TASK_SET		0xc
+#define SBP2_LOGICAL_UNIT_RESET		0xe
+#define SBP2_TARGET_RESET_REQUEST	0xf
+
+/* Offsets for command block agent registers */
+#define SBP2_AGENT_STATE		0x00
+#define SBP2_AGENT_RESET		0x04
+#define SBP2_ORB_POINTER		0x08
+#define SBP2_DOORBELL			0x10
+#define SBP2_UNSOLICITED_STATUS_ENABLE	0x14
+
+/* Status write response codes */
+#define SBP2_STATUS_REQUEST_COMPLETE	0x0
+#define SBP2_STATUS_TRANSPORT_FAILURE	0x1
+#define SBP2_STATUS_ILLEGAL_REQUEST	0x2
+#define SBP2_STATUS_VENDOR_DEPENDENT	0x3
+
+struct sbp2_status {
+	unsigned int orb_high:16;
+	unsigned int sbp_status:8;
+	unsigned int len:3;
+	unsigned int dead:1;
+	unsigned int response:2;
+	unsigned int source:2;
+	u32 orb_low;
+	u8 data[24];
+};
+
+struct sbp2_orb {
+	struct fw_transaction t;
+	dma_addr_t request_bus;
+	int rcode;
+	u32 pointer[2];
+	void (*callback) (struct sbp2_orb * orb, struct sbp2_status * status);
+	struct list_head link;
+};
+
+struct sbp2_management_orb {
+	struct sbp2_orb base;
+	struct {
+		u32 password_high;
+		u32 password_low;
+		u32 response_high;
+		u32 response_low;
+		unsigned int lun:16;
+		unsigned int function:4;
+		unsigned int reconnect:4;
+		unsigned int reserved:4;
+		unsigned int exclusive:1;
+		unsigned int request_format:2;
+		unsigned int notify:1;
+		unsigned int response_length:16;
+		unsigned int password_length:16;
+		u32 status_fifo_high;
+		u32 status_fifo_low;
+	} request;
+	u32 response[4];
+	dma_addr_t response_bus;
+	struct completion done;
+	struct sbp2_status status;
+};
+
+struct sbp2_login_response {
+	u16 login_id;
+	u16 length;
+	u32 command_block_agent_high;
+	u32 command_block_agent_low;
+	u32 reconnect_hold;
+};
+
+struct sbp2_command_orb {
+	struct sbp2_orb base;
+	struct {
+		u32 next_high;
+		u32 next_low;
+		u32 data_descriptor_high;
+		u32 data_descriptor_low;
+		u32 data_size:16;
+		u32 page_size:3;
+		u32 page_table_present:1;
+		u32 max_payload:4;
+		u32 speed:3;
+		u32 direction:1;
+		u32 reserved:1;
+		u32 request_format:2;
+		u32 notify:1;
+		u8 command_block[12];
+	} request;
+	struct scsi_cmnd *cmd;
+	scsi_done_fn_t done;
+	struct fw_unit *unit;
+
+	struct {
+		u16 segment_base_high;
+		u16 length;
+		u32 segment_base_low;
+	} page_table[SG_ALL];
+	dma_addr_t page_table_bus;
+	dma_addr_t request_buffer_bus;
+};
+
+/*
+ * List of devices with known bugs.
+ *
+ * The firmware_revision field, masked with 0xffff00, is the best
+ * indicator for the type of bridge chip of a device.  It yields a few
+ * false positives but this did not break correctly behaving devices
+ * so far.  We use ~0 as a wildcard, since the 24 bit values we get
+ * from the config rom can never match that.
+ */
+static const struct {
+	u32 firmware_revision;
+	u32 model;
+	unsigned workarounds;
+} sbp2_workarounds_table[] = {
+	/* DViCO Momobay CX-1 with TSB42AA9 bridge */ {
+		.firmware_revision	= 0x002800,
+		.model			= 0x001010,
+		.workarounds		= SBP2_WORKAROUND_INQUIRY_36 |
+					  SBP2_WORKAROUND_MODE_SENSE_8,
+	},
+	/* Initio bridges, actually only needed for some older ones */ {
+		.firmware_revision	= 0x000200,
+		.model			= ~0,
+		.workarounds		= SBP2_WORKAROUND_INQUIRY_36,
+	},
+	/* Symbios bridge */ {
+		.firmware_revision	= 0xa0b800,
+		.model			= ~0,
+		.workarounds		= SBP2_WORKAROUND_128K_MAX_TRANS,
+	},
+	/*
+	 * Note about the following Apple iPod blacklist entries:
+	 *
+	 * There are iPods (2nd gen, 3rd gen) with model_id==0.  Since our
+	 * matching logic treats 0 as a wildcard, we cannot match this ID
+	 * without rewriting the matching routine.  Fortunately these iPods
+	 * do not feature the read_capacity bug according to one report.
+	 * Read_capacity behaviour as well as model_id could change due to
+	 * Apple-supplied firmware updates though.
+	 */
+	/* iPod 4th generation */ {
+		.firmware_revision	= 0x0a2700,
+		.model			= 0x000021,
+		.workarounds		= SBP2_WORKAROUND_FIX_CAPACITY,
+	},
+	/* iPod mini */ {
+		.firmware_revision	= 0x0a2700,
+		.model			= 0x000023,
+		.workarounds		= SBP2_WORKAROUND_FIX_CAPACITY,
+	},
+	/* iPod Photo */ {
+		.firmware_revision	= 0x0a2700,
+		.model			= 0x00007e,
+		.workarounds		= SBP2_WORKAROUND_FIX_CAPACITY,
+	}
+};
+
+static void
+sbp2_status_write(struct fw_card *card, struct fw_request *request,
+		  int tcode, int destination, int source,
+		  int generation, int speed,
+		  unsigned long long offset,
+		  u32 * payload, size_t length, void *callback_data)
+{
+	struct fw_unit *unit = (struct fw_unit *)callback_data;
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct sbp2_orb *orb;
+	struct sbp2_status status;
+	size_t header_size;
+	unsigned long flags;
+
+	if (tcode != TCODE_WRITE_BLOCK_REQUEST ||
+	    length == 0 || length > sizeof status) {
+		fw_send_response(card, request, RCODE_TYPE_ERROR);
+		return;
+	}
+
+	header_size = min(length, 2 * sizeof(u32));
+	fw_memcpy_from_be32(&status, payload, header_size);
+	if (length > header_size)
+		memcpy(status.data, &payload[2], length - header_size);
+	if (status.source == 2 || status.source == 3) {
+		fw_notify("non-orb related status write, not handled\n");
+		fw_send_response(card, request, RCODE_COMPLETE);
+		return;
+	}
+
+	/* Lookup the orb corresponding to this status write. */
+	spin_lock_irqsave(&card->lock, flags);
+	list_for_each_entry(orb, &sd->orb_list, link) {
+		if (status.orb_high == 0 &&
+		    status.orb_low == orb->request_bus) {
+			list_del(&orb->link);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&card->lock, flags);
+
+	if (&orb->link != &sd->orb_list)
+		orb->callback(orb, &status);
+	else
+		fw_error("status write for unknown orb\n");
+
+	fw_send_response(card, request, RCODE_COMPLETE);
+}
+
+static void
+complete_transaction(struct fw_card *card, int rcode,
+		     u32 *payload, size_t length, void *data)
+{
+	struct sbp2_orb *orb = data;
+	unsigned long flags;
+
+	orb->rcode = rcode;
+	if (rcode != RCODE_COMPLETE) {
+		spin_lock_irqsave(&card->lock, flags);
+		list_del(&orb->link);
+		spin_unlock_irqrestore(&card->lock, flags);
+		orb->callback(orb, NULL);
+	}
+}
+
+static void
+sbp2_send_orb(struct sbp2_orb *orb, struct fw_unit *unit,
+	      int node_id, int generation, u64 offset)
+{
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	unsigned long flags;
+
+	orb->pointer[0] = 0;
+	orb->pointer[1] = orb->request_bus;
+	fw_memcpy_to_be32(orb->pointer, orb->pointer, sizeof orb->pointer);
+
+	spin_lock_irqsave(&device->card->lock, flags);
+	list_add_tail(&orb->link, &sd->orb_list);
+	spin_unlock_irqrestore(&device->card->lock, flags);
+
+	fw_send_request(device->card, &orb->t, TCODE_WRITE_BLOCK_REQUEST,
+			node_id | LOCAL_BUS, generation,
+			device->node->max_speed, offset,
+			orb->pointer, sizeof orb->pointer,
+			complete_transaction, orb);
+}
+
+static void sbp2_cancel_orbs(struct fw_unit *unit)
+{
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct sbp2_orb *orb, *next;
+	struct list_head list;
+	unsigned long flags;
+
+	INIT_LIST_HEAD(&list);
+	spin_lock_irqsave(&device->card->lock, flags);
+	list_splice_init(&sd->orb_list, &list);
+	spin_unlock_irqrestore(&device->card->lock, flags);
+
+	list_for_each_entry_safe(orb, next, &list, link) {
+		orb->rcode = RCODE_CANCELLED;
+		orb->callback(orb, NULL);
+	}
+}
+
+static void
+complete_management_orb(struct sbp2_orb *base_orb, struct sbp2_status *status)
+{
+	struct sbp2_management_orb *orb =
+	    (struct sbp2_management_orb *)base_orb;
+
+	if (status)
+		memcpy(&orb->status, status, sizeof *status);
+	complete(&orb->done);
+}
+
+static int
+sbp2_send_management_orb(struct fw_unit *unit, int node_id, int generation,
+			 int function, int lun, void *response)
+{
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct sbp2_management_orb *orb;
+	unsigned long timeout;
+	int retval = -EIO;
+
+	orb = kzalloc(sizeof *orb, GFP_ATOMIC);
+	if (orb == NULL)
+		return -ENOMEM;
+
+	/* The sbp2 device is going to send a block read request to
+	 * read out the request from host memory, so map it for
+	 * dma. */
+	orb->base.request_bus =
+		dma_map_single(device->card->device, &orb->request,
+			       sizeof orb->request, DMA_TO_DEVICE);
+
+	orb->response_bus =
+		dma_map_single(device->card->device, &orb->response,
+			       sizeof orb->response, DMA_FROM_DEVICE);
+
+	orb->request.response_high    = 0;
+	orb->request.response_low     = orb->response_bus;
+	orb->request.notify           = 1;
+	orb->request.function         = function;
+	orb->request.lun              = lun;
+	orb->request.response_length  = sizeof orb->response;
+	orb->request.status_fifo_high = sd->address_handler.offset >> 32;
+	orb->request.status_fifo_low  = sd->address_handler.offset;
+
+	/* FIXME: Yeah, ok this isn't elegant, we hardwire exclusive
+	 * login and 1 second reconnect time.  The reconnect setting
+	 * is probably fine, but the exclusive login should be an
+	 * option. */
+	if (function == SBP2_LOGIN_REQUEST) {
+		orb->request.exclusive = 1;
+		orb->request.reconnect = 0;
+	}
+
+	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
+
+	init_completion(&orb->done);
+	orb->base.callback = complete_management_orb;
+	sbp2_send_orb(&orb->base, unit,
+		      node_id, generation, sd->management_agent_address);
+
+	timeout = wait_for_completion_timeout(&orb->done, 10 * HZ);
+
+	/* FIXME: Handle bus reset race here. */
+
+	if (orb->base.rcode != RCODE_COMPLETE) {
+		fw_error("management write failed, rcode 0x%02x\n",
+			 orb->base.rcode);
+		goto out;
+	}
+
+	if (timeout == 0) {
+		fw_error("orb reply timed out, rcode=0x%02x\n",
+			 orb->base.rcode);
+		goto out;
+	}
+
+	if (orb->status.response != 0 || orb->status.sbp_status != 0) {
+		fw_error("error status: %d:%d\n",
+			 orb->status.response, orb->status.sbp_status);
+		goto out;
+	}
+
+	retval = 0;
+ out:
+	dma_unmap_single(device->card->device, orb->base.request_bus,
+			 sizeof orb->request, DMA_TO_DEVICE);
+	dma_unmap_single(device->card->device, orb->response_bus,
+			 sizeof orb->response, DMA_FROM_DEVICE);
+
+	if (response)
+		fw_memcpy_from_be32(response,
+				    orb->response, sizeof orb->response);
+
+	kfree(orb);
+
+	return retval;
+}
+
+static void
+complete_agent_reset_write(struct fw_card *card, int rcode,
+			   u32 *payload, size_t length, void *data)
+{
+	struct fw_transaction *t = data;
+
+	fw_notify("agent reset write rcode=%d\n", rcode);
+	kfree(t);
+}
+
+static int sbp2_agent_reset(struct fw_unit *unit)
+{
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct fw_transaction *t;
+	static u32 zero;
+
+	t = kzalloc(sizeof *t, GFP_ATOMIC);
+	if (t == NULL)
+		return -ENOMEM;
+
+	fw_send_request(device->card, t, TCODE_WRITE_QUADLET_REQUEST,
+			sd->node_id | LOCAL_BUS, sd->generation, SCODE_400,
+			sd->command_block_agent_address + SBP2_AGENT_RESET,
+			&zero, sizeof zero, complete_agent_reset_write, t);
+
+	return 0;
+}
+
+static int add_scsi_devices(struct fw_unit *unit);
+static void remove_scsi_devices(struct fw_unit *unit);
+
+static int sbp2_probe(struct device *dev)
+{
+	struct fw_unit *unit = fw_unit(dev);
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd;
+	struct fw_csr_iterator ci;
+	int i, key, value, lun, retval;
+	int node_id, generation, local_node_id;
+	struct sbp2_login_response response;
+	u32 model, firmware_revision;
+
+	sd = kzalloc(sizeof *sd, GFP_KERNEL);
+	if (sd == NULL)
+		return -ENOMEM;
+
+	unit->device.driver_data = sd;
+	INIT_LIST_HEAD(&sd->orb_list);
+
+	sd->address_handler.length = 0x100;
+	sd->address_handler.address_callback = sbp2_status_write;
+	sd->address_handler.callback_data = unit;
+
+	if (fw_core_add_address_handler(&sd->address_handler,
+					&fw_high_memory_region) < 0) {
+		kfree(sd);
+		return -EBUSY;
+	}
+
+	if (fw_device_enable_phys_dma(device) < 0) {
+		fw_core_remove_address_handler(&sd->address_handler);
+		kfree(sd);
+		return -EBUSY;
+	}
+
+	/* Scan unit directory to get management agent address,
+	 * firmware revison and model.  Initialize firmware_revision
+	 * and model to values that wont match anything in our table. */
+	firmware_revision = 0xff000000;
+	model = 0xff000000;
+	fw_csr_iterator_init(&ci, unit->directory);
+	while (fw_csr_iterator_next(&ci, &key, &value)) {
+		switch (key) {
+		case CSR_DEPENDENT_INFO | CSR_OFFSET:
+			sd->management_agent_address =
+				0xfffff0000000ULL + 4 * value;
+			break;
+		case SBP2_FIRMWARE_REVISION:
+			firmware_revision = value;
+			break;
+		case CSR_MODEL:
+			model = value;
+			break;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(sbp2_workarounds_table); i++) {
+		if (sbp2_workarounds_table[i].firmware_revision !=
+		    (firmware_revision & 0xffffff00))
+			continue;
+		if (sbp2_workarounds_table[i].model != model)
+			continue;
+		sd->workarounds |= sbp2_workarounds_table[i].workarounds;
+		break;
+	}
+
+	if (sd->workarounds)
+		fw_notify("Workarounds for node %s: 0x%x "
+			  "(firmware_revision 0x%06x, model_id 0x%06x)\n",
+			  unit->device.bus_id,
+			  sd->workarounds, firmware_revision, model);
+
+	/* FIXME: Make this work for multi-lun devices. */
+	lun = 0;
+
+	generation    = device->card->generation;
+	node_id       = device->node->node_id;
+	local_node_id = device->card->local_node->node_id;
+
+	/* FIXME: We should probably do this from a keventd callback
+	 * and handle retries by rescheduling the work. */
+	if (sbp2_send_management_orb(unit, node_id, generation,
+				     SBP2_LOGIN_REQUEST, lun, &response) < 0) {
+		fw_core_remove_address_handler(&sd->address_handler);
+		kfree(sd);
+		return -EBUSY;
+	}
+
+	sd->generation   = generation;
+	sd->node_id      = node_id;
+	sd->address_high = (LOCAL_BUS | local_node_id) << 16;
+
+	/* Get command block agent offset and login id. */
+	sd->command_block_agent_address =
+		((u64) response.command_block_agent_high << 32) |
+		response.command_block_agent_low;
+	sd->login_id = response.login_id;
+
+	fw_notify("logged in to sbp2 unit %s\n", unit->device.bus_id);
+	fw_notify(" - management_agent_address: 0x%012llx\n",
+		  sd->management_agent_address);
+	fw_notify(" - command_block_agent_address: 0x%012llx\n",
+		  sd->command_block_agent_address);
+	fw_notify(" - status write address: 0x%012llx\n",
+		  sd->address_handler.offset);
+
+#if 0
+	/* FIXME: The linux1394 sbp2 does these last few steps. */
+	sbp2_set_busy_timeout(scsi_id);
+
+	sbp2_max_speed_and_size(scsi_id);
+#endif
+
+	sbp2_agent_reset(unit);
+
+	retval = add_scsi_devices(unit);
+	if (retval < 0) {
+		sbp2_send_management_orb(unit, sd->node_id, sd->generation,
+					 SBP2_LOGOUT_REQUEST, sd->login_id,
+					 NULL);
+		fw_core_remove_address_handler(&sd->address_handler);
+		kfree(sd);
+		return retval;
+	}
+
+	return 0;
+}
+
+static int sbp2_remove(struct device *dev)
+{
+	struct fw_unit *unit = fw_unit(dev);
+	struct sbp2_device *sd = unit->device.driver_data;
+
+	sbp2_send_management_orb(unit, sd->node_id, sd->generation,
+				 SBP2_LOGOUT_REQUEST, sd->login_id, NULL);
+
+	remove_scsi_devices(unit);
+
+	fw_core_remove_address_handler(&sd->address_handler);
+	kfree(sd);
+
+	fw_notify("removed sbp2 unit %s\n", dev->bus_id);
+
+	return 0;
+}
+
+static void sbp2_reconnect(void *data)
+{
+	struct fw_unit *unit = data;
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	int generation, node_id, local_node_id;
+
+	fw_notify("in sbp2_reconnect, reconnecting to unit %s\n",
+		  unit->device.bus_id);
+
+	generation    = device->card->generation;
+	node_id       = device->node->node_id;
+	local_node_id = device->card->local_node->node_id;
+
+	sbp2_send_management_orb(unit, node_id, generation,
+				 SBP2_RECONNECT_REQUEST, sd->login_id, NULL);
+
+	/* FIXME: handle reconnect failures. */
+
+	sbp2_cancel_orbs(unit);
+
+	sd->generation   = generation;
+	sd->node_id      = node_id;
+	sd->address_high = (LOCAL_BUS | local_node_id) << 16;
+}
+
+static void sbp2_update(struct fw_unit *unit)
+{
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+
+	fw_device_enable_phys_dma(device);
+
+	INIT_WORK(&sd->work, sbp2_reconnect, unit);
+	schedule_work(&sd->work);
+}
+
+#define SBP2_UNIT_SPEC_ID_ENTRY	0x0000609e
+#define SBP2_SW_VERSION_ENTRY	0x00010483
+
+static struct fw_device_id sbp2_id_table[] = {
+	{
+		.match_flags  = FW_MATCH_SPECIFIER_ID | FW_MATCH_VERSION,
+		.specifier_id = SBP2_UNIT_SPEC_ID_ENTRY,
+		.version      = SBP2_SW_VERSION_ENTRY
+	},
+	{ }
+};
+
+static struct fw_driver sbp2_driver = {
+	.driver   = {
+		.owner  = THIS_MODULE,
+		.name   = sbp2_driver_name,
+		.bus    = &fw_bus_type,
+		.probe  = sbp2_probe,
+		.remove = sbp2_remove,
+	},
+	.update   = sbp2_update,
+	.id_table = sbp2_id_table,
+};
+
+static unsigned int sbp2_status_to_sense_data(u8 * sbp2_status, u8 * sense_data)
+{
+	sense_data[0] = 0x70;
+	sense_data[1] = 0x0;
+	sense_data[2] = sbp2_status[1];
+	sense_data[3] = sbp2_status[4];
+	sense_data[4] = sbp2_status[5];
+	sense_data[5] = sbp2_status[6];
+	sense_data[6] = sbp2_status[7];
+	sense_data[7] = 10;
+	sense_data[8] = sbp2_status[8];
+	sense_data[9] = sbp2_status[9];
+	sense_data[10] = sbp2_status[10];
+	sense_data[11] = sbp2_status[11];
+	sense_data[12] = sbp2_status[2];
+	sense_data[13] = sbp2_status[3];
+	sense_data[14] = sbp2_status[12];
+	sense_data[15] = sbp2_status[13];
+
+	switch (sbp2_status[0] & 0x3f) {
+	case SAM_STAT_GOOD:
+		return DID_OK;
+
+	case SAM_STAT_CHECK_CONDITION:
+		/* return CHECK_CONDITION << 1 | DID_OK << 16; */
+		return DID_OK;
+
+	case SAM_STAT_BUSY:
+		return DID_BUS_BUSY;
+
+	case SAM_STAT_CONDITION_MET:
+	case SAM_STAT_RESERVATION_CONFLICT:
+	case SAM_STAT_COMMAND_TERMINATED:
+	default:
+		return DID_ERROR;
+	}
+}
+
+static void
+complete_command_orb(struct sbp2_orb *base_orb, struct sbp2_status *status)
+{
+	struct sbp2_command_orb *orb = (struct sbp2_command_orb *)base_orb;
+	struct fw_unit *unit = orb->unit;
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct scatterlist *sg;
+	int result;
+
+	if (status != NULL) {
+		if (status->dead) {
+			fw_notify("agent died, issuing agent reset\n");
+			sbp2_agent_reset(unit);
+		}
+
+		switch (status->response) {
+		case SBP2_STATUS_REQUEST_COMPLETE:
+			result = DID_OK;
+			break;
+		case SBP2_STATUS_TRANSPORT_FAILURE:
+			result = DID_BUS_BUSY;
+			break;
+		case SBP2_STATUS_ILLEGAL_REQUEST:
+		case SBP2_STATUS_VENDOR_DEPENDENT:
+		default:
+			result = DID_ERROR;
+			break;
+		}
+
+		if (result == DID_OK && status->len > 1)
+			result = sbp2_status_to_sense_data(status->data,
+							   orb->cmd->sense_buffer);
+	} else {
+		/* If the orb completes with status == NULL, something
+		 * went wrong, typically a bus reset happened mid-orb
+		 * or when sending the write (less likely). */
+		fw_notify("no command orb status, rcode=%d\n",
+			  orb->base.rcode);
+		result = DID_ERROR;
+	}
+
+	dma_unmap_single(device->card->device, orb->base.request_bus,
+			 sizeof orb->request, DMA_TO_DEVICE);
+
+	if (orb->cmd->use_sg > 0) {
+		sg = (struct scatterlist *)orb->cmd->request_buffer;
+		dma_unmap_sg(device->card->device, sg, orb->cmd->use_sg,
+			     orb->cmd->sc_data_direction);
+	}
+
+	if (orb->page_table_bus != 0)
+		dma_unmap_single(device->card->device, orb->page_table_bus,
+				 sizeof orb->page_table_bus, DMA_TO_DEVICE);
+
+	if (orb->request_buffer_bus != 0)
+		dma_unmap_single(device->card->device, orb->request_buffer_bus,
+				 sizeof orb->request_buffer_bus,
+				 DMA_FROM_DEVICE);
+
+	orb->cmd->result = result << 16;
+	orb->done(orb->cmd);
+
+	kfree(orb);
+}
+
+static void sbp2_command_orb_map_scatterlist(struct sbp2_command_orb *orb)
+{
+	struct fw_unit *unit =
+		(struct fw_unit *)orb->cmd->device->host->hostdata[0];
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct scatterlist *sg;
+	int sg_len, l, i, j, count;
+	size_t size;
+	dma_addr_t sg_addr;
+
+	sg = (struct scatterlist *)orb->cmd->request_buffer;
+	count = dma_map_sg(device->card->device, sg, orb->cmd->use_sg,
+			   orb->cmd->sc_data_direction);
+
+	/* Handle the special case where there is only one element in
+	 * the scatter list by converting it to an immediate block
+	 * request. This is also a workaround for broken devices such
+	 * as the second generation iPod which doesn't support page
+	 * tables. */
+	if (count == 1 && sg_dma_len(sg) < SBP2_MAX_SG_ELEMENT_LENGTH) {
+		orb->request.data_descriptor_high = sd->address_high;
+		orb->request.data_descriptor_low  = sg_dma_address(sg);
+		orb->request.page_table_present   = 0;
+		orb->request.data_size            = sg_dma_len(sg);
+		return;
+	}
+
+	/* Convert the scatterlist to an sbp2 page table.  If any
+	 * scatterlist entries are too big for sbp2 we split the as we go. */
+	for (i = 0, j = 0; i < count; i++) {
+		sg_len = sg_dma_len(sg + i);
+		sg_addr = sg_dma_address(sg + i);
+		while (sg_len) {
+			l = min(sg_len, SBP2_MAX_SG_ELEMENT_LENGTH);
+			orb->page_table[j].segment_base_low = sg_addr;
+			orb->page_table[j].segment_base_high = 0;
+			orb->page_table[j].length = l;
+			sg_addr += l;
+			sg_len -= l;
+			j++;
+		}
+	}
+
+	size = sizeof orb->page_table[0] * j;
+
+	/* The data_descriptor pointer is the one case where we need
+	 * to fill in the node ID part of the address.  All other
+	 * pointers assume that the data referenced reside on the
+	 * initiator (i.e. us), but data_descriptor can refer to data
+	 * on other nodes so we need to put our ID in descriptor_high. */
+
+	orb->page_table_bus =
+		dma_map_single(device->card->device, orb->page_table,
+			       size, DMA_TO_DEVICE);
+	orb->request.data_descriptor_high = sd->address_high;
+	orb->request.data_descriptor_low  = orb->page_table_bus;
+	orb->request.page_table_present   = 1;
+	orb->request.data_size            = j;
+
+	fw_memcpy_to_be32(orb->page_table, orb->page_table, size);
+}
+
+static void sbp2_command_orb_map_buffer(struct sbp2_command_orb *orb)
+{
+	struct fw_unit *unit =
+		(struct fw_unit *)orb->cmd->device->host->hostdata[0];
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+
+	/* As for map_scatterlist, we need to fill in the high bits of
+	 * the data_descriptor pointer. */
+
+	orb->request_buffer_bus =
+		dma_map_single(device->card->device,
+			       orb->cmd->request_buffer,
+			       orb->cmd->request_bufflen,
+			       orb->cmd->sc_data_direction);
+	orb->request.data_descriptor_high = sd->address_high;
+	orb->request.data_descriptor_low  = orb->request_buffer_bus;
+	orb->request.page_table_present   = 0;
+	orb->request.data_size            = orb->cmd->request_bufflen;
+}
+
+static void sbp2_send_command_orb(struct scsi_cmnd *cmd, scsi_done_fn_t done)
+{
+	struct fw_unit *unit = (struct fw_unit *)cmd->device->host->hostdata[0];
+	struct fw_device *device = fw_device(unit->device.parent);
+	struct sbp2_device *sd = unit->device.driver_data;
+	struct sbp2_command_orb *orb;
+
+	orb = kzalloc(sizeof *orb, GFP_ATOMIC);
+	if (orb == NULL) {
+		fw_notify("failed to alloc orb\n");
+		cmd->result = DID_NO_CONNECT << 16;
+		done(cmd);
+		return;
+	}
+
+	orb->base.request_bus =
+		dma_map_single(device->card->device, &orb->request,
+			       sizeof orb->request, DMA_TO_DEVICE);
+
+	orb->unit = unit;
+	orb->done = done;
+	orb->cmd  = cmd;
+
+	orb->request.next_high   = SBP2_ORB_NULL;
+	orb->request.next_low    = 0x0;
+	/* FIXME: Calculate real payload here. */
+	orb->request.max_payload = 12;	/* 2 ^ (12 + 2) = 4096 */
+	orb->request.speed       = device->node->max_speed;
+	orb->request.notify      = 1;
+
+	if (cmd->sc_data_direction == DMA_FROM_DEVICE)
+		orb->request.direction = SBP2_DIRECTION_FROM_MEDIA;
+	else if (cmd->sc_data_direction == DMA_TO_DEVICE)
+		orb->request.direction = SBP2_DIRECTION_TO_MEDIA;
+
+	if (cmd->use_sg) {
+		sbp2_command_orb_map_scatterlist(orb);
+	} else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
+		/* FIXME: Need to split this into a sg list... but
+		 * could we get the scsi or blk layer to do that by
+		 * reporting our max supported block size? */
+		fw_error("command > 64k\n");
+		cmd->result = DID_ERROR << 16;
+		done(cmd);
+		return;
+	} else if (cmd->request_bufflen > 0) {
+		sbp2_command_orb_map_buffer(orb);
+	}
+
+	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
+
+	memset(orb->request.command_block,
+	       0, sizeof orb->request.command_block);
+	memcpy(orb->request.command_block, cmd->cmnd, COMMAND_SIZE(*cmd->cmnd));
+
+	orb->base.callback = complete_command_orb;
+
+	sbp2_send_orb(&orb->base, unit, sd->node_id, sd->generation,
+		      sd->command_block_agent_address + SBP2_ORB_POINTER);
+}
+
+/* SCSI stack integration */
+
+static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
+{
+	if (cmd->cmnd[0] == REQUEST_SENSE) {
+		fw_notify("request_sense");
+		memcpy(cmd->request_buffer, cmd->sense_buffer, cmd->request_bufflen);
+		memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
+		cmd->result = DID_OK << 16;
+		done(cmd);
+		return 0;
+	}
+
+	/* Bidirectional commands are not yet implemented, and unknown
+	 * transfer direction not handled. */
+	if (cmd->sc_data_direction == DMA_BIDIRECTIONAL) {
+		fw_error("Cannot handle DMA_BIDIRECTIONAL - rejecting command");
+		cmd->result = DID_ERROR << 16;
+		done(cmd);
+		return 0;
+	}
+
+	sbp2_send_command_orb(cmd, done);
+
+	return 0;
+}
+
+static int sbp2_scsi_slave_configure(struct scsi_device *sdev)
+{
+	struct fw_unit *unit = (struct fw_unit *)sdev->host->hostdata[0];
+	struct sbp2_device *sd = unit->device.driver_data;
+
+	if (sdev->type == TYPE_DISK &&
+	    sd->workarounds & SBP2_WORKAROUND_MODE_SENSE_8)
+		sdev->skip_ms_page_8 = 1;
+	if (sd->workarounds & SBP2_WORKAROUND_FIX_CAPACITY) {
+		fw_notify("setting fix_capacity for %s\n", unit->device.bus_id);
+		sdev->fix_capacity = 1;
+	}
+
+	return 0;
+}
+
+/*
+ * Called by scsi stack when something has really gone wrong.  Usually
+ * called when a command has timed-out for some reason.
+ */
+static int sbp2_scsi_abort(struct scsi_cmnd *cmd)
+{
+	struct fw_unit *unit = (struct fw_unit *)cmd->device->host->hostdata[0];
+
+	fw_notify("sbp2_scsi_abort\n");
+
+	sbp2_cancel_orbs(unit);
+
+	return SUCCESS;
+}
+
+static struct scsi_host_template scsi_driver_template = {
+	.module			= THIS_MODULE,
+	.name			= "SBP-2 IEEE-1394",
+	.proc_name		= (char *)sbp2_driver_name,
+	.queuecommand		= sbp2_scsi_queuecommand,
+	.slave_configure	= sbp2_scsi_slave_configure,
+	.eh_abort_handler	= sbp2_scsi_abort,
+	.this_id		= -1,
+	.sg_tablesize		= SG_ALL,
+	.use_clustering		= ENABLE_CLUSTERING,
+	.cmd_per_lun		= 1,	/* SBP2_MAX_CMDS, */
+	.can_queue		= 1,		/* SBP2_MAX_CMDS, */
+	.emulated		= 1,
+};
+
+static int add_scsi_devices(struct fw_unit *unit)
+{
+	struct sbp2_device *sd = unit->device.driver_data;
+	int retval, lun;
+
+	sd->scsi_host = scsi_host_alloc(&scsi_driver_template,
+					sizeof(unsigned long));
+	if (sd->scsi_host == NULL) {
+		fw_error("failed to register scsi host\n");
+		return -1;
+	}
+
+	sd->scsi_host->hostdata[0] = (unsigned long)unit;
+	retval = scsi_add_host(sd->scsi_host, &unit->device);
+	if (retval < 0) {
+		fw_error("failed to add scsi host\n");
+		scsi_host_put(sd->scsi_host);
+		return retval;
+	}
+
+	/* FIXME: Loop over luns here. */
+	lun = 0;
+	retval = scsi_add_device(sd->scsi_host, 0, 0, lun);
+	if (retval < 0) {
+		fw_error("failed to add scsi device\n");
+		scsi_remove_host(sd->scsi_host);
+		scsi_host_put(sd->scsi_host);
+		return retval;
+	}
+
+	return 0;
+}
+
+static void remove_scsi_devices(struct fw_unit *unit)
+{
+	struct sbp2_device *sd = unit->device.driver_data;
+
+	scsi_remove_host(sd->scsi_host);
+	scsi_host_put(sd->scsi_host);
+}
+
+MODULE_AUTHOR("Kristian Høgsberg <krh@bitplanet.net>");
+MODULE_DESCRIPTION("SCSI over IEEE1394");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(ieee1394, sbp2_id_table);
+
+static int __init sbp2_init(void)
+{
+	int retval;
+
+	retval = driver_register(&sbp2_driver.driver);
+	if (retval) {
+		fw_error("Failed to load fw-sbp2 driver.\n");
+		return retval;
+	}
+
+	fw_notify("Loaded fw-sbp2 driver.\n");
+
+	return 0;
+}
+
+static void __exit sbp2_cleanup(void)
+{
+	driver_unregister(&sbp2_driver.driver);
+
+	fw_notify("Unloaded fw-sbp2 driver.\n");
+}
+
+module_init(sbp2_init);
+module_exit(sbp2_cleanup);

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Jeff Garzik]

Kristian Høgsberg wrote:
> Pull in the fw-sbp2 driver for firewire storage devices.
> 
> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
> 
>  drivers/fw/fw-ohci.c |    2 
>  drivers/fw/fw-sbp2.c | 1083 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1084 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/fw/fw-ohci.c b/drivers/fw/fw-ohci.c
> index 78e0324..444e8f0 100644
> --- a/drivers/fw/fw-ohci.c
> +++ b/drivers/fw/fw-ohci.c
> @@ -594,7 +594,7 @@ static void bus_reset_tasklet(unsigned l
>  				 self_id_count, ohci->self_id_buffer);
>  }
>  
> -static irqreturn_t irq_handler(int irq, void *data, struct pt_regs *unused)
> +static irqreturn_t irq_handler(int irq, void *data)
>  {
>  	struct fw_ohci *ohci = data;
>  	u32 event, iso_event;
> diff --git a/drivers/fw/fw-sbp2.c b/drivers/fw/fw-sbp2.c
> new file mode 100644
> index 0000000..e0e7590
> --- /dev/null
> +++ b/drivers/fw/fw-sbp2.c
> @@ -0,0 +1,1083 @@
> +/*						-*- c-basic-offset: 8 -*-
> + * fw-sbp2.c -- SBP2 driver (SCSI over IEEE1394)
> + *
> + * Copyright © 2005  Kristian Høgsberg <krh@bitplanet.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_host.h>
> +
> +#include "fw-transaction.h"
> +#include "fw-topology.h"
> +#include "fw-device.h"
> +
> +/* I don't know why the SCSI stack doesn't define something like this... */
> +typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);

submit a patch?


> +static const char sbp2_driver_name[] = "sbp2";
> +
> +struct sbp2_device {
> +	struct fw_address_handler address_handler;
> +	struct list_head orb_list;
> +	u64 management_agent_address;
> +	u64 command_block_agent_address;
> +	u32 workarounds;
> +	int login_id;
> +
> +	/* We cache these addresses and only update them once we've
> +	 * logged in or reconnected to the sbp2 device.  That way, any
> +	 * IO to the device will automatically fail and get retried if
> +	 * it happens in a window where the device is not ready to
> +	 * handle it (e.g. after a bus reset but before we reconnect). */
> +	int node_id;
> +	int address_high;
> +	int generation;
> +
> +	struct work_struct work;
> +	struct Scsi_Host *scsi_host;
> +};
> +
> +#define SBP2_MAX_SG_ELEMENT_LENGTH	0xf000
> +#define SBP2_MAX_SECTORS		255	/* Max sectors supported */
> +#define SBP2_MAX_CMDS			8	/* This should be safe */
> +
> +#define SBP2_ORB_NULL			0x80000000
> +
> +#define SBP2_DIRECTION_TO_MEDIA		0x0
> +#define SBP2_DIRECTION_FROM_MEDIA	0x1
> +
> +/* Unit directory keys */
> +#define SBP2_COMMAND_SET_SPECIFIER	0x38
> +#define SBP2_COMMAND_SET		0x39
> +#define SBP2_COMMAND_SET_REVISION	0x3b
> +#define SBP2_FIRMWARE_REVISION		0x3c
> +
> +/* Flags for detected oddities and brokeness */
> +#define SBP2_WORKAROUND_128K_MAX_TRANS	0x1
> +#define SBP2_WORKAROUND_INQUIRY_36	0x2
> +#define SBP2_WORKAROUND_MODE_SENSE_8	0x4
> +#define SBP2_WORKAROUND_FIX_CAPACITY	0x8
> +#define SBP2_WORKAROUND_OVERRIDE	0x100
> +
> +/* Management orb opcodes */
> +#define SBP2_LOGIN_REQUEST		0x0
> +#define SBP2_QUERY_LOGINS_REQUEST	0x1
> +#define SBP2_RECONNECT_REQUEST		0x3
> +#define SBP2_SET_PASSWORD_REQUEST	0x4
> +#define SBP2_LOGOUT_REQUEST		0x7
> +#define SBP2_ABORT_TASK_REQUEST		0xb
> +#define SBP2_ABORT_TASK_SET		0xc
> +#define SBP2_LOGICAL_UNIT_RESET		0xe
> +#define SBP2_TARGET_RESET_REQUEST	0xf
> +
> +/* Offsets for command block agent registers */
> +#define SBP2_AGENT_STATE		0x00
> +#define SBP2_AGENT_RESET		0x04
> +#define SBP2_ORB_POINTER		0x08
> +#define SBP2_DOORBELL			0x10
> +#define SBP2_UNSOLICITED_STATUS_ENABLE	0x14
> +
> +/* Status write response codes */
> +#define SBP2_STATUS_REQUEST_COMPLETE	0x0
> +#define SBP2_STATUS_TRANSPORT_FAILURE	0x1
> +#define SBP2_STATUS_ILLEGAL_REQUEST	0x2
> +#define SBP2_STATUS_VENDOR_DEPENDENT	0x3

consider enum{} rather than #define


> +struct sbp2_status {
> +	unsigned int orb_high:16;

unsigned short?  probably generates better code than a bitfield:16


> +	unsigned int sbp_status:8;

unsigned char?


> +	unsigned int len:3;
> +	unsigned int dead:1;
> +	unsigned int response:2;
> +	unsigned int source:2;
> +	u32 orb_low;
> +	u8 data[24];
> +};
> +
> +struct sbp2_orb {
> +	struct fw_transaction t;
> +	dma_addr_t request_bus;
> +	int rcode;
> +	u32 pointer[2];
> +	void (*callback) (struct sbp2_orb * orb, struct sbp2_status * status);
> +	struct list_head link;
> +};
> +
> +struct sbp2_management_orb {
> +	struct sbp2_orb base;
> +	struct {
> +		u32 password_high;
> +		u32 password_low;
> +		u32 response_high;
> +		u32 response_low;
> +		unsigned int lun:16;

unsigned short?


> +		unsigned int function:4;
> +		unsigned int reconnect:4;
> +		unsigned int reserved:4;
> +		unsigned int exclusive:1;
> +		unsigned int request_format:2;
> +		unsigned int notify:1;
> +		unsigned int response_length:16;
> +		unsigned int password_length:16;

unsigned short?


> +		u32 status_fifo_high;
> +		u32 status_fifo_low;
> +	} request;
> +	u32 response[4];
> +	dma_addr_t response_bus;
> +	struct completion done;
> +	struct sbp2_status status;
> +};
> +
> +struct sbp2_login_response {
> +	u16 login_id;
> +	u16 length;
> +	u32 command_block_agent_high;
> +	u32 command_block_agent_low;
> +	u32 reconnect_hold;
> +};

__le16 and __le32?


> +struct sbp2_command_orb {
> +	struct sbp2_orb base;
> +	struct {
> +		u32 next_high;
> +		u32 next_low;
> +		u32 data_descriptor_high;
> +		u32 data_descriptor_low;
> +		u32 data_size:16;
> +		u32 page_size:3;
> +		u32 page_table_present:1;
> +		u32 max_payload:4;
> +		u32 speed:3;
> +		u32 direction:1;
> +		u32 reserved:1;
> +		u32 request_format:2;
> +		u32 notify:1;
> +		u8 command_block[12];

ditto


> +	} request;
> +	struct scsi_cmnd *cmd;
> +	scsi_done_fn_t done;
> +	struct fw_unit *unit;

> +static int
> +sbp2_send_management_orb(struct fw_unit *unit, int node_id, int generation,
> +			 int function, int lun, void *response)
> +{
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	struct sbp2_management_orb *orb;
> +	unsigned long timeout;
> +	int retval = -EIO;
> +
> +	orb = kzalloc(sizeof *orb, GFP_ATOMIC);
> +	if (orb == NULL)
> +		return -ENOMEM;
> +
> +	/* The sbp2 device is going to send a block read request to
> +	 * read out the request from host memory, so map it for
> +	 * dma. */
> +	orb->base.request_bus =
> +		dma_map_single(device->card->device, &orb->request,
> +			       sizeof orb->request, DMA_TO_DEVICE);
> +
> +	orb->response_bus =
> +		dma_map_single(device->card->device, &orb->response,
> +			       sizeof orb->response, DMA_FROM_DEVICE);

check for DMA mapping error


> +	orb->request.response_high    = 0;
> +	orb->request.response_low     = orb->response_bus;
> +	orb->request.notify           = 1;
> +	orb->request.function         = function;
> +	orb->request.lun              = lun;
> +	orb->request.response_length  = sizeof orb->response;
> +	orb->request.status_fifo_high = sd->address_handler.offset >> 32;
> +	orb->request.status_fifo_low  = sd->address_handler.offset;
> +
> +	/* FIXME: Yeah, ok this isn't elegant, we hardwire exclusive
> +	 * login and 1 second reconnect time.  The reconnect setting
> +	 * is probably fine, but the exclusive login should be an
> +	 * option. */
> +	if (function == SBP2_LOGIN_REQUEST) {
> +		orb->request.exclusive = 1;
> +		orb->request.reconnect = 0;
> +	}
> +
> +	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
> +
> +	init_completion(&orb->done);
> +	orb->base.callback = complete_management_orb;
> +	sbp2_send_orb(&orb->base, unit,
> +		      node_id, generation, sd->management_agent_address);
> +
> +	timeout = wait_for_completion_timeout(&orb->done, 10 * HZ);
> +
> +	/* FIXME: Handle bus reset race here. */
> +
> +	if (orb->base.rcode != RCODE_COMPLETE) {
> +		fw_error("management write failed, rcode 0x%02x\n",
> +			 orb->base.rcode);
> +		goto out;
> +	}
> +
> +	if (timeout == 0) {
> +		fw_error("orb reply timed out, rcode=0x%02x\n",
> +			 orb->base.rcode);
> +		goto out;
> +	}
> +
> +	if (orb->status.response != 0 || orb->status.sbp_status != 0) {
> +		fw_error("error status: %d:%d\n",
> +			 orb->status.response, orb->status.sbp_status);
> +		goto out;
> +	}
> +
> +	retval = 0;
> + out:
> +	dma_unmap_single(device->card->device, orb->base.request_bus,
> +			 sizeof orb->request, DMA_TO_DEVICE);
> +	dma_unmap_single(device->card->device, orb->response_bus,
> +			 sizeof orb->response, DMA_FROM_DEVICE);
> +
> +	if (response)
> +		fw_memcpy_from_be32(response,
> +				    orb->response, sizeof orb->response);
> +
> +	kfree(orb);
> +
> +	return retval;
> +}
> +
> +static void
> +complete_agent_reset_write(struct fw_card *card, int rcode,
> +			   u32 *payload, size_t length, void *data)
> +{
> +	struct fw_transaction *t = data;
> +
> +	fw_notify("agent reset write rcode=%d\n", rcode);
> +	kfree(t);
> +}
> +
> +static int sbp2_agent_reset(struct fw_unit *unit)
> +{
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	struct fw_transaction *t;
> +	static u32 zero;
> +
> +	t = kzalloc(sizeof *t, GFP_ATOMIC);
> +	if (t == NULL)
> +		return -ENOMEM;
> +
> +	fw_send_request(device->card, t, TCODE_WRITE_QUADLET_REQUEST,
> +			sd->node_id | LOCAL_BUS, sd->generation, SCODE_400,
> +			sd->command_block_agent_address + SBP2_AGENT_RESET,
> +			&zero, sizeof zero, complete_agent_reset_write, t);
> +
> +	return 0;
> +}
> +
> +static int add_scsi_devices(struct fw_unit *unit);
> +static void remove_scsi_devices(struct fw_unit *unit);
> +
> +static int sbp2_probe(struct device *dev)
> +{
> +	struct fw_unit *unit = fw_unit(dev);
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd;
> +	struct fw_csr_iterator ci;
> +	int i, key, value, lun, retval;
> +	int node_id, generation, local_node_id;
> +	struct sbp2_login_response response;
> +	u32 model, firmware_revision;
> +
> +	sd = kzalloc(sizeof *sd, GFP_KERNEL);
> +	if (sd == NULL)
> +		return -ENOMEM;
> +
> +	unit->device.driver_data = sd;
> +	INIT_LIST_HEAD(&sd->orb_list);
> +
> +	sd->address_handler.length = 0x100;
> +	sd->address_handler.address_callback = sbp2_status_write;
> +	sd->address_handler.callback_data = unit;
> +
> +	if (fw_core_add_address_handler(&sd->address_handler,
> +					&fw_high_memory_region) < 0) {
> +		kfree(sd);
> +		return -EBUSY;
> +	}
> +
> +	if (fw_device_enable_phys_dma(device) < 0) {
> +		fw_core_remove_address_handler(&sd->address_handler);
> +		kfree(sd);
> +		return -EBUSY;
> +	}
> +
> +	/* Scan unit directory to get management agent address,
> +	 * firmware revison and model.  Initialize firmware_revision
> +	 * and model to values that wont match anything in our table. */
> +	firmware_revision = 0xff000000;
> +	model = 0xff000000;
> +	fw_csr_iterator_init(&ci, unit->directory);
> +	while (fw_csr_iterator_next(&ci, &key, &value)) {
> +		switch (key) {
> +		case CSR_DEPENDENT_INFO | CSR_OFFSET:
> +			sd->management_agent_address =
> +				0xfffff0000000ULL + 4 * value;
> +			break;
> +		case SBP2_FIRMWARE_REVISION:
> +			firmware_revision = value;
> +			break;
> +		case CSR_MODEL:
> +			model = value;
> +			break;
> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(sbp2_workarounds_table); i++) {
> +		if (sbp2_workarounds_table[i].firmware_revision !=
> +		    (firmware_revision & 0xffffff00))
> +			continue;
> +		if (sbp2_workarounds_table[i].model != model)
> +			continue;
> +		sd->workarounds |= sbp2_workarounds_table[i].workarounds;
> +		break;
> +	}
> +
> +	if (sd->workarounds)
> +		fw_notify("Workarounds for node %s: 0x%x "
> +			  "(firmware_revision 0x%06x, model_id 0x%06x)\n",
> +			  unit->device.bus_id,
> +			  sd->workarounds, firmware_revision, model);
> +
> +	/* FIXME: Make this work for multi-lun devices. */
> +	lun = 0;

doesn't allowing the stack to issue REPORT LUNS take care of this?


> +	generation    = device->card->generation;
> +	node_id       = device->node->node_id;
> +	local_node_id = device->card->local_node->node_id;
> +
> +	/* FIXME: We should probably do this from a keventd callback
> +	 * and handle retries by rescheduling the work. */
> +	if (sbp2_send_management_orb(unit, node_id, generation,
> +				     SBP2_LOGIN_REQUEST, lun, &response) < 0) {
> +		fw_core_remove_address_handler(&sd->address_handler);
> +		kfree(sd);
> +		return -EBUSY;
> +	}
> +
> +	sd->generation   = generation;
> +	sd->node_id      = node_id;
> +	sd->address_high = (LOCAL_BUS | local_node_id) << 16;
> +
> +	/* Get command block agent offset and login id. */
> +	sd->command_block_agent_address =
> +		((u64) response.command_block_agent_high << 32) |
> +		response.command_block_agent_low;
> +	sd->login_id = response.login_id;
> +
> +	fw_notify("logged in to sbp2 unit %s\n", unit->device.bus_id);
> +	fw_notify(" - management_agent_address: 0x%012llx\n",
> +		  sd->management_agent_address);
> +	fw_notify(" - command_block_agent_address: 0x%012llx\n",
> +		  sd->command_block_agent_address);
> +	fw_notify(" - status write address: 0x%012llx\n",
> +		  sd->address_handler.offset);
> +
> +#if 0
> +	/* FIXME: The linux1394 sbp2 does these last few steps. */
> +	sbp2_set_busy_timeout(scsi_id);
> +
> +	sbp2_max_speed_and_size(scsi_id);
> +#endif
> +
> +	sbp2_agent_reset(unit);
> +
> +	retval = add_scsi_devices(unit);
> +	if (retval < 0) {
> +		sbp2_send_management_orb(unit, sd->node_id, sd->generation,
> +					 SBP2_LOGOUT_REQUEST, sd->login_id,
> +					 NULL);
> +		fw_core_remove_address_handler(&sd->address_handler);
> +		kfree(sd);
> +		return retval;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sbp2_remove(struct device *dev)
> +{
> +	struct fw_unit *unit = fw_unit(dev);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +
> +	sbp2_send_management_orb(unit, sd->node_id, sd->generation,
> +				 SBP2_LOGOUT_REQUEST, sd->login_id, NULL);
> +
> +	remove_scsi_devices(unit);
> +
> +	fw_core_remove_address_handler(&sd->address_handler);
> +	kfree(sd);
> +
> +	fw_notify("removed sbp2 unit %s\n", dev->bus_id);
> +
> +	return 0;
> +}
> +
> +static void sbp2_reconnect(void *data)
> +{
> +	struct fw_unit *unit = data;
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	int generation, node_id, local_node_id;
> +
> +	fw_notify("in sbp2_reconnect, reconnecting to unit %s\n",
> +		  unit->device.bus_id);
> +
> +	generation    = device->card->generation;
> +	node_id       = device->node->node_id;
> +	local_node_id = device->card->local_node->node_id;
> +
> +	sbp2_send_management_orb(unit, node_id, generation,
> +				 SBP2_RECONNECT_REQUEST, sd->login_id, NULL);
> +
> +	/* FIXME: handle reconnect failures. */
> +
> +	sbp2_cancel_orbs(unit);
> +
> +	sd->generation   = generation;
> +	sd->node_id      = node_id;
> +	sd->address_high = (LOCAL_BUS | local_node_id) << 16;
> +}
> +
> +static void sbp2_update(struct fw_unit *unit)
> +{
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +
> +	fw_device_enable_phys_dma(device);
> +
> +	INIT_WORK(&sd->work, sbp2_reconnect, unit);
> +	schedule_work(&sd->work);
> +}
> +
> +#define SBP2_UNIT_SPEC_ID_ENTRY	0x0000609e
> +#define SBP2_SW_VERSION_ENTRY	0x00010483
> +
> +static struct fw_device_id sbp2_id_table[] = {
> +	{
> +		.match_flags  = FW_MATCH_SPECIFIER_ID | FW_MATCH_VERSION,
> +		.specifier_id = SBP2_UNIT_SPEC_ID_ENTRY,
> +		.version      = SBP2_SW_VERSION_ENTRY
> +	},
> +	{ }
> +};
> +
> +static struct fw_driver sbp2_driver = {
> +	.driver   = {
> +		.owner  = THIS_MODULE,
> +		.name   = sbp2_driver_name,
> +		.bus    = &fw_bus_type,
> +		.probe  = sbp2_probe,
> +		.remove = sbp2_remove,
> +	},
> +	.update   = sbp2_update,
> +	.id_table = sbp2_id_table,
> +};
> +
> +static unsigned int sbp2_status_to_sense_data(u8 * sbp2_status, u8 * sense_data)
> +{
> +	sense_data[0] = 0x70;
> +	sense_data[1] = 0x0;
> +	sense_data[2] = sbp2_status[1];
> +	sense_data[3] = sbp2_status[4];
> +	sense_data[4] = sbp2_status[5];
> +	sense_data[5] = sbp2_status[6];
> +	sense_data[6] = sbp2_status[7];
> +	sense_data[7] = 10;
> +	sense_data[8] = sbp2_status[8];
> +	sense_data[9] = sbp2_status[9];
> +	sense_data[10] = sbp2_status[10];
> +	sense_data[11] = sbp2_status[11];
> +	sense_data[12] = sbp2_status[2];
> +	sense_data[13] = sbp2_status[3];
> +	sense_data[14] = sbp2_status[12];
> +	sense_data[15] = sbp2_status[13];
> +
> +	switch (sbp2_status[0] & 0x3f) {
> +	case SAM_STAT_GOOD:
> +		return DID_OK;
> +
> +	case SAM_STAT_CHECK_CONDITION:
> +		/* return CHECK_CONDITION << 1 | DID_OK << 16; */
> +		return DID_OK;
> +
> +	case SAM_STAT_BUSY:
> +		return DID_BUS_BUSY;
> +
> +	case SAM_STAT_CONDITION_MET:
> +	case SAM_STAT_RESERVATION_CONFLICT:
> +	case SAM_STAT_COMMAND_TERMINATED:
> +	default:
> +		return DID_ERROR;
> +	}
> +}
> +
> +static void
> +complete_command_orb(struct sbp2_orb *base_orb, struct sbp2_status *status)
> +{
> +	struct sbp2_command_orb *orb = (struct sbp2_command_orb *)base_orb;
> +	struct fw_unit *unit = orb->unit;
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct scatterlist *sg;
> +	int result;
> +
> +	if (status != NULL) {
> +		if (status->dead) {
> +			fw_notify("agent died, issuing agent reset\n");
> +			sbp2_agent_reset(unit);
> +		}
> +
> +		switch (status->response) {
> +		case SBP2_STATUS_REQUEST_COMPLETE:
> +			result = DID_OK;
> +			break;
> +		case SBP2_STATUS_TRANSPORT_FAILURE:
> +			result = DID_BUS_BUSY;
> +			break;
> +		case SBP2_STATUS_ILLEGAL_REQUEST:
> +		case SBP2_STATUS_VENDOR_DEPENDENT:
> +		default:
> +			result = DID_ERROR;
> +			break;
> +		}
> +
> +		if (result == DID_OK && status->len > 1)
> +			result = sbp2_status_to_sense_data(status->data,
> +							   orb->cmd->sense_buffer);
> +	} else {
> +		/* If the orb completes with status == NULL, something
> +		 * went wrong, typically a bus reset happened mid-orb
> +		 * or when sending the write (less likely). */
> +		fw_notify("no command orb status, rcode=%d\n",
> +			  orb->base.rcode);
> +		result = DID_ERROR;
> +	}
> +
> +	dma_unmap_single(device->card->device, orb->base.request_bus,
> +			 sizeof orb->request, DMA_TO_DEVICE);
> +
> +	if (orb->cmd->use_sg > 0) {
> +		sg = (struct scatterlist *)orb->cmd->request_buffer;
> +		dma_unmap_sg(device->card->device, sg, orb->cmd->use_sg,
> +			     orb->cmd->sc_data_direction);
> +	}
> +
> +	if (orb->page_table_bus != 0)
> +		dma_unmap_single(device->card->device, orb->page_table_bus,
> +				 sizeof orb->page_table_bus, DMA_TO_DEVICE);
> +
> +	if (orb->request_buffer_bus != 0)
> +		dma_unmap_single(device->card->device, orb->request_buffer_bus,
> +				 sizeof orb->request_buffer_bus,
> +				 DMA_FROM_DEVICE);
> +
> +	orb->cmd->result = result << 16;
> +	orb->done(orb->cmd);
> +
> +	kfree(orb);
> +}
> +
> +static void sbp2_command_orb_map_scatterlist(struct sbp2_command_orb *orb)
> +{
> +	struct fw_unit *unit =
> +		(struct fw_unit *)orb->cmd->device->host->hostdata[0];
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	struct scatterlist *sg;
> +	int sg_len, l, i, j, count;
> +	size_t size;
> +	dma_addr_t sg_addr;
> +
> +	sg = (struct scatterlist *)orb->cmd->request_buffer;
> +	count = dma_map_sg(device->card->device, sg, orb->cmd->use_sg,
> +			   orb->cmd->sc_data_direction);
> +
> +	/* Handle the special case where there is only one element in
> +	 * the scatter list by converting it to an immediate block
> +	 * request. This is also a workaround for broken devices such
> +	 * as the second generation iPod which doesn't support page
> +	 * tables. */
> +	if (count == 1 && sg_dma_len(sg) < SBP2_MAX_SG_ELEMENT_LENGTH) {
> +		orb->request.data_descriptor_high = sd->address_high;
> +		orb->request.data_descriptor_low  = sg_dma_address(sg);
> +		orb->request.page_table_present   = 0;
> +		orb->request.data_size            = sg_dma_len(sg);
> +		return;
> +	}
> +
> +	/* Convert the scatterlist to an sbp2 page table.  If any
> +	 * scatterlist entries are too big for sbp2 we split the as we go. */
> +	for (i = 0, j = 0; i < count; i++) {
> +		sg_len = sg_dma_len(sg + i);
> +		sg_addr = sg_dma_address(sg + i);
> +		while (sg_len) {
> +			l = min(sg_len, SBP2_MAX_SG_ELEMENT_LENGTH);
> +			orb->page_table[j].segment_base_low = sg_addr;
> +			orb->page_table[j].segment_base_high = 0;
> +			orb->page_table[j].length = l;
> +			sg_addr += l;
> +			sg_len -= l;
> +			j++;
> +		}
> +	}
> +
> +	size = sizeof orb->page_table[0] * j;
> +
> +	/* The data_descriptor pointer is the one case where we need
> +	 * to fill in the node ID part of the address.  All other
> +	 * pointers assume that the data referenced reside on the
> +	 * initiator (i.e. us), but data_descriptor can refer to data
> +	 * on other nodes so we need to put our ID in descriptor_high. */
> +
> +	orb->page_table_bus =
> +		dma_map_single(device->card->device, orb->page_table,
> +			       size, DMA_TO_DEVICE);
> +	orb->request.data_descriptor_high = sd->address_high;
> +	orb->request.data_descriptor_low  = orb->page_table_bus;
> +	orb->request.page_table_present   = 1;
> +	orb->request.data_size            = j;
> +
> +	fw_memcpy_to_be32(orb->page_table, orb->page_table, size);
> +}
> +
> +static void sbp2_command_orb_map_buffer(struct sbp2_command_orb *orb)
> +{
> +	struct fw_unit *unit =
> +		(struct fw_unit *)orb->cmd->device->host->hostdata[0];
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +
> +	/* As for map_scatterlist, we need to fill in the high bits of
> +	 * the data_descriptor pointer. */
> +
> +	orb->request_buffer_bus =
> +		dma_map_single(device->card->device,
> +			       orb->cmd->request_buffer,
> +			       orb->cmd->request_bufflen,
> +			       orb->cmd->sc_data_direction);
> +	orb->request.data_descriptor_high = sd->address_high;
> +	orb->request.data_descriptor_low  = orb->request_buffer_bus;
> +	orb->request.page_table_present   = 0;
> +	orb->request.data_size            = orb->cmd->request_bufflen;
> +}
> +
> +static void sbp2_send_command_orb(struct scsi_cmnd *cmd, scsi_done_fn_t done)
> +{
> +	struct fw_unit *unit = (struct fw_unit *)cmd->device->host->hostdata[0];
> +	struct fw_device *device = fw_device(unit->device.parent);
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	struct sbp2_command_orb *orb;
> +
> +	orb = kzalloc(sizeof *orb, GFP_ATOMIC);
> +	if (orb == NULL) {
> +		fw_notify("failed to alloc orb\n");
> +		cmd->result = DID_NO_CONNECT << 16;
> +		done(cmd);
> +		return;
> +	}
> +
> +	orb->base.request_bus =
> +		dma_map_single(device->card->device, &orb->request,
> +			       sizeof orb->request, DMA_TO_DEVICE);
> +
> +	orb->unit = unit;
> +	orb->done = done;
> +	orb->cmd  = cmd;
> +
> +	orb->request.next_high   = SBP2_ORB_NULL;
> +	orb->request.next_low    = 0x0;
> +	/* FIXME: Calculate real payload here. */
> +	orb->request.max_payload = 12;	/* 2 ^ (12 + 2) = 4096 */
> +	orb->request.speed       = device->node->max_speed;
> +	orb->request.notify      = 1;
> +
> +	if (cmd->sc_data_direction == DMA_FROM_DEVICE)
> +		orb->request.direction = SBP2_DIRECTION_FROM_MEDIA;
> +	else if (cmd->sc_data_direction == DMA_TO_DEVICE)
> +		orb->request.direction = SBP2_DIRECTION_TO_MEDIA;
> +
> +	if (cmd->use_sg) {
> +		sbp2_command_orb_map_scatterlist(orb);
> +	} else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
> +		/* FIXME: Need to split this into a sg list... but
> +		 * could we get the scsi or blk layer to do that by
> +		 * reporting our max supported block size? */
> +		fw_error("command > 64k\n");
> +		cmd->result = DID_ERROR << 16;
> +		done(cmd);
> +		return;
> +	} else if (cmd->request_bufflen > 0) {
> +		sbp2_command_orb_map_buffer(orb);
> +	}
> +
> +	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
> +
> +	memset(orb->request.command_block,
> +	       0, sizeof orb->request.command_block);
> +	memcpy(orb->request.command_block, cmd->cmnd, COMMAND_SIZE(*cmd->cmnd));
> +
> +	orb->base.callback = complete_command_orb;
> +
> +	sbp2_send_orb(&orb->base, unit, sd->node_id, sd->generation,
> +		      sd->command_block_agent_address + SBP2_ORB_POINTER);
> +}
> +
> +/* SCSI stack integration */
> +
> +static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
> +{
> +	if (cmd->cmnd[0] == REQUEST_SENSE) {
> +		fw_notify("request_sense");
> +		memcpy(cmd->request_buffer, cmd->sense_buffer, cmd->request_bufflen);
> +		memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
> +		cmd->result = DID_OK << 16;
> +		done(cmd);
> +		return 0;
> +	}

this is a broken emulation.  this command is specified to not repeatedly 
return the same sense data.


> +	/* Bidirectional commands are not yet implemented, and unknown
> +	 * transfer direction not handled. */
> +	if (cmd->sc_data_direction == DMA_BIDIRECTIONAL) {
> +		fw_error("Cannot handle DMA_BIDIRECTIONAL - rejecting command");
> +		cmd->result = DID_ERROR << 16;
> +		done(cmd);
> +		return 0;
> +	}
> +
> +	sbp2_send_command_orb(cmd, done);
> +
> +	return 0;
> +}
> +
> +static int sbp2_scsi_slave_configure(struct scsi_device *sdev)
> +{
> +	struct fw_unit *unit = (struct fw_unit *)sdev->host->hostdata[0];
> +	struct sbp2_device *sd = unit->device.driver_data;
> +
> +	if (sdev->type == TYPE_DISK &&
> +	    sd->workarounds & SBP2_WORKAROUND_MODE_SENSE_8)
> +		sdev->skip_ms_page_8 = 1;
> +	if (sd->workarounds & SBP2_WORKAROUND_FIX_CAPACITY) {
> +		fw_notify("setting fix_capacity for %s\n", unit->device.bus_id);
> +		sdev->fix_capacity = 1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Called by scsi stack when something has really gone wrong.  Usually
> + * called when a command has timed-out for some reason.
> + */
> +static int sbp2_scsi_abort(struct scsi_cmnd *cmd)
> +{
> +	struct fw_unit *unit = (struct fw_unit *)cmd->device->host->hostdata[0];
> +
> +	fw_notify("sbp2_scsi_abort\n");
> +
> +	sbp2_cancel_orbs(unit);
> +
> +	return SUCCESS;
> +}
> +
> +static struct scsi_host_template scsi_driver_template = {
> +	.module			= THIS_MODULE,
> +	.name			= "SBP-2 IEEE-1394",
> +	.proc_name		= (char *)sbp2_driver_name,
> +	.queuecommand		= sbp2_scsi_queuecommand,
> +	.slave_configure	= sbp2_scsi_slave_configure,
> +	.eh_abort_handler	= sbp2_scsi_abort,
> +	.this_id		= -1,
> +	.sg_tablesize		= SG_ALL,
> +	.use_clustering		= ENABLE_CLUSTERING,
> +	.cmd_per_lun		= 1,	/* SBP2_MAX_CMDS, */
> +	.can_queue		= 1,		/* SBP2_MAX_CMDS, */
> +	.emulated		= 1,
> +};
> +
> +static int add_scsi_devices(struct fw_unit *unit)
> +{
> +	struct sbp2_device *sd = unit->device.driver_data;
> +	int retval, lun;
> +
> +	sd->scsi_host = scsi_host_alloc(&scsi_driver_template,
> +					sizeof(unsigned long));
> +	if (sd->scsi_host == NULL) {
> +		fw_error("failed to register scsi host\n");
> +		return -1;
> +	}
> +
> +	sd->scsi_host->hostdata[0] = (unsigned long)unit;
> +	retval = scsi_add_host(sd->scsi_host, &unit->device);
> +	if (retval < 0) {
> +		fw_error("failed to add scsi host\n");
> +		scsi_host_put(sd->scsi_host);
> +		return retval;
> +	}
> +
> +	/* FIXME: Loop over luns here. */

should be no need, let the stack do that for you



> +	lun = 0;
> +	retval = scsi_add_device(sd->scsi_host, 0, 0, lun);
> +	if (retval < 0) {
> +		fw_error("failed to add scsi device\n");
> +		scsi_remove_host(sd->scsi_host);
> +		scsi_host_put(sd->scsi_host);
> +		return retval;
> +	}
> +
> +	return 0;
> +}
> +
> +static void remove_scsi_devices(struct fw_unit *unit)
> +{
> +	struct sbp2_device *sd = unit->device.driver_data;
> +
> +	scsi_remove_host(sd->scsi_host);
> +	scsi_host_put(sd->scsi_host);
> +}
> +
> +MODULE_AUTHOR("Kristian Høgsberg <krh@bitplanet.net>");
> +MODULE_DESCRIPTION("SCSI over IEEE1394");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(ieee1394, sbp2_id_table);
> +
> +static int __init sbp2_init(void)
> +{
> +	int retval;
> +
> +	retval = driver_register(&sbp2_driver.driver);
> +	if (retval) {
> +		fw_error("Failed to load fw-sbp2 driver.\n");
> +		return retval;
> +	}
> +
> +	fw_notify("Loaded fw-sbp2 driver.\n");
> +
> +	return 0;
> +}
> +
> +static void __exit sbp2_cleanup(void)
> +{
> +	driver_unregister(&sbp2_driver.driver);
> +
> +	fw_notify("Unloaded fw-sbp2 driver.\n");
> +}
> +
> +module_init(sbp2_init);
> +module_exit(sbp2_cleanup);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Stefan Richter]

Jeff Garzik wrote:
> Kristian Høgsberg wrote:
>> +struct sbp2_status {
>> +    unsigned int orb_high:16;
> 
> unsigned short?  probably generates better code than a bitfield:16
> 
> 
>> +    unsigned int sbp_status:8;
> 
> unsigned char?
> 
> 
>> +    unsigned int len:3;
>> +    unsigned int dead:1;
>> +    unsigned int response:2;
>> +    unsigned int source:2;
>> +    u32 orb_low;
>> +    u8 data[24];
>> +};

This as well as ORBs and responses are series of u32 that go in or out
in big endian byte order.

The old FireWire drivers have two styles to deal with such data
structures: Define them as a struct or array of u32 and manipulate bits
by arithmetic expressions, or define them as bitfields similarly to what
you see above. Of course either approach has to account for host byte
order in one or another way.

...
>> +
>> +    /* FIXME: Make this work for multi-lun devices. */
>> +    lun = 0;
> 
> doesn't allowing the stack to issue REPORT LUNS take care of this?

SBP-2 LUs and their LUNs are obtained from the ISO/IEC 13213
configuration ROM of the target.

The FIXME comment is misleadingly worded, at least as far as the "old"
sbp2 driver is concerned, which AFAIU served as a starting point for
fw-sbp2. The sbp2 driver supports multi-unit targets; it just represents
each LU behind an own Scsi_Host. This had several reasons, most of them
historic now. I am considering to make sbp2 use a single Scsi_Host for
all its LUs and perhaps join LUs of the same target beneath a single
scsi_target. But this project doesn't have priority for me.
-- 
Stefan Richter
-=====-=-==- ==-- --=-=
http://arcgraph.de/sr/

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Kristian Høgsberg]

Jeff Garzik wrote:

Again, thanks for your comments, I've added patches to my git repo, will send 
out a new set on LKML before the end of this week.

>> +/* I don't know why the SCSI stack doesn't define something like 
>> this... */
>> +typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);
> 
> submit a patch?

Hehe, yeah, that might work.

>> +struct sbp2_status {
>> +    unsigned int orb_high:16;
> 
> unsigned short?  probably generates better code than a bitfield:16

This and all other bit fields are now just u32s that I access by shifting and 
masking.

>> +struct sbp2_login_response {
>> +    u16 login_id;
>> +    u16 length;
>> +    u32 command_block_agent_high;
>> +    u32 command_block_agent_low;
>> +    u32 reconnect_hold;
>> +};
> 
> __le16 and __le32?

This struct is filled in using fw_memcpy_from_be32() to copy and byteswap the 
incoming payload, so the fields here are always cpu endian.  The two u16 
fields assume little endian ordering though, I fixed that.

>> +sbp2_send_management_orb(struct fw_unit *unit, int node_id, int 
>> generation,
>> +             int function, int lun, void *response)
>> +{
>> +    struct fw_device *device = fw_device(unit->device.parent);
>> +    struct sbp2_device *sd = unit->device.driver_data;
>> +    struct sbp2_management_orb *orb;
>> +    unsigned long timeout;
>> +    int retval = -EIO;
>> +
>> +    orb = kzalloc(sizeof *orb, GFP_ATOMIC);
>> +    if (orb == NULL)
>> +        return -ENOMEM;
>> +
>> +    /* The sbp2 device is going to send a block read request to
>> +     * read out the request from host memory, so map it for
>> +     * dma. */
>> +    orb->base.request_bus =
>> +        dma_map_single(device->card->device, &orb->request,
>> +                   sizeof orb->request, DMA_TO_DEVICE);
>> +
>> +    orb->response_bus =
>> +        dma_map_single(device->card->device, &orb->response,
>> +                   sizeof orb->response, DMA_FROM_DEVICE);
> 
> check for DMA mapping error

Oops, fixed.

>> +    if (sd->workarounds)
>> +        fw_notify("Workarounds for node %s: 0x%x "
>> +              "(firmware_revision 0x%06x, model_id 0x%06x)\n",
>> +              unit->device.bus_id,
>> +              sd->workarounds, firmware_revision, model);
>> +
>> +    /* FIXME: Make this work for multi-lun devices. */
>> +    lun = 0;
> 
> doesn't allowing the stack to issue REPORT LUNS take care of this?

Possibly, I don't have firewire multi-LUN devices to test with here.  The LUNs 
are also discoverable from the firewire config rom, which is why I put the 
comment there.  This doesn't mean that the SCSI commands for discovering LUNs 
doesn't also work.

>> +/* SCSI stack integration */
>> +
>> +static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, 
>> scsi_done_fn_t done)
>> +{
>> +    if (cmd->cmnd[0] == REQUEST_SENSE) {
>> +        fw_notify("request_sense");
>> +        memcpy(cmd->request_buffer, cmd->sense_buffer, 
>> cmd->request_bufflen);
>> +        memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
>> +        cmd->result = DID_OK << 16;
>> +        done(cmd);
>> +        return 0;
>> +    }
> 
> this is a broken emulation.  this command is specified to not repeatedly 
> return the same sense data.

I copied it over from the old stack under the assumption that it fixed 
something for some device.  I took it out and tested with the 10 or so storage 
devices I have here and it makes no difference.  I've never seen the 
fw_notify() that I put in there trigger.  I'm taking out this workaround for 
now, unless someone can tell me why it should stay there.

Kristian

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Stefan Richter]

Kristian Høgsberg wrote:
> Jeff Garzik wrote:
>> doesn't allowing the stack to issue REPORT LUNS take care of this?
> 
> Possibly, I don't have firewire multi-LUN devices to test with here. 
> The LUNs are also discoverable from the firewire config rom, which is
> why I put the comment there.  This doesn't mean that the SCSI commands
> for discovering LUNs doesn't also work.

I expect REPORT LUNS won't work for many SBP-2 devices. It is not included
in RBC.

We discover LUs properly from the information in the ISO 13213 ROM. We just
don't map multiple LUs of the same target to scsi_device's beneath a single
scsi_target. (We instantiate one Scsi_Host for each LU. I might implement
a respective mapping some day, but there is no bigger benefit of doing so.)

>>> +static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd,
>>> scsi_done_fn_t done)
>>> +{
>>> +    if (cmd->cmnd[0] == REQUEST_SENSE) {
>>> +        fw_notify("request_sense");
>>> +        memcpy(cmd->request_buffer, cmd->sense_buffer,
>>> cmd->request_bufflen);
>>> +        memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
>>> +        cmd->result = DID_OK << 16;
>>> +        done(cmd);
>>> +        return 0;
>>> +    }
>>
>> this is a broken emulation.  this command is specified to not
>> repeatedly return the same sense data.
> 
> I copied it over from the old stack under the assumption that it fixed
> something for some device.

Yes, it's in the old driver. I haven't checked yet when and why it was
written that way. Will do so eventually.

Usually, the SBP-2 status block is used to communicate autosense data.
Targets which do so may not support REQUEST SENSE. Targets which don't
do so have to support REQUEST SENSE; I suppose sbp2's curious REQUEST
SENSE handling is badly broken for such devices as far as any exist.

> I took it out and tested with the 10 or so
> storage devices I have here and it makes no difference.  I've never seen
> the fw_notify() that I put in there trigger.  I'm taking out this
> workaround for now,

OK.

> unless someone can tell me why it should stay there.
> 
> Kristian

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

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Kristian Høgsberg]

Stefan Richter wrote:
> Kristian Høgsberg wrote:
>> Jeff Garzik wrote:
>>> doesn't allowing the stack to issue REPORT LUNS take care of this?
>> Possibly, I don't have firewire multi-LUN devices to test with here. 
>> The LUNs are also discoverable from the firewire config rom, which is
>> why I put the comment there.  This doesn't mean that the SCSI commands
>> for discovering LUNs doesn't also work.
> 
> I expect REPORT LUNS won't work for many SBP-2 devices. It is not included
> in RBC.
> 
> We discover LUs properly from the information in the ISO 13213 ROM. We just
> don't map multiple LUs of the same target to scsi_device's beneath a single
> scsi_target. (We instantiate one Scsi_Host for each LU. I might implement
> a respective mapping some day, but there is no bigger benefit of doing so.)

Yeah, I saw that the stack creates a struct device per LUN, which is kinda 
gross in my opinion.  It's easy enough to discover the LUNs from the rom, I 
just need to figure out how to tell the SCSI stack about multiple LUNs.

Kristian

Re: [PATCH 3/3] Import fw-sbp2 driver.

[Stefan Richter]

(added Cc: lsml)

Kristian Høgsberg wrote at lkml:
> I saw that the stack
[the FireWire stack]
> creates a struct device per LUN, which is kinda gross in my opinion.

If you mean regular "unit directories" here, not SCSI LUs, then one
device per unit makes sense. Different units of a node may implement
different protocols and may have different lifetimes.

If you mean the special representation of SBP-2 logical units by
multiple leaf entries in one and the same unit directory (Why the heck
did they allow two different representations?), then you are right that
the burden could certainly be shifted from the FireWire core to the
SBP-2 protocol driver, WRT ROM scanning and generic device representation.

> It's easy enough to discover the LUNs from the rom, I just need to
> figure out how to tell the SCSI stack about multiple LUNs.

Since REPORT LUNS is not guaranteed to work with SBP-2 targets, we stick
to scsi_add_device(). With some effort we could supply scsi_add_device()
with common instances of Scsi_Host and target for units which reside on
the same target. Alas the scsi_add_device() API has a concept of target
which is quite unfit for most SCSI transports other than SPI. We have
got the following alternatives:
 - Implement a mapping of SAM targets to scsi_add_device targets in
   sbp2. Effectively, map from pointer or EUI-64 to uint. <linux/idr.h>
   has an infrastructure for such a mapping.
   (This is not the Linux way of doing things, but wicked people may be
   tempted to call it 'the Linux SCSI way so far'.)
 - Reform the scsi_add_device API to support SAM targets.
 - Leave stuff as is, concentrate on fixing the FireWire stack's issues
   first.
If you look closely, you see the order of list items reflecting my
personal preference. :-)

Of course, as mentioned before, a precondition to represent multiple LUs
beneath a single representation of a target is to convert sbp2 to
instantiante only one Scsi_Host for many or all SBP-2 units.
-- 
Stefan Richter
-=====-=-==- ==-- -====
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Benjamin Herrenschmidt]

On Tue, 2006-12-05 at 00:22 -0500, Kristian Høgsberg wrote:
> Hi,
> 
> I'm announcing an alternative firewire stack that I've been working on
> the last few weeks.  I'm aiming to implement feature parity with the
> current firewire stack, but not necessarily interface compatibility.
> For now, I have the low-level OHCI driver done, the mid-level
> transaction logic done, and the SBP-2 (storage) driver is basically
> done.  What's missing is a streaming interface (in progress) to allow
> reception and transmission of isochronous data and a userspace
> interface for controlling devices (much like raw1394 or libusb for
> usb).  I'm working out of this git repository:

A very very very quick look at the code shows that:

 - It looks nice / clear
 - It's horribly broken in at least two area :

 DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!

 and

 Where do you handle endianness ? (no need to shout for
 that one).

(Or in general, do not use bitfields period ....)

bitfields format is not guaranteed, and is not endian consistent. 

Ben.

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Benjamin Herrenschmidt wrote:
> On Tue, 2006-12-05 at 00:22 -0500, Kristian Høgsberg wrote:
>> Hi,
>>
>> I'm announcing an alternative firewire stack that I've been working on
>> the last few weeks.  I'm aiming to implement feature parity with the
>> current firewire stack, but not necessarily interface compatibility.
>> For now, I have the low-level OHCI driver done, the mid-level
>> transaction logic done, and the SBP-2 (storage) driver is basically
>> done.  What's missing is a streaming interface (in progress) to allow
>> reception and transmission of isochronous data and a userspace
>> interface for controlling devices (much like raw1394 or libusb for
>> usb).  I'm working out of this git repository:
> 
> A very very very quick look at the code shows that:
> 
>  - It looks nice / clear

Great, good to hear.

>  - It's horribly broken in at least two area :
> 
>  DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!
> 
>  and
> 
>  Where do you handle endianness ? (no need to shout for
>  that one).

Well, the code isn't big-endian safe yet, but the only place where I expect to 
have to fix this is in fw-ohci.c.  I need to figure out how I want to set up 
the OHCI controller to this - it has a couple of bits to control this.  All 
data outside the low-level driver is cpu-endian, with the exception of payload 
data.  IEEE1394 doesn't specify an endianness for the payload data, even 
though most protocols use big-endian.    Some protocols have a mix of 
byte-arrays and be32 words (eg SBP-2) so it's up to the protocol to byteswap 
its data as appropriate.

> (Or in general, do not use bitfields period ....)
> 
> bitfields format is not guaranteed, and is not endian consistent. 

Ok... I was planning to make big-endian versions of the structs so that the 
endian issue would be solved.  But if the bit layout is not consistent, I 
guess bitfields are useless for wire formats.  I didn't know that though, I 
thought the C standard specified that the compiler should allocate bits out of 
a word using the lower bits first.  Is the problem that it allocates them out 
of a 64-bit word on 64-bit platforms?

cheers,
Kristian

Re: [PATCH 0/3] New firewire stack

[Ray Lee]

On 12/4/06, Kristian Høgsberg <krh@redhat.com> wrote:
> Ok... I was planning to make big-endian versions of the structs so that the
> endian issue would be solved.  But if the bit layout is not consistent, I
> guess bitfields are useless for wire formats.  I didn't know that though, I
> thought the C standard specified that the compiler should allocate bits out of
> a word using the lower bits first.

The C standard explicitly allows it to be implementation defined.
Having been bit by this exact problem, I can also recommend never
using bitfields for anything other than things kept solely in local
memory.

Ray

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Ray Lee wrote:
> On 12/4/06, Kristian Høgsberg <krh@redhat.com> wrote:
>> Ok... I was planning to make big-endian versions of the structs so 
>> that the
>> endian issue would be solved.  But if the bit layout is not consistent, I
>> guess bitfields are useless for wire formats.  I didn't know that 
>> though, I
>> thought the C standard specified that the compiler should allocate 
>> bits out of
>> a word using the lower bits first.
> 
> The C standard explicitly allows it to be implementation defined.
> Having been bit by this exact problem, I can also recommend never
> using bitfields for anything other than things kept solely in local
> memory.

Yeah, I just read that paragraph in K&R... sigh.  Bitfields make the code so 
readable, though :)  Anyway, I'll rewrite it to use good old shifting and masking.

Kristian

Re: [PATCH 0/3] New firewire stack

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Tue, 05 Dec 2006 16:42:42 +1100

>  - It's horribly broken in at least two area :
> 
>  DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!
> 
>  and
> 
>  Where do you handle endianness ? (no need to shout for
>  that one).
> 
> (Or in general, do not use bitfields period ....)

Yes, this is a show stopper, the endianness and
word-size/endian testing should have been done before
submission.

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Tue, 05 Dec 2006 16:42:42 +1100
> 
>>  - It's horribly broken in at least two area :
>>
>>  DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!
>>
>>  and
>>
>>  Where do you handle endianness ? (no need to shout for
>>  that one).
>>
>> (Or in general, do not use bitfields period ....)
> 
> Yes, this is a show stopper, the endianness and
> word-size/endian testing should have been done before
> submission.

I guess my mistake here was to present it as a patch submission.  I 
acknowledged in my cover letter that it wasn't feature complete and I'm not 
pushing for inclusion just yet.  I'm very much aware of the point that when 
replacing a subsystem like this, the new code has to be as good as the old 
code.  In that respect, the patches I posted are lacking in other areas 
(isochronous streaming is the big one) that will take more work to fix than 
just making it work on big-endian and 64-bit architectures.  It's still a work 
in progress.

Having said that, I've been working on this for a while now, and I wanted to 
announce the effort and open the discussion about replacing the old stack. 
Something about "release early, release often"... :)  Anyway, I've moved the 
portability issues to the top of my list now.

Kristian

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Kristian Høgsberg wrote:
> David Miller wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>  DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!

Actually we do so in some places of the existing FireWire drivers.
Didn't go wrong so far. :-)

>>>  Where do you handle endianness ? (no need to shout for
>>>  that one).
>>>
>>> (Or in general, do not use bitfields period ....)
>>
>> Yes, this is a show stopper, the endianness and
>> word-size/endian testing should have been done before
>> submission.
> 
> I guess my mistake here was to present it as a patch submission.  I
> acknowledged in my cover letter that it wasn't feature complete and I'm
> not pushing for inclusion just yet.  I'm very much aware of the point
> that when replacing a subsystem like this, the new code has to be as
> good as the old code.  In that respect, the patches I posted are lacking
> in other areas (isochronous streaming is the big one) that will take
> more work to fix than just making it work on big-endian and 64-bit
> architectures.  It's still a work in progress.
[...]

That's right; there are a few in-kernel features and, much more
importantly, userspace ABIs missing before Kristian's stack could
replace the old one.

The good news is that the ABIs are either hidden behind userspace
libraries or are deprecated and to be phased out next year anyway.

The bad news is that the old stack is internally not as cleanly modular
as it was initially targeted to be. This makes it difficult to replace
the stack in parts, particularly where isochronous protocols are
concerned. Maybe this becomes a non-issue once the old ABIs were
removed; although I suppose that Kristian would like to see his stack in
wider use much earlier than that. The asynchronous stuff (what's left of
it besides sbp2) should be easy to move over.
-- 
Stefan Richter
-=====-=-==- ==-- --=-=
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Benjamin Herrenschmidt]

On Tue, 2006-12-05 at 19:49 +0100, Stefan Richter wrote:
> Kristian Høgsberg wrote:
> > David Miller wrote:
> >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>  DO NOT USE BITFIELDS FOR DATA ON THE WIRE !!!
> 
> Actually we do so in some places of the existing FireWire drivers.
> Didn't go wrong so far. :-)

Yeah, because you used 

#if defined __BIG_ENDIAN_BITFIELD

and

#if defined __LITTLE_ENDIAN_BITFIELD

which relies on the fact that it -seems- that by luck, gcc only has two
representations around and they match little/big endian archs (though
have we verified that is always correct, especially between 32 and 64
bits archs ?)

It's still wrong to do.

Cheers,
Ben.

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Benjamin Herrenschmidt wrote:
(bitfields as accessors to on-the-wire data)
...
> relies on the fact that it -seems- that by luck, gcc only has two
> representations around and they match little/big endian archs (though
> have we verified that is always correct, especially between 32 and 64
> bits archs ?)

As you perhaps noticed, this is already used on very basic packets (the
selfIDs which are the first ones generated by PHYs after each bus
reset). Therefore I suppose that it works also on 64bit architectures
even though they are certainly less extensively tested than 32bit ones.
But since there are no guarantees for it to work as intended,...

> It's still wrong to do.

...I added it to my list of things to fix. Thanks,
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Marcel Holtmann]

Hi Kristian,

> I'm announcing an alternative firewire stack that I've been working on
> the last few weeks.  I'm aiming to implement feature parity with the
> current firewire stack, but not necessarily interface compatibility.
> For now, I have the low-level OHCI driver done, the mid-level
> transaction logic done, and the SBP-2 (storage) driver is basically
> done.  What's missing is a streaming interface (in progress) to allow
> reception and transmission of isochronous data and a userspace
> interface for controlling devices (much like raw1394 or libusb for
> usb).  I'm working out of this git repository:
> 
>   http://gitweb.freedesktop.org/?p=users/krh/juju.git
> 
> but I'll be sending 3 patches for review after this mail: first the
> core subsystem, then the OHCI driver and finally the SBP-2 (SCSI over
> firewire) driver.  For people who want to test this out, the easiest
> approach right now is to clone the git repo and run make.  This
> requires the kernel-devel RPM on Fedora Core; I'm sure other distros
> have a similar package.

can you please use drivers/firewire/ if you want to start clean or
aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
the directory path is not really helpful.

Regards

Marcel

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Marcel Holtmann wrote:
> Hi Kristian,
> 
>> I'm announcing an alternative firewire stack that I've been working on
>> the last few weeks.  I'm aiming to implement feature parity with the
...
> can you please use drivers/firewire/ if you want to start clean or
> aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> the directory path is not really helpful.

Yes, that's probably a better idea.  Do you see a problem with using fw_* as a 
prefix in the code though?  I don't see anybody using that prefix, but Stefan 
pointed out to me that it's often used to abbreviate firmware too.

Kristian

Re: [PATCH 0/3] New firewire stack

[Marcel Holtmann]

Hi Kristian,

> >> I'm announcing an alternative firewire stack that I've been working on
> >> the last few weeks.  I'm aiming to implement feature parity with the
> ...
> > can you please use drivers/firewire/ if you want to start clean or
> > aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> > the directory path is not really helpful.
> 
> Yes, that's probably a better idea.  Do you see a problem with using fw_* as a 
> prefix in the code though?  I don't see anybody using that prefix, but Stefan 
> pointed out to me that it's often used to abbreviate firmware too.

the only problem are public and exported interfaces and function. For
static code you can use whatever you want. I personally would go with
"ieee1394", because that is the official name for it. Otherwise go with
"firewire" if you wanna separate yourself from the previous stack.

Besides having "fw" for firmware it also is used in terms of firewall
and you don't wanna have that confusion.

Regards

Marcel

Re: [PATCH 0/3] New firewire stack

[Geert Uytterhoeven]

On Tue, 5 Dec 2006, Marcel Holtmann wrote:
> the only problem are public and exported interfaces and function. For
> static code you can use whatever you want. I personally would go with
> "ieee1394", because that is the official name for it. Otherwise go with
> "firewire" if you wanna separate yourself from the previous stack.

Which still leaves the opportunity for having a third stack in drivers/ilink
:-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Geert Uytterhoeven wrote:
> On Tue, 5 Dec 2006, Marcel Holtmann wrote:
>> I personally would go with
>> "ieee1394", because that is the official name for it. Otherwise go with
>> "firewire" if you wanna separate yourself from the previous stack.
> 
> Which still leaves the opportunity for having a third stack in drivers/ilink
> :-)

Wait for the real one, to appear in drivers/high_performance_serial_bus.
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Erik Mouw]

On Tue, Dec 05, 2006 at 10:13:55AM -0500, Kristian Høgsberg wrote:
> Marcel Holtmann wrote:
> >can you please use drivers/firewire/ if you want to start clean or
> >aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> >the directory path is not really helpful.
> 
> Yes, that's probably a better idea.  Do you see a problem with using fw_* 
> as a prefix in the code though?  I don't see anybody using that prefix, but 
> Stefan pointed out to me that it's often used to abbreviate firmware too.

So what about fiwi_*? If that's too close to wifi_*, try frwr_.


Erik

-- 
+-- Erik Mouw -- www.harddisk-recovery.com -- +31 70 370 12 90 --
| Lab address: Delftechpark 26, 2628 XH, Delft, The Netherlands

Re: [PATCH 0/3] New firewire stack

[Pavel Machek]

On Tue 05-12-06 17:05:30, Erik Mouw wrote:
> On Tue, Dec 05, 2006 at 10:13:55AM -0500, Kristian H?gsberg wrote:
> > Marcel Holtmann wrote:
> > >can you please use drivers/firewire/ if you want to start clean or
> > >aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> > >the directory path is not really helpful.
> > 
> > Yes, that's probably a better idea.  Do you see a problem with using fw_* 
> > as a prefix in the code though?  I don't see anybody using that prefix, but 
> > Stefan pointed out to me that it's often used to abbreviate firmware too.
> 
> So what about fiwi_*? If that's too close to wifi_*, try frwr_.

Ugly, but fwire could be acceptable.
							Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Pavel Machek wrote at linux-kernel:
> On Tue 05-12-06 17:05:30, Erik Mouw wrote:
>> On Tue, Dec 05, 2006 at 10:13:55AM -0500, Kristian H?gsberg wrote:
>> > Marcel Holtmann wrote:
>> > >can you please use drivers/firewire/ if you want to start clean or
>> > >aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
>> > >the directory path is not really helpful.
>> > 
>> > Yes, that's probably a better idea.  Do you see a problem with using fw_* 
>> > as a prefix in the code though?  I don't see anybody using that prefix, but 
>> > Stefan pointed out to me that it's often used to abbreviate firmware too.
>> 
>> So what about fiwi_*? If that's too close to wifi_*, try frwr_.
> 
> Ugly, but fwire could be acceptable.

How about this:
Let's let Kristian continue to work with his chosen fw_ prefix until his
drivers are ready to be pulled in (into the linux1394 repo and -mm),
then make a decision about prefixes. It's mostly a matter of running sed
over the files.

One thing is for sure, the fw_ prefix is not too well suited to stay
if/when Kristian's code is pushed to mainline. During a switch period,
we could e.g. replace the old stack's prefixes by hpsb1_ (as the 1st
generation of FireWire kernel APIs) or whatever and replace Kristian's
prefixes by hpsb_. I would actually like fw_ most (if it wasn't so
overloaded and already used in other contexts) and the author's decision
should be honored too.

Regarding the directory name, I favor to stick everything into
drivers/ieee1394 even if it could get crowded during a transition period.
-- 
Stefan Richter
-=====-=-==- ==-- -=---
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

On 12/8/06, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Pavel Machek wrote at linux-kernel:
> > On Tue 05-12-06 17:05:30, Erik Mouw wrote:
> >> On Tue, Dec 05, 2006 at 10:13:55AM -0500, Kristian H?gsberg wrote:
> >> > Marcel Holtmann wrote:
> >> > >can you please use drivers/firewire/ if you want to start clean or
> >> > >aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> >> > >the directory path is not really helpful.
> >> >
> >> > Yes, that's probably a better idea.  Do you see a problem with using fw_*
> >> > as a prefix in the code though?  I don't see anybody using that prefix, but
> >> > Stefan pointed out to me that it's often used to abbreviate firmware too.
> >>
> >> So what about fiwi_*? If that's too close to wifi_*, try frwr_.
> >
> > Ugly, but fwire could be acceptable.
>
> How about this:
> Let's let Kristian continue to work with his chosen fw_ prefix until his
> drivers are ready to be pulled in (into the linux1394 repo and -mm),
> then make a decision about prefixes. It's mostly a matter of running sed
> over the files.

Yeah, I'm not changing it just yet, but I'm not too attached to fw_
and I think that ieee1394_ will work better.  The modutil tools
already use ieee1394 for device_id tables.

> Regarding the directory name, I favor to stick everything into
> drivers/ieee1394 even if it could get crowded during a transition period.

Yeah, if we're going with the ieee1394 prefix, it'd make the most
sense if the files live in drivers/ieee1394.

Kristian

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Kristian Høgsberg wrote:
> On 12/8/06, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
...
>>>> On Tue, Dec 05, 2006 at 10:13:55AM -0500, Kristian H?gsberg wrote:
>>>>> Marcel Holtmann wrote:
>>>>>> can you please use drivers/firewire/ if you want to start clean or
>>>>>> aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
>>>>>> the directory path is not really helpful.
>>>>>
>>>>> Yes, that's probably a better idea.  Do you see a problem with using fw_*
>>>>> as a prefix in the code though?  I don't see anybody using that prefix, but
>>>>> Stefan pointed out to me that it's often used to abbreviate firmware too.
[...]
> I'm not changing it just yet, but I'm not too attached to fw_
> and I think that ieee1394_ will work better.  The modutil tools
> already use ieee1394 for device_id tables.
[...]

Alas the length of "ieee1394_" gets in the way of readability.


There are currently no exported symbols starting with "fw_":

linux-2.6.19 $ find . -type f -exec grep -e 'EXPORT.*fw_' {} \;
EXPORT_SYMBOL(iop_fw_load_spu);
EXPORT_SYMBOL(iop_fw_load_mpu);
EXPORT_SYMBOL(hostap_check_sta_fw_version);
EXPORT_SYMBOL(mpt_alloc_fw_memory);
EXPORT_SYMBOL(mpt_free_fw_memory);


There are a few names of struct members starting with "fw_" and four
global variables on the MIPS architecture:

linux-2.6.19 $ find include/ -type f -exec grep -e '[ ]fw_' {} \;
        __u64 fw_ver;
            unsigned char fw_mpx_cap; /* forward multiplexing capability */
        unsigned long fw_vendor;        /* physical addr of CHAR16 vendor string */
        u32 fw_revision;
        int             fw_rev[MAX_BOARD];
        char fw_version[2];                /* major = [0], minor = [1] */
        u64 fw_flags;
extern unsigned long fw_arg0, fw_arg1, fw_arg2, fw_arg3;
linux-2.6.19 $ find include/ -type f -exec grep -l '[ ]fw_' {} \;
include/rdma/ib_user_verbs.h
include/linux/atmsap.h
include/linux/efi.h
include/linux/cyclades.h
include/sound/snd_wavefront.h
include/asm-alpha/gct.h
include/asm-mips/bootinfo.h


There are a few constants starting with "FW_":

linux-2.6.19 $ find include/ -type f -exec grep -e '[ ]FW_' {} \;|wc -l
38
linux-2.6.19 $ find include/ -type f -exec grep -l '[ ]FW_' {} \;
include/linux/firmware.h
include/video/radeon.h
include/asm-powerpc/pmac_feature.h
include/asm-powerpc/firmware.h
include/asm-ppc/residual.h


Since cross-subsystem drivers like sbp2 (a SCSI driver) and eth1394 (a
networking driver) live in linux/drivers/ieee1394/, the IEEE 1394 subsystem
does not need to ship header files in linux/include/, except for the minimum
stuff in linux/include/linux/mod_devicetable.h. Therefore the only conflicts
that the IEEE 1394 subsystem code could create would be via its EXPORTs.

I would therefore prefer "fw_" or "hpsb_" over any of the other suggestions
made here:
  - ieee1394_ makes sense in linux/mod_devicetable.h but is too long
    otherwise.
  - fiwi_, frwr_, and fwire_ are artificial abbreviations which come very
    unnatural. (fw_ is an artificial abbreviation too but is not as awkward
    as the others. hpsb_ is not just an abbreviation, it is an established
    acronym of the canonical name of the bus.)

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

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Stefan Richter wrote:
> Kristian Høgsberg wrote:
...
>> I'm not changing it just yet, but I'm not too attached to fw_
>> and I think that ieee1394_ will work better.  The modutil tools
>> already use ieee1394 for device_id tables.
> [...]
> 
> Alas the length of "ieee1394_" gets in the way of readability.

It's not too bad and it's only for exported symbols:

[krh@dinky fw]$ grep EXPORT *.c | wc -l
27

and using the same prefix as the device_id struct will be nice.  When I 
submitted the ieee1394_device_id patch I originally proposed hpsb_device_id, 
but nobody knew what that meant so we went with the ieee1394_device_id we have 
now.  Oh, and net/ieee80211 uses ieee80211 as prefix, so it wont be the 
longest subsytem prefix :).  Plus I want to go throught the list of exported 
symbols, some of the names can be trimmed a bit.

Having said that, using drivers/firewire and the fw_ prefix, as Marcel 
suggests, works too.  It's what bluetooth and infiniband does, so there is 
some precedence there.

...
> I would therefore prefer "fw_" or "hpsb_" over any of the other suggestions
> made here:
>   - ieee1394_ makes sense in linux/mod_devicetable.h but is too long
>     otherwise.
>   - fiwi_, frwr_, and fwire_ are artificial abbreviations which come very
>     unnatural. (fw_ is an artificial abbreviation too but is not as awkward
>     as the others. hpsb_ is not just an abbreviation, it is an established
>     acronym of the canonical name of the bus.)

Oh, I don't know... for the longest time I didn't know what hpsb meant, and 
high performance serial bus is pretty generic sounding... are we talking about 
usb, sata, ieee1394 or rs232?  Ok, I guess rs232 is neither hp or b.  But 
seriously, except for the current stack, I've never seen the hpsb abbreviation 
used much.

Kristian

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Kristian Høgsberg wrote:
> Stefan Richter wrote:
>>     as the others. hpsb_ is not just an abbreviation, it is an established
>>     acronym of the canonical name of the bus.)
> 
> Oh, I don't know... for the longest time I didn't know what hpsb meant,
> and high performance serial bus is pretty generic sounding... are we
> talking about usb, sata, ieee1394 or rs232?  Ok, I guess rs232 is
> neither hp or b.  But seriously, except for the current stack, I've
> never seen the hpsb abbreviation used much.

Sure, by "established" I merely meant "established for and by usage
in Linux kernel sources", nothing more. Also, High Performance Serial
Bus is not the only generic sounding name of a bus or architecture.
How about Small Computer System Interface for example? Universal Serial
Bus and Serial Advanced Technology Attachment aren't much more colorful
either. Granted, their acronyms are prettier than HPSB, but not as
elitist. ;-)
-- 
Stefan Richter
-=====-=-==- ==-- -=-==
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Benjamin Herrenschmidt]

> One thing is for sure, the fw_ prefix is not too well suited to stay
> if/when Kristian's code is pushed to mainline. During a switch period,
> we could e.g. replace the old stack's prefixes by hpsb1_ (as the 1st
> generation of FireWire kernel APIs) or whatever and replace Kristian's
> prefixes by hpsb_. I would actually like fw_ most (if it wasn't so
> overloaded and already used in other contexts) and the author's decision
> should be honored too.

One of the problems with hpsb_ is that it's a total pain to type and
doesn't mean anything at first sight :-)

Ben.

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Benjamin Herrenschmidt wrote:
> One of the problems with hpsb_ is that it's a total pain to type
> and doesn't mean anything at first sight :-)

Both prevent that other people snatch this prefix from us. Also, only
people who can recite the meaning of that acronym when asleep are
permitted to call hpsb_ functions in their code. :-)
-- 
Stefan Richter
-=====-=-==- ==-- -=--=
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Marcel Holtmann]

Hi Erik,

> > >can you please use drivers/firewire/ if you want to start clean or
> > >aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
> > >the directory path is not really helpful.
> > 
> > Yes, that's probably a better idea.  Do you see a problem with using fw_* 
> > as a prefix in the code though?  I don't see anybody using that prefix, but 
> > Stefan pointed out to me that it's often used to abbreviate firmware too.
> 
> So what about fiwi_*? If that's too close to wifi_*, try frwr_.

please don't. These kind of abbreviations make my brain tilt. For the
directory name you basically should use the full name. In this case it
will be drivers/ieee1394/ or drivers/firewire/. Nothing else is really
acceptable and if you look at other subsystems, you will see that they
always use the long name.

For the exported public functions you might wanna use abbreviations, but
in general I don't recommend it. And normally we only talk about a
limited functions that are needed to be exposed via EXPORT_SYMBOL.

Regards

Marcel

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Marcel Holtmann wrote:
> Hi Erik,
> 
>>>> can you please use drivers/firewire/ if you want to start clean or
>>>> aiming at replacing drivers/ieee1394/. Using "fw" as an abbreviation in
>>>> the directory path is not really helpful.
>>> Yes, that's probably a better idea.  Do you see a problem with using fw_* 
>>> as a prefix in the code though?  I don't see anybody using that prefix, but 
>>> Stefan pointed out to me that it's often used to abbreviate firmware too.
>> So what about fiwi_*? If that's too close to wifi_*, try frwr_.
> 
> please don't. These kind of abbreviations make my brain tilt. For the
> directory name you basically should use the full name. In this case it
> will be drivers/ieee1394/ or drivers/firewire/. Nothing else is really
> acceptable and if you look at other subsystems, you will see that they
> always use the long name.
> 
> For the exported public functions you might wanna use abbreviations, but
> in general I don't recommend it. And normally we only talk about a
> limited functions that are needed to be exposed via EXPORT_SYMBOL.

I think I'll stick with my fw_* prefix for now, it's nice and short and not 
too cryptic.  I'm only exporting a small set of functions anyway, and they're 
all only used inside the firewire stack.

Kristian

Re: [PATCH 0/3] New firewire stack

[Alexey Dobriyan]

On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
> I'm announcing an alternative firewire stack that I've been working on
> the last few weeks.

Is mainline firewire so hopeless, that you've decided to rewrite it?
Could you show some ugly places in it?

We can end up with two not quite working sets of firewire drivers your
way.

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Alexey Dobriyan wrote:
> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
>> I'm announcing an alternative firewire stack that I've been working on
>> the last few weeks.
> 
> Is mainline firewire so hopeless, that you've decided to rewrite it?
> Could you show some ugly places in it?

Although that's for Kristian to answer (and much of the answer can
already be found in his posting), here are some ugly things.
http://bugzilla.kernel.org/buglist.cgi?product=Drivers&component=IEEE1394&bug_status=NEW&bug_status=ASSIGNED&bug_status=NEEDINFO
Or look here first: http://bugzilla.kernel.org/show_bug.cgi?id=6070

There have been severe issues with the FireWire drivers during a certain
period, due to lack of care. That's partly a thing of the past, and I
definitely don't consider the mainline drivers hopeless. (Otherwise I
hadn't stepped in as maintainer.) But we still lack manpower for
bugfixing. Also, the bugs which are left now are the ones that are the
hardest to find and fix. Therefore I am glad that Kristian is back again
and is contributing some real work. (For those who don't know him, he
has worked on the drivers in the past, long before I did.)

> We can end up with two not quite working sets of firewire drivers your
> way.

As long as I will be interested in maintenance of the FireWire drivers,
I intend to help that either a successful switch to the new stack is put
into practice, or that Kristian's designs and implementation are copied
where they benefit the old stack. I'm not sure which way it will go; it
depends on (1) who contributes what and (2) the shape in which
mainline's FireWire stack will be in in 2007. Right now it lacks modularity.
-- 
Stefan Richter
-=====-=-==- ==-- --=-=
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Alexey Dobriyan wrote:
> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
>> I'm announcing an alternative firewire stack that I've been working on 
>> the last few weeks.
> 
> Is mainline firewire so hopeless, that you've decided to rewrite it? Could
> you show some ugly places in it?

Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
introduce regressions and instability for a little while.

My main point about ohci1394 (the old stacks PCI driver) is, that if you
really want to fix the issues with this driver, you have to shuffle the code
around so much that you'll introduce as many regressions as a clean rewrite.
The big problems in the ohci1394 drivers is the irq_handler, bus reset
handling and config rom handling.  These are some of the strong points of
fw-ohci.c:

  - Rock solid handling of generations and node IDs around bus resets.
    The only way to handle this atomically is to pass the generation
    count along all the way to the transmit function in the low-level
    driver.  The linux1394 low level driver API is broken in this
    respect.

  - Better handling of self ID receive and possible recursive bus
    resets.  Successive bus resets could overwrite the self ID DMA
    buffer, while we read out the contents.  The OHCI specification
    recommends a method similiar to linux/seqlock.h for reading out
    self IDs, to ensure we get a consistent result.

  - Much simpler bus reset handling; we only subscribe to the
    selfIDComplete interrupt and don't use the troublesome busReset
    interrupt.  Rely on async transmit context to not send data while
    busReset event bit is set.

  - Atomic updates of config rom contents as specified in section 5.5.6
    in the OHCI specification. The contents of the ConfigROMheader,
    BusOptions and ConfigROMmap registers are updated atomically by the
    controller after a reset.

The OHCI specification describes a number of the techniques to ensure race
free operation for the above cases, but the ohci1394 driver generally doesn't
use any of these.  If you want to see ugly code look at the ohci1394 irq
handler.  Much of the uglyness comes from trying to handle the busReset
interrupt, so that the mid-level linux1394 stack can fail I/O while the bus
reset takes place.  Now, OHCI hardware already reliably fails I/O during bus
reset, so there is no need to complicate the core stack with this extra state,
and the OHCI driver becomes much simpler and more reliable, since we now just
need to know when a bus reset has successfully completed.

Fixing this problem requires significant changes to the ohci1394 driver and
the mid-level stack, and will destabilize things until we've figure out how to
work around the odd flaky device out there.  Similar problems exists related
to sending packets without bus reset races, updating the config rom, and
reporting self ID packets and all require significant changes to the core
stack and ohci1394.  All taken together the scale tips towards a rewrite.

Another point is the various streaming drivers.  There used to be 5 different
userspace streaming APIs in the linux1394: raw1394, video1394, amdtp, dv1394
and rawiso.  Recently, amdtp (audio streaming) has been removed, since with
the rawiso interface, this can be done in userspace.  However the remaining 4
interfaces have slightly disjoint feature sets and can't really be phased out.
  In the long run, supporting 4 different interfaces that does almost the same
thing isn't feasible.  The streaming interface in my new stack (only
transmission implemented at this point) can replace all of these interfaces.

Finally, some parts aren't actually rewritten, just ported over and
refactored.  This is the case for the SBP-2 driver.  Functionally, my
fw-sbp2.c is identical to sbp2.c in the current stack, but I've changed it to
work with the new interfaces and cleaned up some of the redundancy.

> We can end up with two not quite working sets of firewire drivers your way.
> 
You can patch up the current stack to be less flaky, and Stefan has been doing
a great job at that lately, but it's still fundamentally broken in the ways
described above.

While my stack may less stable for the first couple of weeks, these are
transient issues, such as, say, lack of big endian testing, that are easily
fixed.  In the long run this new stack is much more maintainable and has a
bigger potential for stability.

Kristian

Re: [PATCH 0/3] New firewire stack

[Ben Collins]

On Tue, 2006-12-05 at 18:21 -0500, Kristian Høgsberg wrote:
> Alexey Dobriyan wrote:
> > On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
> >> I'm announcing an alternative firewire stack that I've been working on 
> >> the last few weeks.
> > 
> > Is mainline firewire so hopeless, that you've decided to rewrite it? Could
> > you show some ugly places in it?
> 
> Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
> introduce regressions and instability for a little while.
> 
> My main point about ohci1394 (the old stacks PCI driver) is, that if you
> really want to fix the issues with this driver, you have to shuffle the code
> around so much that you'll introduce as many regressions as a clean rewrite.
> The big problems in the ohci1394 drivers is the irq_handler, bus reset
> handling and config rom handling.  These are some of the strong points of
> fw-ohci.c:

My main concern is that when I picked up ieee1394 maint myself, it was
because it was not big-endian or 64-bit friendly. I spent the better
part of 3 months getting this right on PPC and UltraSPARC. Not because
it's hard to fix these issues, but because the hardware is not well
defined for a lot of these cases (I know you've seen the ohci1394 code
to handle endianness).

So while I can understand that ieee1394 doesn't have much man power
right now, and that there are some hard to find bugs in the current
tree, I can't see how starting from scratch alleviates this.

The tree is years old, and a lot of work has been put into it (lots of
my work, I'll admit I'm being a little protective). I'm not sure that
"replacing" it is wise, or even needed. Fork it, clean it up, but
rewriting just doesn't make sense.

Granted, this is your time, and you can spend it how you want, I just
don't want to see the ieee1394 stack take a step backwards in the hopes
that it will be better a year down the road.

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

(Adding Cc: linux1394-devel)

Ben Collins wrote at linux-kernel:
> On Tue, 2006-12-05 at 18:21 -0500, Kristian Høgsberg wrote:
>> Alexey Dobriyan wrote:
>>> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
>>>> I'm announcing an alternative firewire stack that I've been working on 
>>>> the last few weeks.
>>> Is mainline firewire so hopeless, that you've decided to rewrite it? Could
>>> you show some ugly places in it?
>> Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
>> introduce regressions and instability for a little while.
>>
>> My main point about ohci1394 (the old stacks PCI driver) is, that if you
>> really want to fix the issues with this driver, you have to shuffle the code
>> around so much that you'll introduce as many regressions as a clean rewrite.
>> The big problems in the ohci1394 drivers is the irq_handler, bus reset
>> handling and config rom handling.  These are some of the strong points of
>> fw-ohci.c:
> 
> My main concern is that when I picked up ieee1394 maint myself, it was
> because it was not big-endian or 64-bit friendly.

I would like to see new development efforts take cleanliness WRT host
byte order and 64bit architectures into account from the ground up. (I
understand though why Kristian made the announcement in this early
phase, and I agree with him that this kind of development has to go into
the open early.)

> I spent the better
> part of 3 months getting this right on PPC and UltraSPARC. Not because
> it's hard to fix these issues, but because the hardware is not well
> defined for a lot of these cases (I know you've seen the ohci1394 code
> to handle endianness).
> 
> So while I can understand that ieee1394 doesn't have much man power
> right now, and that there are some hard to find bugs in the current
> tree, I can't see how starting from scratch alleviates this.
> 
> The tree is years old, and a lot of work has been put into it (lots of
> my work, I'll admit I'm being a little protective). I'm not sure that
> "replacing" it is wise, or even needed. Fork it, clean it up, but
> rewriting just doesn't make sense.
> 
> Granted, this is your time, and you can spend it how you want, I just
> don't want to see the ieee1394 stack take a step backwards in the hopes
> that it will be better a year down the road.

No matter in which way we will put Kristian's work to use, regressions
must not happen --- beyond the measure that unintentionally happens when
bugs get fixed. Mainline's FireWire stack lost a lot of trust at
end-users and application developers because of periods of sometimes
very visible regressions.
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/

Re: Re: [PATCH 0/3] New firewire stack

[Alexander Neundorf]

Hi,

Von: Stefan Richter <stefanr@s5r6.in-berlin.de>

...
> bugs get fixed. Mainline's FireWire stack lost a lot of trust at
> end-users and application developers because of periods of sometimes
> very visible regressions.

For us it's working well, with no major problems (there was a problem with SMP kernels and the arm mapping, but my kernel is not recent and I didn't find the time yet to update to current versions, so I could not report the bug). We have customers and it works for them.

OTOH I heard from some people who wanted to use the 1394 stack for embedded devices without PCI and they didn't succeed to add support for their selected chipset. 

Bye
Alex

-- 
Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen! 
Ideal für Modem und ISDN: http://www.gmx.net/de/go/smartsurfer

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Alexander Neundorf wrote:
> Von: Stefan Richter <stefanr@s5r6.in-berlin.de>
>> Mainline's FireWire stack lost a lot of trust
...
> For us it's working well, with no major problems (there was a problem with
> SMP kernels and the arm mapping, but my kernel is not recent and I didn't
> find the time yet to update to current versions, so I could not report the
> bug). We have customers and it works for them.

Perhaps the fix which was released in 2.6.19 is relevant. As always, you
can get it as part of my patchkits too which are currently available for
2.6.16.x and 2.6.18(.x). I had also patchkits for 2.6.1[457].x which I
could revive on request. If need be, I would also try to assist
distributors to identify and backport specific fixes. I am currently
wondering if I should take the time to pick out a collection of fixes
for Adrian's 2.6.16.x series.

> OTOH I heard from some people who wanted to use the 1394 stack for embedded
> devices without PCI and they didn't succeed to add support for their selected
> chipset. 

The ieee1394 core currently dependends on the PCI subsystem for no
obvious reason. The fix probably consists mostly of a rather trivial
conversion from the PCI DMA mapping API to generic DMA mapping. I
actually intend to do this conversion RSN.

Another question is whether the stack-internal APIs are really fit for
non-OHCI chips. There is an unfinished low-level driver for GP2Lynx
which worked to some degree at some point, but other than that I don't
remember positive or negative reports in this department. Maybe proper
documentation of the stack-internal APIs would already help embedded
developers a lot. Furthermore, there may be question marks WRT
interaction of the FireWire stack with architecture specific kernel code.

But back to the subject matter: Clearly, Kristian concentrates on
PCI/OHCI-1394 hardware at the moment. If embedded developers have
specific requirements on the FireWire stack's design, they should IMO
contribute with a list of requirements or maybe even with patches.
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Stefan Richter wrote:
...
> Another question is whether the stack-internal APIs are really fit for
> non-OHCI chips. There is an unfinished low-level driver for GP2Lynx
> which worked to some degree at some point, but other than that I don't
> remember positive or negative reports in this department. Maybe proper
> documentation of the stack-internal APIs would already help embedded
> developers a lot. Furthermore, there may be question marks WRT
> interaction of the FireWire stack with architecture specific kernel code.

I think some of the problems with the current stack come from the fact that it 
was originally written (by Andreas Bombe) for the PCILynx chipset, in other 
words, *not* for the OHCI chipset.  The PCILynx chipset is a much lower level 
chipset, it offloads much more to software.  For example, each self ID is 
received as an individual packet, where the OHCI chipset receives these into a 
special buffer and notifies software when it has received a consistent set of 
packets.  The current stack has a callback for the host controller driver to 
call once the bus reset phase starts, a callback for each received self ID and 
a callback to indicate the end of the bus reset phase.

In the new stack, the controller/core interface is more suited for the OHCI 
controller.  The stack doens't go into a bus reset state, and all self IDs are 
reported as an atomic event.  This makes the upper layers much simpler, suits 
the OHCI controller better, and should only require a few lines extra code in 
a PCILynx driver to buffer up the self IDs.  And it's arguably better to have 
the PCILynx driver do this than have the OHCI controller split up and 
otherwise atomic event.

> But back to the subject matter: Clearly, Kristian concentrates on
> PCI/OHCI-1394 hardware at the moment. If embedded developers have
> specific requirements on the FireWire stack's design, they should IMO
> contribute with a list of requirements or maybe even with patches.

It's true that I'm developing for PCI+OHCI, but I've kept the controller/core 
stack split that the old stack has, nothing outside the OHCI driver depends on 
PCI (I'm using the generic DMA API).  I've shifted the abstraction level up a 
bit for the controller interface, which makes sense in general, but also since 
this is what every desktop or laptop out there has.  That said, I don't think 
anything in the stack design will break for embedded/non-OHCI chipsets.

cheers,
Kristian

Re: [PATCH 0/3] New firewire stack

[Ben Collins]

On Wed, 2006-12-06 at 09:56 +0100, Stefan Richter wrote:
> (Adding Cc: linux1394-devel)
> 
> Ben Collins wrote at linux-kernel:
> > On Tue, 2006-12-05 at 18:21 -0500, Kristian Høgsberg wrote:
> >> Alexey Dobriyan wrote:
> >>> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
> >>>> I'm announcing an alternative firewire stack that I've been working on 
> >>>> the last few weeks.
> >>> Is mainline firewire so hopeless, that you've decided to rewrite it? Could
> >>> you show some ugly places in it?
> >> Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
> >> introduce regressions and instability for a little while.
> >>
> >> My main point about ohci1394 (the old stacks PCI driver) is, that if you
> >> really want to fix the issues with this driver, you have to shuffle the code
> >> around so much that you'll introduce as many regressions as a clean rewrite.
> >> The big problems in the ohci1394 drivers is the irq_handler, bus reset
> >> handling and config rom handling.  These are some of the strong points of
> >> fw-ohci.c:
> > 
> > My main concern is that when I picked up ieee1394 maint myself, it was
> > because it was not big-endian or 64-bit friendly.
> 
> I would like to see new development efforts take cleanliness WRT host
> byte order and 64bit architectures into account from the ground up. (I
> understand though why Kristian made the announcement in this early
> phase, and I agree with him that this kind of development has to go into
> the open early.)

And yet endianness is not the focus from the ground up in Kristian's
work. That was my point.

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Ben Collins wrote:
...
>> I would like to see new development efforts take cleanliness WRT host
>> byte order and 64bit architectures into account from the ground up. (I
>> understand though why Kristian made the announcement in this early
>> phase, and I agree with him that this kind of development has to go into
>> the open early.)
> 
> And yet endianness is not the focus from the ground up in Kristian's
> work. That was my point.

I don't know what you base this on, it's not true.  Everything outside 
fw-ohci.c is endian aware and I know the two things I need to look into for 
fw-ohci: DMA programs and IEEE1394 headers.  My plan was to develop the stack 
towards feature completeness and then test on big-endian and 64-bit platforms.

If you're thinking of the bitfield problem BenH pointed out, that doesn't 
imply I didn't have endianess in mind when writing the code.  As Stefan 
already mentioned, we use bitfields for wire data in the current stack.  We 
have to sets of structs, one for big endian architectures and one for little 
endian architectures.  My plan was to write big endian versions of these 
structs and then test on various architectures.

So my bitfield approach doesn't work and I haven't gotten around to doing the 
big-endian testing yet, but don't mistake that for lack of endianess 
awareness.  Of course, big endian and 64-bit architectures *must* work, but I 
contend that it can impact the overall design of the stack.  It's a detail 
that you need to get right, not a design principle.  But let's not argue this 
further, I'll post a new set of patches in a few days that work on big-endian 
and 64-bit.

cheers,
Kristian

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

(Adding Cc: linux1394-devel)

Kristian Høgsberg wrote to linux-kernel:
> Alexey Dobriyan wrote:
>> On Tue, Dec 05, 2006 at 12:22:29AM -0500, Kristian Høgsberg wrote:
>>> I'm announcing an alternative firewire stack that I've been working
>>> on the last few weeks.
>>
>> Is mainline firewire so hopeless, that you've decided to rewrite it?
>> Could you show some ugly places in it?
> 
> Yes.  I'm not doing this lightheartedly.  It's a lot of work and it will
> introduce regressions and instability for a little while.
> 
> My main point about ohci1394 (the old stacks PCI driver) is, that if you
> really want to fix the issues with this driver, you have to shuffle the code
> around so much that you'll introduce as many regressions as a clean rewrite.
> The big problems in the ohci1394 drivers is the irq_handler, bus reset
> handling and config rom handling.  These are some of the strong points of
> fw-ohci.c:
> 
>  - Rock solid handling of generations and node IDs around bus resets.
>    The only way to handle this atomically is to pass the generation
>    count along all the way to the transmit function in the low-level
>    driver.  The linux1394 low level driver API is broken in this
>    respect.
> 
>  - Better handling of self ID receive and possible recursive bus
>    resets.  Successive bus resets could overwrite the self ID DMA
>    buffer, while we read out the contents.  The OHCI specification
>    recommends a method similiar to linux/seqlock.h for reading out
>    self IDs, to ensure we get a consistent result.
> 
>  - Much simpler bus reset handling; we only subscribe to the
>    selfIDComplete interrupt and don't use the troublesome busReset
>    interrupt.  Rely on async transmit context to not send data while
>    busReset event bit is set.
> 
>  - Atomic updates of config rom contents as specified in section 5.5.6
>    in the OHCI specification. The contents of the ConfigROMheader,
>    BusOptions and ConfigROMmap registers are updated atomically by the
>    controller after a reset.
> 
> The OHCI specification describes a number of the techniques to ensure race
> free operation for the above cases, but the ohci1394 driver generally doesn't
> use any of these.  If you want to see ugly code look at the ohci1394 irq
> handler.  Much of the uglyness comes from trying to handle the busReset
> interrupt, so that the mid-level linux1394 stack can fail I/O while the bus
> reset takes place.  Now, OHCI hardware already reliably fails I/O during bus
> reset, so there is no need to complicate the core stack with this extra state,
> and the OHCI driver becomes much simpler and more reliable, since we now just
> need to know when a bus reset has successfully completed.
> 
> Fixing this problem requires significant changes to the ohci1394 driver and
> the mid-level stack, and will destabilize things until we've figure out how to
> work around the odd flaky device out there.  Similar problems exists related
> to sending packets without bus reset races, updating the config rom, and
> reporting self ID packets and all require significant changes to the core
> stack and ohci1394.  All taken together the scale tips towards a rewrite.
> 
> Another point is the various streaming drivers.  There used to be 5 different
> userspace streaming APIs in the linux1394: raw1394, video1394, amdtp, dv1394
> and rawiso.  Recently, amdtp (audio streaming) has been removed, since with
> the rawiso interface, this can be done in userspace.  However the remaining 4
> interfaces have slightly disjoint feature sets and can't really be phased out.

The old iso API of (lib)raw1394 has been marked deprecated and
undocumented in libraw1394's documentation for some time, and will go
away in 2007.

Dv1394 might go away in 2007 too if there is enough effort to move
high-profile users over to rawiso a.k.a. the current iso API of
(lib)raw1394.

I suppose video1394 might get a viable migration path with your new
driver, if you and interested developers put effort into development
(and help with deployment) of a proper replacement.

>  In the long run, supporting 4 different interfaces that does almost the same
> thing isn't feasible.  The streaming interface in my new stack (only
> transmission implemented at this point) can replace all of these interfaces.

You have to look at the matter not only from the POV of API design but
also of deployment and support.

> Finally, some parts aren't actually rewritten, just ported over and
> refactored.  This is the case for the SBP-2 driver.  Functionally, my
> fw-sbp2.c is identical to sbp2.c in the current stack, but I've changed it to
> work with the new interfaces and cleaned up some of the redundancy.
> 
>> We can end up with two not quite working sets of firewire drivers your
>> way.
>>
> You can patch up the current stack to be less flaky, and Stefan has been doing
> a great job at that lately, but it's still fundamentally broken in the ways
> described above.
> 
> While my stack may less stable for the first couple of weeks, these are
> transient issues, such as, say, lack of big endian testing, that are easily
> fixed.  In the long run this new stack is much more maintainable and has a
> bigger potential for stability.
> 
> Kristian
> 

I have to say, the really really old bug reports which piled up for
ohci1394 (high latency of the reset event handler, streaming packets
being mistaken as selfID packets...) and the recently reported ohci1394
bugs (event mask being mysteriously blanked out...) and my lack of
progress with these speak for themselves. (I don't have enough insight
yet, nor enough spare time nor affected hardware to make reasonable
headway.)
-- 
Stefan Richter
-=====-=-==- ==-- --==-
http://arcgraph.de/sr/

Re: [PATCH 0/3] New firewire stack

[Kristian Høgsberg]

Stefan Richter wrote:
...
>> Another point is the various streaming drivers.  There used to be 5 different
>> userspace streaming APIs in the linux1394: raw1394, video1394, amdtp, dv1394
>> and rawiso.  Recently, amdtp (audio streaming) has been removed, since with
>> the rawiso interface, this can be done in userspace.  However the remaining 4
>> interfaces have slightly disjoint feature sets and can't really be phased out.
> 
> The old iso API of (lib)raw1394 has been marked deprecated and
> undocumented in libraw1394's documentation for some time, and will go
> away in 2007.
> 
> Dv1394 might go away in 2007 too if there is enough effort to move
> high-profile users over to rawiso a.k.a. the current iso API of
> (lib)raw1394.
> 
> I suppose video1394 might get a viable migration path with your new
> driver, if you and interested developers put effort into development
> (and help with deployment) of a proper replacement.

As discussed on linux1394-devel, it may be possible to do a thin video1394 
compatibility driver for this one, but since the biggest user of this 
interface is libdc1394, it is probably better to just write a new iso 
streaming backend for this library. libdc1394 already supports different 
streaming backends.  For non-libdc1394 users of video1394, the interface I'm 
providing is very close to the video1394 ioctl interface, so porting should be 
easy enough.

>>  In the long run, supporting 4 different interfaces that does almost the same
>> thing isn't feasible.  The streaming interface in my new stack (only
>> transmission implemented at this point) can replace all of these interfaces.
> 
> You have to look at the matter not only from the POV of API design but
> also of deployment and support.

My POV here *is* about deployment and support, but from the kernel side of 
things.  If you commit yourself to long time support for the firewire stack, 
would you prefer 4 slightly different streaming drivers with different user 
space interfaces, or just one userspace driver with one userspace interface, 
that enables the 4 different types of streaming to be done in userspace?  The 
design of the streaming interfaces have been focused on enabling all these 
ad-hoc, in-kernel drivers to move to userspace, to make it feasible to 
actually support the stack.

Kristian

Re: [PATCH 0/3] New firewire stack

[Stefan Richter]

Kristian Høgsberg wrote:
> Stefan Richter wrote:
>> You have to look at the matter not only from the POV of API design but
>> also of deployment and support.
> 
> My POV here *is* about deployment and support, but from the kernel side
> of things.  If you commit yourself to long time support for the firewire
> stack, would you prefer 4 slightly different streaming drivers with
> different user space interfaces, or just one userspace driver with one
> userspace interface, that enables the 4 different types of streaming to
> be done in userspace?

My own preference certainly came across during in the recent thread on
linux1394-devel about deprecation dates... :-)

> The design of the streaming interfaces have been
> focused on enabling all these ad-hoc, in-kernel drivers to move to
> userspace, to make it feasible to actually support the stack.

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

Re: [PATCH 0/3] New firewire stack

[Olaf Hering]

On Tue, Dec 05, Kristian Høgsberg wrote:

> I'm announcing an alternative firewire stack that I've been working on

I suggest you hash out the most obvious bugs in -mm.
Once it you have it in a reasonable shape, replace the drivers in
drivers/ieee1394 in one go.
Its just not worth the pain to switch from module a.ko to module b.ko,
keep the name a.ko because its the very same functionality after all.