Re: PCI driver

[Manu Abraham <manu@linuxtv.org>]

Rolf Eike Beer wrote:

> Manu Abraham wrote:
>  
>
> > Jiri Slaby wrote:
> >    
> >
> > > Manu Abraham napsal(a):
> > >      
> > >
> > > > Jiri Slaby wrote:
> > > >        
> > > >
> > > > > Manu Abraham napsal(a):
> > > > >          
> > > > >
> > > > > > Jiri Slaby wrote:
> > > > > >            
> > > > > >
> > > > > > > you do NOT do this at all, because you have pdev already (the param
> > > > > > > of the probe function)
> > > > > > >              
> > > > > > I rewrote the entire thing like this including the pci_remove
> > > > > > function too, but now it so seems that in the remove function,
> > > > > > pci_get_drvdata(pdev) returns NULL, and hence i get an Oops at
> > > > > > module removal.
> > > > > >            
> > > > I just found that, pci_enable_device() fails. So what's the way to go
> > > > ahead ?
> > > >        
> > > JESUS.
> > >      
> > Hmm.. i finally got it to work. It seems pci_get_device() is necessary,
> > i can't seem to enable the device or request for an IRQ the way you
> > suggested. It looks some quirks are there though ..
> >
> > If only i could explain why it works this way and not the other way ..
> >    
>
> Because pci_enable_device() works like most other kernel (and also libc) 
> functions: it returns 0 if everything went fine.
>
>
>  
>
> > [   81.269655] mantis_pci_probe: Got a device
> > [   81.269825] mantis_pci_probe: We got an IRQ
> > [   81.269987] mantis_pci_probe: We finally enabled the device
> > [   81.270191] Mantis Rev 1, irq: 23, latency: 32
> > [   81.270289] memory: 0xefeff000, mmio: f9218000
> > [   81.270519] Trying to free free IRQ23
> > [   90.485885] mantis_pci_remove: Removing -->Mantis irq: 23, latency: 32
> > [   90.485887] memory: 0xefeff000, mmio: 0xf9218000
> > [   90.486293] Trying to free free IRQ23
> > [   90.486429] Trying to free nonexistent resource <efeff000-efefffff>
> >    
>
> You should introduce a table of PCI devices here that your driver feels 
> responsible for. Then put this into a struct pci_driver which you pass to 
>  
I am in fact doing that ..

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 *struct* pci_driver mantis_pci_driver = {
	.name = "Mantis PCI combo driver",
	.id_table = mantis_pci_table,
	.probe = mantis_pci_probe,
	.remove = mantis_pci_remove,
};

static int __devinit mantis_pci_init(void)
{ 
	*return* pci_register_driver(&mantis_pci_driver);
}

static void __devexit mantis_pci_exit(void)
{
	pci_unregister_driver(&mantis_pci_driver);
}

module_init(mantis_pci_init);
module_exit(mantis_pci_exit);

MODULE_DESCRIPTION("Mantis PCI DTV bridge driver");
MODULE_AUTHOR("Manu Abraham");
MODULE_LICENSE("GPL");



> pci_module_init. Take a look on a random other PCI driver, 
> drivers/net/8139too.c, drivers/scsi/aic7xxx/aic7xxx_osm_pci.c, whatever.
>
>  
>
> > 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;
> > 	mantis = (struct mantis_pci *) kmalloc(sizeof (struct mantis_pci),
> > GFP_KERNEL);
> > 	if (mantis == NULL) {
> > 		dprintk(verbose, MANTIS_ERROR, 1, "Out of memory");
> > 		return -ENOMEM;
> > 	}
> >
> > 	pdev = pci_get_device(PCI_VENDOR_ID_MANTIS, PCI_DEVICE_ID_MANTIS_R11,
> > NULL);
> >    
>
> This is not needed anymore then. Your probe function will get called with for 
> any pci dev your driver can handle.
>
>  

I will just check it up again to see what went wrong ..

> > 	if (pdev) {
> > 		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 for memory region failed");
> >    
>
> Line length is maximum 80 characters. See Documentation/CodingStyle
>  

That was not meant to go into the kernel straight away, as it needs 
*lot* of work, the PCI part is only something extremely small.

>  
>
> > 			goto err0;
> > 		}
> > 		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, (void *) mantis_pci_irq, SA_SHIRQ |
> > 						SA_INTERRUPT, DRIVER_NAME, (void *) mantis) < 0) {
> >    
>
> You don't need to cast a pointer to void* or vice versa.
>
>  
Ah, thanks ...

> > 			dprintk(verbose, MANTIS_ERROR, 1, "Mantis IRQ registration failed");
> > 			goto err2;
> > 		}
> > 		dprintk(verbose, MANTIS_DEBUG, 1, "We got an IRQ");
> > 		if (pci_enable_device(pdev)) {
> > 			dprintk(verbose, MANTIS_ERROR, 1, "Mantis PCI device 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);
> > 		}
> >    
>
> The value in mantis->latency and the one in the card's address space now differ.
>  

Yes, i set it to the default latency as specified by vendor.. But 
temporarily to test the card ..

>  
>
> > 		pci_set_drvdata(pdev, mantis);
> > 		dprintk(verbose, MANTIS_ERROR, 0, "Mantis Rev %d, ", mantis->revision);
> > 		dprintk(verbose, MANTIS_ERROR, 0, "irq: %d, latency: %d\nmemory:
> > 0x%04x, mmio: %p\n", pdev->irq, mantis->latency,
> > 			mantis->mantis_addr, mantis->mantis_mmio);
> >
> > 		pci_dev_put(pdev);
> >    
>
> No, DON'T DO THAT! This will drop the a reference count from the struct 
> pci_dev, which means it can get freed while your driver still wants to work 
> with it.
>
>  

Hmm.. I thought after i make a call to pci_get_device(), i have to do a 
pci_dev_put() after the usage ..
I was a bit lost when to use pci_dev_put() in this case.

> > 	} else {
> > 		dprintk(verbose, MANTIS_ERROR, 1, "No device found");
> > 		return -ENODEV;
> > 	}
> >
> > 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");
> >    
>
> a) this should really never happen. If it happens, it's a kernel bug.
> b) if you catch this error while debugging, you should return here so you do
>   not dereference this NULL pointer.
>
>  
Ack.

> > 	}
> > 	dprintk(verbose, MANTIS_ERROR, 1, "Removing -->Mantis irq: %d, latency:
> > %d\nmemory: 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_disable_device(pdev);
> > 	pci_set_drvdata(pdev, NULL);
> > 	kfree(mantis);
> > }
> >    

Thanks ..

Regards,
Manu

-
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/