On Fri, Jul 09, 2010 at 12:43:18AM +0900, Isaku Yamahata wrote:
> On Thu, Jul 08, 2010 at 05:04:32PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 07, 2010 at 11:38:58AM +0900, Isaku Yamahata wrote:
> > > But you claim it's only for root bus, not for secondary bus.
> >
> > It is currently, isn't it?
> >
> > > Now I realized why you've rejected such patches so far.
> > > Then, you also mean the current pci_register_secondary_bus() is broken.
> >
> > Sorry about being dense, what is broken?
>
> I've regarded pci_bus_new() (or _inplace) as new qdev style API.
The names are pretty bad btw, aren't they? Would
pci_bus_init/pci_bus_cleanup be better? And whoever wants to allocate
memory, can do it with malloc, right?
> And pci_register_bus() (or pci_register_secondary_bus()) as old
> (so deprecated) API.
> So pci_reguster_bus() would be replaced with pci_bus_new() gradually
> like the changeset of 7cd9eee0f6fd6953114068dd98d91fca1237880b
> I've thought that pci_bus_new() is for both root and secondary bus.
> However, according to your comment, the situation seems different.
Forget my comment, it's different according to the code, isn't it?
> > > I also think it's broken. So how do we want to fix it?
> > > My idea is as follows.
> > >
> > > - introduce something like pci_secondary_bus_new()
> > > (pci_sec_bus_new() for short?) for secondary bus.
> > > fix pci_register_secondary_bus() with it.
> > >
> > > - introduce something like pci_host_bus_new() (or pci_root_bus_new()?)
> > > for pci host bus which is more generic than pci_bus_new().
> > > It's for
> > > - to avoid confusion.
> >
> > IMHO the confusion comes from the fact we have too
> > many functions that do almost, but not quite, the same
> > thing, and the function names do not say anything.
> >
> > We have a ton of 5 line functions with names like
> > _allocate_inplace, _new, _register, _simple
>
> Fully Agreed. Some clean up is necessary.
>
>
> > > - to eliminate assumption of pci_bus_new().
> > > pci_bus_new() assumes that its pci segment is 0.
> > > keep pci_bus_new() as a convenience wrapper of
> > > pci_host_bus_new(segment = 0). Thus we can avoid fixing up
> > > all the caller.
> >
> > We have a single caller, right? I think you mean pci_register_bus?
> > So IIUC, you propose that we add pci_register_host_bus,
> > and make pci_register_bus a compatibility wrapper?
> > Sure, let's just add a comment this is deprecated.
> >
> > I am not sure why do we need an API to deal with secondary bus:
> > it is always a part of the bridge, so all users can and should call
> > pci_bridge_init?
>
> Okay, then how about the following?
>
> For root bus:
> - pci_host_bus_new()/pci_host_bus_new_inplace()
> qbus style api. pci segment must be specified.
> New code should use this.
I'd prefer a simple _init which works like _inplace.
Allocating memory is simple enough for users.
> - pci_bus_new()
> qbus style API.
> convenience wrapper for compatibility of
> pci_host_bus_new(pci segment = 0)
> In fact, the only current user piix_pci.c. It's easy to remove it.
> - pci_register_bus()
> old style API. deprecated.
> It has been kept for compatibility so far.
> This will be gradually replaced with pci_host_bus_new()
Also, let's make these helpers inline: will make it possible
to check code by comparing binary after changes.
> For secondary bus:
> - pci_bridge_init()
> qdev style API.
> New code should use this.
Well - isn't the way we do this a bit backwards?
I thought the idea was that each device has its own
PCIDeviceInfo qdev structure, instead of the common pci-bridge.
And then pci_bridge_init (or _setup to avoid reusing existing names)
would be a common function that devices can reuse in their init
functions..
> - pci_{register, unregister}_secondary_bus():
> old stype API. deprecated.
> Keep them only for internal use in pci.c
> or they can be easily removed or renamed for qdev style.
>
> For pci device:
> - pci_create()
> qdev style API.
> The transitional function until completion of qdev conversion.
> If the creation of a device tree from config file is implemented,
> this function will be unnecessary.
>
> - pci_create_simple()
> qdev style API.
> convenience function = pci_create() + qdev_init_nofail()
>
> --
> yamahata
