Re: [KJ] question about pci_dev_put

[Mark Hollomon <markhollomon@comcast.net>]

Nishanth Aravamudan wrote:
> On 17.05.2006 [21:43:26 -0400], Mark Hollomon wrote:
>> Nishanth Aravamudan wrote:
>>> On 18.05.2006 [01:16:22 +0200], trem wrote:
>>>> Hi
>>>>
>>>> I'm looking for information about pci_get_device and pci_dev_put. If my
>>>> research are good,
>>>> I've understood that pci_get_device increment the counter of the device
>>>> returned, and it also
>>>> decremented the counter of the device given as "from".
>>>>
>>>> I've searched example in the source. I've found this one in
>>>> arch/ppc/platforms/85xx/mpc85xx_cds_common.c(git kernel) :
>>>>
>>>>    if ((dev = pci_get_device(PCI_VENDOR_ID_VIA,
>>>>                                        PCI_DEVICE_ID_VIA_82C586_2,
>>>> NULL))) {
>>>>                dev->irq = 10;
>>>>                pci_write_config_byte(dev, PCI_INTERRUPT_LINE, 10);
>>>>        pci_dev_put(dev);
>>>>        }
>>>>
>>>>    if ((dev = pci_get_device(PCI_VENDOR_ID_VIA,
>>>>                                        PCI_DEVICE_ID_VIA_82C586_2, 
>>>>                                        dev))) {
>>>>                dev->irq = 11;
>>>>                pci_write_config_byte(dev, PCI_INTERRUPT_LINE, 11);
>>>>        pci_dev_put(dev);
>>>>        }
>>>>
>>>> So, if I don't mistake, this code is buggy, the counter is decremented
>>>> twice in the second if.
>>>> One in the pci_get_device and one in pci_dev_put. I'm on the right way ?
>>> /me knows nothing about the PCI space, but looking at the comments for
>>> pci_get_device() and tracing what pci_dev_put() does, it seems like you
>>> are right.
>>>
>>> It seems like the correct method is to remove the pci_dev_put(dev); from
>>> the first if above and change the second one to
>>>
>>> 	if ((dev = pci_get_device(PCI_VENDOR_ID_VIA,
>>> 					PCI_DEVICE_ID_VIA_82C586_2, dev))) {
>>> 		dev->irq = 11;
>>> 		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, 11);
>>> 		pci_dev_put(dev);
>>> 	} else
>>> 		pci_dev_put(dev);
>> That can't be right. you will only reach the else leg if pci_get_device() 
>> returns
>> NULL. which means that you will be calling pci_dev_put(NULL).
> 
> Gah, true :) Thanks, Mark. I guess you'd need to save the a pointer to
> the old_dev.

Why? "from" always has its reference count decremented in pci_get_device.
Going back to the original code smippet, I think all that needs to happen
is that the pci_dev_put(dev) needs to be deleted from the first "if" block.

Of course, there seems to be another problem - well nonoptimal behavior. If
the first "if" fails, the code does the exact same search again. The second
"if" could be nested inside the first.

> 
> So much for quick pseudocode.
> 
> Thanks,
> Nish
> 


-- 
Mark Hollomon
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors