Re: [Qemu-devel] [PATCH 6/9] pci.c: factor out while(bus) bus->next loop logic into pci_find_bus_from().

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:45:06PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2009 at 08:15:06PM +0900, Isaku Yamahata wrote:
> > factor out while(bus) bus->next loop logic into pci_find_bus_from()
> > which will be used later.
> > 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/pci.c |   26 ++++++++++++++------------
> >  1 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 9639a32..f06e1da 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -574,6 +574,16 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
> > addr, uint32_t val, int l)
> >          pci_update_mappings(d);
> >  }
> >  
> > +static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> 
> Why what is "from"? pci_find_parent_bus a better name?

Its intention is to go down from a given parent bus
to find child bus of a given bus number, bus_num.

The current implementation arranges PCIBus in a single linked list,
and a single linked list doesn't represent pci bus topology well.
So it may find a pci bus which isn't child of a given bus.
So far it hasn't been a issue because only a single bus PCI.0 is supported.

Given that several people including me want multiple PCI bus,
your question arises that the linked list should be changed/enhanced to
some kind of tree structure to represent PCI bus topology accurately.
Does it sound a good idea?


> > +{
> > +    PCIBus *s = from;
> > +
> > +    while (s && s->bus_num != bus_num)
> 
> No space after while.
> 
> > +        s = s->next;
> > +
> > +    return s;
> > +}
> > +
> >  void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> >  {
> >      PCIBus *s = opaque;
> > @@ -585,8 +595,7 @@ void pci_data_write(void *opaque, uint32_t addr, 
> > uint32_t val, int len)
> >                  addr, val, len);
> >  #endif
> >      bus_num = (addr >> 16) & 0xff;
> > -    while (s && s->bus_num != bus_num)
> > -        s = s->next;
> > +    s = pci_find_bus_from(s, bus_num);
> >      if (!s)
> >          return;
> >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > @@ -607,8 +616,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int 
> > len)
> >      uint32_t val;
> >  
> >      bus_num = (addr >> 16) & 0xff;
> > -    while (s && s->bus_num != bus_num)
> > -        s= s->next;
> > +    s = pci_find_bus_from(s, bus_num);
> >      if (!s)
> >          goto fail;
> >      pci_dev = s->devices[(addr >> 8) & 0xff];
> > @@ -792,8 +800,7 @@ void pci_for_each_device(int bus_num, void 
> > (*fn)(PCIDevice *d))
> >      PCIDevice *d;
> >      int devfn;
> >  
> > -    while (bus && bus->bus_num != bus_num)
> > -        bus = bus->next;
> > +    bus = pci_find_bus_from(bus, bus_num);
> >      if (bus) {
> >          for(devfn = 0; devfn < 256; devfn++) {
> >              d = bus->devices[devfn];
> > @@ -891,12 +898,7 @@ static void pci_bridge_write_config(PCIDevice *d,
> >  
> >  PCIBus *pci_find_bus(int bus_num)
> >  {
> > -    PCIBus *bus = first_bus;
> > -
> > -    while (bus && bus->bus_num != bus_num)
> > -        bus = bus->next;
> > -
> > -    return bus;
> > +    return pci_find_bus_from(first_bus, bus_num);
> >  }
> >  
> >  PCIDevice *pci_find_device(int bus_num, int slot, int function)
> 

-- 
yamahata