[BUG/MEMLEAK?] struct pci_bus, child busses & bridges

[BUG/MEMLEAK?] struct pci_bus, child busses & bridges

[Matthew Dobson]

Whilst working on what was supposed to be a fairly trivial patch, I 
stumbled across what looks to be a relatively significant bug in the way 
pci bridges are handled.  I could easily be wrong, as the pci code is 
far from anything I'd be willing to call an area of expertise.  That 
said, my description of the problem follows:

I am trying to add a file to the pci bus (ie: /sysfs/devices/pciXXXX:XX) 
directories in sysfs.  This led to the discovery that struct pci_bus 
(inlude/linux/pci.h) does not have an *actual* struct dev inside it, 
rather a pointer to a struct dev.  I found this interesting, as most 
device-type-thingies have an honest to goodness struct dev, allowing the 
use of container_of(), etc.  A quick perusal of the code offered no 
reason why struct pci_bus couldn't be changed to have the actual struct 
dev inside it, saving us kmalloc'ing and kfree'ing these, and generally 
keeping track of them.  I was wrong.

In pci_alloc_child_bus (drivers/pci/probe.c), the child bus is allocated 
and it's struct dev * is set to point to the struct dev belonging to the 
bridge that this bus is 'on', or 'behind'.  pci_alloc_child_bus is 
called in 3 places: pci_add_new_bus and twice in pci_scan_bridge.  The 
calls in pci_scan_bridge allocate a new struct pci_bus, but then seem to 
throw the references away, *without* freeing them.

It appears that these pseudo-busses are allocated and used as a kind of 
hack, to allow devices to be parented to pci bridge devices.  The 
pci_dev for the bridge is allocated and initialized, then a child bus is 
created, and it's *dev is pointed to the struct device belonging to the 
pci bridge.  There are no refcounts used for this, and it doesn't appear 
to be cleaned up in any way.  If anyone can offer any insight into this 
problem, or show me why I'm wrong, it would be greatly appreciated.

Thanks!

-Matt

Re: [BUG/MEMLEAK?] struct pci_bus, child busses & bridges

[Russell King]

On Wed, Sep 24, 2003 at 05:34:03PM -0700, Matthew Dobson wrote:
> In pci_alloc_child_bus (drivers/pci/probe.c), the child bus is allocated 
> and it's struct dev * is set to point to the struct dev belonging to the 
> bridge that this bus is 'on', or 'behind'.  pci_alloc_child_bus is 
> called in 3 places: pci_add_new_bus and twice in pci_scan_bridge.  The 
> calls in pci_scan_bridge allocate a new struct pci_bus, but then seem to 
> throw the references away, *without* freeing them.

That is correct - they persist after they have been allocated until the
bridge device is destroyed (if ever) - it's lifetime is directly equivalent
to the lifetime of the bridge.

If you look carefully at pci_alloc_child_bus(), you will notice that
bridge->subordinate is setup to point at the pci_bus, which provides
a method to access the data held in the pci_bus later (eg, while we're
freeing the structures.)

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: [BUG/MEMLEAK?] struct pci_bus, child busses & bridges

[Matthew Dobson]

Russell King wrote:
> On Wed, Sep 24, 2003 at 05:34:03PM -0700, Matthew Dobson wrote:
> 
>>In pci_alloc_child_bus (drivers/pci/probe.c), the child bus is allocated 
>>and it's struct dev * is set to point to the struct dev belonging to the 
>>bridge that this bus is 'on', or 'behind'.  pci_alloc_child_bus is 
>>called in 3 places: pci_add_new_bus and twice in pci_scan_bridge.  The 
>>calls in pci_scan_bridge allocate a new struct pci_bus, but then seem to 
>>throw the references away, *without* freeing them.
> 
> 
> That is correct - they persist after they have been allocated until the
> bridge device is destroyed (if ever) - it's lifetime is directly equivalent
> to the lifetime of the bridge.
> 
> If you look carefully at pci_alloc_child_bus(), you will notice that
> bridge->subordinate is setup to point at the pci_bus, which provides
> a method to access the data held in the pci_bus later (eg, while we're
> freeing the structures.)

Ok, I see that now.  I guess my only remaining question is why do child 
busses not get their own struct device, but rather only a pointer to the 
bridge's struct device?  There's no refcounting done on this, ie: no 
pci_dev_get/put calls, but I guess that's kinda ok, since we're pretty 
sure that the child bus won't exist for longer than the bridge that owns 
it, right?  So using the bridge's struct dev allows the pci topology to 
look cleaner?  As in, there's no actual bus exposed in sysfs/procfs/etc, 
just devices that seem to be hanging off the bridge?

Cheers!

-Matt

Re: [BUG/MEMLEAK?] struct pci_bus, child busses & bridges

[Patrick Mochel]

> Ok, I see that now.  I guess my only remaining question is why do child 
> busses not get their own struct device, but rather only a pointer to the 
> bridge's struct device?  There's no refcounting done on this, ie: no 
> pci_dev_get/put calls, but I guess that's kinda ok, since we're pretty 
> sure that the child bus won't exist for longer than the bridge that owns 
> it, right?  So using the bridge's struct dev allows the pci topology to 
> look cleaner?  As in, there's no actual bus exposed in sysfs/procfs/etc, 
> just devices that seem to be hanging off the bridge?

Buses are not devices. Bridges are devices and get a struct device. Buses 
are physical (or logical) collections of devices at the same topological 
level which reside on one side of a bridge. I.e. they are objects of some 
sort, but not devices, and hence are not represented in /sys/devices/. 

It would be nice to export them in /sys/bus/pci/ somehow, but it's one of 
those things that I haven't gotten around to in the last 6 months or so. 
:) 


	Pat