* 2.6 probe.c "pcibus_class" Device Class, release function
@ 2004-02-03 22:33 John Rose
2004-02-04 1:31 ` Matthew Dobson
0 siblings, 1 reply; 6+ messages in thread
From: John Rose @ 2004-02-03 22:33 UTC (permalink / raw)
To: colpatch, greg KH, lkml
The function release_pcibus_dev() in probe.c defines the release procedure for
device class pcibus_class. I want to suggest that this function be scrapped :)
This release function is called in the code path of class_device_unregister().
The pcibus_class devices aren't currently unregistered anywhere, from what I
can tell, so this release function is currently unused. The runtime removal of
PCI buses from logical partitions on PPC64 requires the unregistration of these
class devices. The natural place to do this IMHO is in pci_remove_bus_device()
in remove.c.
The problem is that this calls pci_destroy_dev(), which calls put() on the same
"bridge" device that the release function does. This should only be done once
in the course of removing a pci_bus, and I doubt that we want to change
pci_destroy_dev(). The kfree() of the pci_bus struct is also done in both
pci_remove_bus_device() and release_pcibus_dev().
So the only two operations in the release function are redundantly performed in
the place where it makes sense to unregister. For these reasons, I think we
should scrap the release function altogether and set that pointer in the struct
class to NULL.
Thoughts?
John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6 probe.c "pcibus_class" Device Class, release function
2004-02-03 22:33 2.6 probe.c "pcibus_class" Device Class, release function John Rose
@ 2004-02-04 1:31 ` Matthew Dobson
2004-02-04 16:00 ` John Rose
2004-02-04 18:01 ` Greg KH
0 siblings, 2 replies; 6+ messages in thread
From: Matthew Dobson @ 2004-02-04 1:31 UTC (permalink / raw)
To: John Rose; +Cc: greg KH, lkml
[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]
John Rose wrote:
> The function release_pcibus_dev() in probe.c defines the release procedure for
> device class pcibus_class. I want to suggest that this function be scrapped :)
>
> This release function is called in the code path of class_device_unregister().
> The pcibus_class devices aren't currently unregistered anywhere, from what I
> can tell, so this release function is currently unused. The runtime removal of
> PCI buses from logical partitions on PPC64 requires the unregistration of these
> class devices. The natural place to do this IMHO is in pci_remove_bus_device()
> in remove.c.
You're right that the class device isn't currently unregistered, and
that was an oversight in the patch I originally sent. Attatched is a
patch that remedies that situation. pci_remove_bus_device() *is* the
natural place to unregister the class_dev, and that's just what the
patch does.
> The problem is that this calls pci_destroy_dev(), which calls put() on the same
> "bridge" device that the release function does. This should only be done once
> in the course of removing a pci_bus, and I doubt that we want to change
> pci_destroy_dev(). The kfree() of the pci_bus struct is also done in both
> pci_remove_bus_device() and release_pcibus_dev().
Yep. The patch pulls the kfree() out of pci_remove_bus_device() and
calls class_device_unregister() in it's place. This will put() the
bridge device and free the pci_bus as needed. put() does need to be
called twice because the bridge device is get()'d twice: once when the
device is registered and once when it's bus device grabs a reference to it.
> So the only two operations in the release function are redundantly performed in
> the place where it makes sense to unregister. For these reasons, I think we
> should scrap the release function altogether and set that pointer in the struct
> class to NULL.
>
> Thoughts?
> John
Disagree, for the reasons above. ;) Patch attatched.
Cheers!
-Matt
[-- Attachment #2: pcibus_memleak-fix.patch --]
[-- Type: text/plain, Size: 495 bytes --]
diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.2-rc3/drivers/pci/remove.c linux-2.6.2-rc3+pcibus_memleak-fix/drivers/pci/remove.c
--- linux-2.6.2-rc3/drivers/pci/remove.c Thu Jan 8 22:59:10 2004
+++ linux-2.6.2-rc3+pcibus_memleak-fix/drivers/pci/remove.c Tue Feb 3 17:17:30 2004
@@ -83,7 +83,7 @@ void pci_remove_bus_device(struct pci_de
list_del(&b->node);
spin_unlock(&pci_bus_lock);
- kfree(b);
+ class_device_unregister(&b->class_dev);
dev->subordinate = NULL;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6 probe.c "pcibus_class" Device Class, release function
2004-02-04 1:31 ` Matthew Dobson
@ 2004-02-04 16:00 ` John Rose
2004-02-04 16:05 ` John Rose
2004-02-04 18:01 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: John Rose @ 2004-02-04 16:00 UTC (permalink / raw)
To: colpatch; +Cc: greg KH, lkml
> put() does need to be
> called twice because the bridge device is get()'d twice: once when the
> device is registered and once when it's bus device grabs a reference to it.
Looks great, thanks Matt.
John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6 probe.c "pcibus_class" Device Class, release function
2004-02-04 16:00 ` John Rose
@ 2004-02-04 16:05 ` John Rose
2004-02-04 17:22 ` John Rose
0 siblings, 1 reply; 6+ messages in thread
From: John Rose @ 2004-02-04 16:05 UTC (permalink / raw)
To: colpatch; +Cc: greg KH, lkml
Oops - quick question. Does unregistering blow away the cpuaffinity
attr and the bridge symlink?
Thanks-
John
On Wed, 2004-02-04 at 10:00, John Rose wrote:
> > put() does need to be
> > called twice because the bridge device is get()'d twice: once when the
> > device is registered and once when it's bus device grabs a reference to it.
>
> Looks great, thanks Matt.
>
> John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6 probe.c "pcibus_class" Device Class, release function
2004-02-04 16:05 ` John Rose
@ 2004-02-04 17:22 ` John Rose
0 siblings, 0 replies; 6+ messages in thread
From: John Rose @ 2004-02-04 17:22 UTC (permalink / raw)
To: colpatch; +Cc: greg KH, lkml
After testing, it would appear that it does. Perfect! Thanks.
John
On Wed, 2004-02-04 at 10:05, John Rose wrote:
> Oops - quick question. Does unregistering blow away the cpuaffinity
> attr and the bridge symlink?
>
> Thanks-
> John
>
> On Wed, 2004-02-04 at 10:00, John Rose wrote:
> > > put() does need to be
> > > called twice because the bridge device is get()'d twice: once when the
> > > device is registered and once when it's bus device grabs a reference to it.
> >
> > Looks great, thanks Matt.
> >
> > John
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.6 probe.c "pcibus_class" Device Class, release function
2004-02-04 1:31 ` Matthew Dobson
2004-02-04 16:00 ` John Rose
@ 2004-02-04 18:01 ` Greg KH
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2004-02-04 18:01 UTC (permalink / raw)
To: Matthew Dobson; +Cc: John Rose, greg KH, lkml
On Tue, Feb 03, 2004 at 05:31:42PM -0800, Matthew Dobson wrote:
> John Rose wrote:
> >The function release_pcibus_dev() in probe.c defines the release procedure
> >for
> >device class pcibus_class. I want to suggest that this function be
> >scrapped :)
> >
> >This release function is called in the code path of
> >class_device_unregister().
> >The pcibus_class devices aren't currently unregistered anywhere, from what
> >I
> >can tell, so this release function is currently unused. The runtime
> >removal of
> >PCI buses from logical partitions on PPC64 requires the unregistration of
> >these
> >class devices. The natural place to do this IMHO is in
> >pci_remove_bus_device()
> >in remove.c.
>
> You're right that the class device isn't currently unregistered, and
> that was an oversight in the patch I originally sent. Attatched is a
> patch that remedies that situation. pci_remove_bus_device() *is* the
> natural place to unregister the class_dev, and that's just what the
> patch does.
Thanks, I've applied this patch to my trees and will send off to Linus
in the next round of PCI patches.
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-02-04 18:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-02-03 22:33 2.6 probe.c "pcibus_class" Device Class, release function John Rose
2004-02-04 1:31 ` Matthew Dobson
2004-02-04 16:00 ` John Rose
2004-02-04 16:05 ` John Rose
2004-02-04 17:22 ` John Rose
2004-02-04 18:01 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox