* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
@ 2006-05-18 1:20 ` Nishanth Aravamudan
2006-05-18 1:43 ` Mark Hollomon
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Nishanth Aravamudan @ 2006-05-18 1:20 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]
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);
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
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
2006-05-18 1:20 ` Nishanth Aravamudan
@ 2006-05-18 1:43 ` Mark Hollomon
2006-05-18 2:00 ` Nishanth Aravamudan
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Mark Hollomon @ 2006-05-18 1:43 UTC (permalink / raw)
To: kernel-janitors
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).
--
Mark Hollomon
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
2006-05-18 1:20 ` Nishanth Aravamudan
2006-05-18 1:43 ` Mark Hollomon
@ 2006-05-18 2:00 ` Nishanth Aravamudan
2006-05-18 2:47 ` Matthew Wilcox
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Nishanth Aravamudan @ 2006-05-18 2:00 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]
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.
So much for quick pseudocode.
Thanks,
Nish
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (2 preceding siblings ...)
2006-05-18 2:00 ` Nishanth Aravamudan
@ 2006-05-18 2:47 ` Matthew Wilcox
2006-05-18 3:39 ` Nishanth Aravamudan
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Matthew Wilcox @ 2006-05-18 2:47 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]
On Wed, May 17, 2006 at 06:20:16PM -0700, Nishanth Aravamudan wrote:
> > 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);
>
> 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);
}
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (3 preceding siblings ...)
2006-05-18 2:47 ` Matthew Wilcox
@ 2006-05-18 3:39 ` Nishanth Aravamudan
2006-05-18 11:59 ` Matthew Wilcox
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Nishanth Aravamudan @ 2006-05-18 3:39 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]
On 17.05.2006 [20:47:53 -0600], Matthew Wilcox wrote:
> On Wed, May 17, 2006 at 06:20:16PM -0700, Nishanth Aravamudan wrote:
> > > 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);
> >
> > 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);
> }
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
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (4 preceding siblings ...)
2006-05-18 3:39 ` Nishanth Aravamudan
@ 2006-05-18 11:59 ` Matthew Wilcox
2006-05-18 12:57 ` Mark Hollomon
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Matthew Wilcox @ 2006-05-18 11:59 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 246 bytes --]
On Wed, May 17, 2006 at 08:39:32PM -0700, Nishanth Aravamudan wrote:
> 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.
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (5 preceding siblings ...)
2006-05-18 11:59 ` Matthew Wilcox
@ 2006-05-18 12:57 ` Mark Hollomon
2006-05-18 17:36 ` Greg KH
2006-05-18 17:37 ` Greg KH
8 siblings, 0 replies; 10+ messages in thread
From: Mark Hollomon @ 2006-05-18 12:57 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (6 preceding siblings ...)
2006-05-18 12:57 ` Mark Hollomon
@ 2006-05-18 17:36 ` Greg KH
2006-05-18 17:37 ` Greg KH
8 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2006-05-18 17:36 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]
On Thu, May 18, 2006 at 01:16:22AM +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 ?
Yes, the code is incorrect. Here's a proper fix for it. Kumar, feel
free to apply.
-------
From: Greg Kroah-Hartman <gregkh@suse.de>
Subject: Fix incorrect pci reference counting in 85xx code
As pointed out by trem <tremyfr@yahoo.fr>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
arch/ppc/platforms/85xx/mpc85xx_cds_common.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
--- gregkh-2.6.orig/arch/ppc/platforms/85xx/mpc85xx_cds_common.c
+++ gregkh-2.6/arch/ppc/platforms/85xx/mpc85xx_cds_common.c
@@ -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 */
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [KJ] question about pci_dev_put
2006-05-17 23:16 [KJ] question about pci_dev_put trem
` (7 preceding siblings ...)
2006-05-18 17:36 ` Greg KH
@ 2006-05-18 17:37 ` Greg KH
8 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2006-05-18 17:37 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]
On Thu, May 18, 2006 at 10:36:12AM -0700, Greg KH wrote:
> On Thu, May 18, 2006 at 01:16:22AM +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 ?
>
> 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 <gregkh@suse.de>
Subject: Fix incorrect pci reference counting in 85xx code
As pointed out by trem <tremyfr@yahoo.fr>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
arch/ppc/platforms/85xx/mpc85xx_cds_common.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
--- gregkh-2.6.orig/arch/ppc/platforms/85xx/mpc85xx_cds_common.c
+++ gregkh-2.6/arch/ppc/platforms/85xx/mpc85xx_cds_common.c
@@ -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);
}
}
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 10+ messages in thread