Re: PCI driver

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

Manu Abraham wrote:
>Rolf Eike Beer wrote:
>>That is true, but you should not call pci_get_device() in this function at
>>all.
>
>I reworked the whole thing over  .. but i am feeling that something's a
>bit wrong somewhere ..
>
>The log i get on a load - unload ..
>
>[  102.261264] mantis_pci_probe: Got a device
>[  102.262852] mantis_pci_probe: We got an IRQ
>[  102.264392] mantis_pci_probe: We finally enabled the device
>[  102.266020] Mantis Rev 1, irq: 23, latency: 32
>[  102.266118]          memory: 0xefeff000, mmio: f9218000
>[  102.269162] Trying to free free IRQ23
>[  110.297341] mantis_pci_remove: Removing -->Mantis irq: 23,
>latency: 32
>[  110.297344]  memory: 0xefeff000, mmio: 0xf9218000
>[  110.301326] Trying to free free IRQ23
>[  110.303445] Trying to free nonexistent resource <efeff000-efefffff>
>
>
>#include <asm/io.h>
>#include <asm/pgtable.h>
>#include <asm/page.h>
>#include <linux/interrupt.h>
>#include <linux/kmod.h>
>#include <linux/vmalloc.h>
>#include <linux/init.h>
>#include <linux/sched.h>
>#include <linux/device.h>
>#include "mantis_common.h"
>#include "mantis_dma.h"
>#include "mantis_i2c.h"
>#include "mantis_eeprom.h"
>
>#define DRIVER_NAME                "Mantis"
>
>static struct pci_device_id mantis_pci_table[] = {
>    { PCI_DEVICE(PCI_VENDOR_ID_MANTIS, PCI_DEVICE_ID_MANTIS_R11) },
>    { 0 },
>};
>
>MODULE_DEVICE_TABLE(pci, mantis_pci_table);
>
>static irqreturn_t mantis_pci_irq(int irq, void *dev_id, struct pt_regs
>*regs)
>{
>    struct mantis_pci *mantis;
>
>    dprintk(verbose, MANTIS_DEBUG, 1, "Mantis PCI IRQ");
>    mantis = (struct mantis_pci *) dev_id;
>    if (mantis == NULL)
>        dprintk(verbose, MANTIS_DEBUG, 1, "Aeio, mantis ISR");
>
>    /*    Events
>     *    (1) PCMCIA insert
>     *    (2) PCMCIA extract
>     *    (3) I2C complete
>     */
>
>    return IRQ_HANDLED;
>}

You must check here if this interrupt was really from your device. If not, you 
must return IRQ_NONE. You have requested your interrupt as a shared one so 
this may happen at any time.

>static int mantis_i2c_setup(struct mantis_pci *mantis)
>{
>    u32 config = 0;

You don't need to set this here, you will overwrite it anyway before looking 
at it.

>//    mmwrite(0x80, MANTIS_DMA_CTL); // MCU i2c read
>    config = mmread(MANTIS_DMA_CTL);
>    dprintk(verbose, MANTIS_DEBUG, 1, "Mantis Ctl reg=0x%04x", config);
>
>    return 0;
>}

>static int __devinit mantis_pci_probe(struct pci_dev *pdev,
>                const struct pci_device_id *mantis_pci_table)
>{
>    u8 revision, latency;
>//    u8 data[2];
>    struct mantis_pci *mantis;

Please insert a blank line here, this improves readability.

>    mantis = (struct mantis_pci *)
>                kmalloc(sizeof (struct mantis_pci), GFP_KERNEL);
>    if (mantis == NULL) {
>        dprintk(verbose, MANTIS_ERROR, 1, "Out of memory");
>        return -ENOMEM;
>    }
>    dprintk(verbose, MANTIS_ERROR, 1, "Got a device");
>    mantis->mantis_addr = pci_resource_start(pdev, 0);
>    if (!request_mem_region(pci_resource_start(pdev, 0),
>        pci_resource_len(pdev, 0), DRIVER_NAME)) {
>        dprintk(verbose, MANTIS_ERROR, 1, "Request mem region failed");
>        goto err0;

I would prefer to use more descriptive names for the labels, like err_memreg, 
err_iomem, err_irq or something like that.

Also you will get wrong assignements here. You must call pci_enable_device() 
_first_, it will set up the BARs of that device. Also I think 
pci_request_regions() might be a better way to get this assignements.

>    }
>    if ((mantis->mantis_mmio =
>                ioremap(mantis->mantis_addr, 0x1000)) == NULL) {
>        dprintk(verbose, MANTIS_ERROR, 1, "IO remap failed");
>        goto err1;
>    }
>    if (request_irq(pdev->irq, mantis_pci_irq, SA_SHIRQ |
>                SA_INTERRUPT, DRIVER_NAME, mantis) < 0) {
>        dprintk(verbose, MANTIS_ERROR, 1, "Mantis IRQ reg failed");
>        goto err2;
>    }
>    dprintk(verbose, MANTIS_DEBUG, 1, "We got an IRQ");
>    if (pci_enable_device(pdev)) {
>        dprintk(verbose, MANTIS_ERROR, 1, "Mantis PCI enable failed");
>        goto err3;
>    }
>    dprintk(verbose, MANTIS_DEBUG, 1, "We finally enabled the device");
>    pci_set_master(pdev);
>    pci_read_config_byte(pdev, PCI_LATENCY_TIMER, &latency);
>    pci_read_config_byte(pdev, PCI_CLASS_REVISION, &revision);
>    mantis->latency = latency;
>    mantis->revision = revision;
>    if (!latency) {
>        pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 32);
>    }
>    pci_set_drvdata(pdev, mantis);
>    dprintk(verbose, MANTIS_ERROR, 0, "Mantis Rev %d, ", mantis->revision);
>    dprintk(verbose, MANTIS_ERROR, 0, "irq: %d, latency: %d\n \
>        memory: 0x%04x, mmio: %p\n", pdev->irq, mantis->latency,    \
>        mantis->mantis_addr, mantis->mantis_mmio);
>err3:
>    free_irq(pdev->irq, pdev);
>err2:
>    if (mantis->mantis_mmio)
>        iounmap(mantis->mantis_mmio);
>err1:
>    release_mem_region(pci_resource_start(pdev, 0),
>                pci_resource_len(pdev, 0));
>err0:
>    kfree(mantis);
>
>    return 0;
>}
>
>static void __devexit mantis_pci_remove(struct pci_dev *pdev)
>{
>    struct mantis_pci *mantis = pci_get_drvdata(pdev);
>    if (mantis == NULL) {
>        dprintk(verbose, MANTIS_ERROR, 1, "Aeio, MAntis NULL ptr");
>        return;
>    }
>    dprintk(verbose, MANTIS_ERROR, 1, "Removing -->Mantis irq: %d, \
>        latency: %d\n memory: 0x%04x, mmio: 0x%p",
>        pdev->irq, mantis->latency, mantis->mantis_addr,
>        mantis->mantis_mmio);
>
>    free_irq(pdev->irq, pdev);
>
>    release_mem_region(pci_resource_start(pdev, 0),
>        pci_resource_len(pdev, 0));

pci_release_regions(pdev);

>    pci_set_drvdata(pdev, NULL);
>    pci_disable_device(pdev);
>    kfree(mantis);
>}

HTH

Eike

Attachment: pgphdAUXlTPJv.pgp
Description: PGP signature