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@xxxxxxxxxxxxx>");
> > +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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/