question about pci_dev_put

KJ trem 5 participants 10 messages May 17, 2006 – May 18, 2006 Kernel janitors

Share & tools

trem

author

2006-05-17 23:16 UTC #0 lore raw

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 ?

thanks,
trem

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

2006-05-18 01:20 UTC #1 lore raw

Quoted 26 lines · trem

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);
        }

Quoted 3 lines

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);

In the if case, pci_get_device will call pci_dev_put() on the old dev before returning the next dev. In the else case, there are no more devices, so we drop the refcount with an explicit pci_dev_put(). In both cases, we drop the refcount on the old dev.

Does that seem right?

I, of course, defer to Greg for the "actual" answer :)

Thanks,
Nish

Signature · 2 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Mark Hollomon

2006-05-18 01:43 UTC #2 lore raw

In reply to trem

Quoted 38 lines · Nishanth Aravamudan

Quoted 26 lines · trem
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);
        }
Quoted 3 lines
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

Quoted 7 lines

	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).

Signature · 5 lines

Mark Hollomon
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

2006-05-18 02:00 UTC #3 lore raw

In reply to trem

Quoted 48 lines · Mark Hollomon

Quoted 39 lines · Nishanth Aravamudan
Quoted 27 lines · trem
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);
       }
Quoted 3 lines
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
Quoted 7 lines
	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);

Quoted 3 lines

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.

So much for quick pseudocode.

Thanks,
Nish

Signature · 2 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Matthew Wilcox

2006-05-18 02:47 UTC #4 lore raw

In reply to trem

Quoted 34 lines · Nishanth Aravamudan

Quoted 15 lines
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);
        }
Quoted 3 lines
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);

Quoted 6 lines

In the if case, pci_get_device will call pci_dev_put() on the old dev before returning the next dev. In the else case, there are no more devices, so we drop the refcount with an explicit pci_dev_put(). In both cases, we drop the refcount on the old dev.
Does that seem right?

No. pci_get_device unconditionally does pci_dev_put(). So if should just be:

	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);
	}

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

2006-05-18 03:39 UTC #5 lore raw

In reply to trem

Quoted 45 lines · Matthew Wilcox

Quoted 34 lines · Nishanth Aravamudan
Quoted 15 lines
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);
        }
Quoted 3 lines
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);
Quoted 6 lines
In the if case, pci_get_device will call pci_dev_put() on the old dev before returning the next dev. In the else case, there are no more devices, so we drop the refcount with an explicit pci_dev_put(). In both cases, we drop the refcount on the old dev.
Does that seem right?
No. pci_get_device unconditionally does pci_dev_put(). So if should just be:

Quoted 6 lines

	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);
	}

That's what the code has now, right? But we did a pci_dev_put() on the dev that we're passing to pci_get_device() -- and pci_get_device() will do an unconditionaly pci_dev_put() again, no?

... <light bulb> ...

Ah, I see what you're saying, sorry. So, yes, take the pci_dev_put() out of the previous if-clause (the IRQ 10 case) and let pci_get_device() unconditionally do it?

Thanks,
Nish

Signature · 2 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Matthew Wilcox

2006-05-18 11:59 UTC #6 lore raw

In reply to trem

Quoted 3 lines · Nishanth Aravamudan

Ah, I see what you're saying, sorry. So, yes, take the pci_dev_put() out of the previous if-clause (the IRQ 10 case) and let pci_get_device() unconditionally do it?

Yes.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Mark Hollomon

2006-05-18 12:57 UTC #7 lore raw

In reply to trem

Quoted 47 lines · Nishanth Aravamudan

Quoted 46 lines · Mark Hollomon
Quoted 45 lines · Nishanth Aravamudan
Quoted 27 lines · trem
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);
       }
Quoted 3 lines
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);

Quoted 6 lines

Quoted 3 lines
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.

Quoted 6 lines

So much for quick pseudocode.
Thanks,
Nish

Signature · 5 lines

Mark Hollomon
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Greg KH

2006-05-18 17:36 UTC #8 lore raw

In reply to trem

Quoted 26 lines · trem

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);
        }

Quoted 3 lines

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 ?

Yes, the code is incorrect. Here's a proper fix for it. Kumar, feel free to apply.

-------

From: Greg Kroah-Hartman <[email protected]>
Subject: Fix incorrect pci reference counting in 85xx code

As pointed out by trem <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/ppc/platforms/85xx/mpc85xx_cds_common.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

arch/ppc/platforms/85xx/mpc85xx_cds_common.c +5 -6

@@ -379,14 +379,13 @@ mpc85xx_cds_pcibios_fixup(void)
                                         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,
+		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);
+	                dev->irq = 11;
+	                pci_write_config_byte(dev, PCI_INTERRUPT_LINE, 11);
+			pci_dev_put(dev);
+		}
         }
 }
 #endif /* CONFIG_PCI */

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Greg KH

2006-05-18 17:37 UTC #9 lore raw

In reply to trem

Quoted 27 lines · Greg KH

Quoted 26 lines · trem
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);
        }

Quoted 6 lines

Quoted 3 lines
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 ?
Yes, the code is incorrect. Here's a proper fix for it. Kumar, feel free to apply.

Oops, that was wrong, here's the correct one, sorry about that:

-------

From: Greg Kroah-Hartman <[email protected]>
Subject: Fix incorrect pci reference counting in 85xx code

As pointed out by trem <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 arch/ppc/platforms/85xx/mpc85xx_cds_common.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

arch/ppc/platforms/85xx/mpc85xx_cds_common.c +4 -5

@@ -379,13 +379,12 @@ mpc85xx_cds_pcibios_fixup(void)
                                         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,
+		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);
+	                dev->irq = 11;
+	                pci_write_config_byte(dev, PCI_INTERRUPT_LINE, 11);
+		}
 		pci_dev_put(dev);
         }
 }

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors