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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <[email protected]>");
+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.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux