Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()

[Igor Mammedov]

On Thu, 22 Jul 2021 13:56:18 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> I would add a description: we want to scan all functions
> not just function 0 to describe hotplug into bridges
> at function != 0. in preparation for this, refactor code to not
> skip functions != 0.
> 
> On Thu, Jul 22, 2021 at 06:59:44AM -0400, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 17836149fe..b40e284b72 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >      Aml *dev, *notify_method = NULL, *method;
> >      QObject *bsel;
> >      PCIBus *sec;
> > -    int i;
> > +    int devfn;
> >  
> >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, 
> > NULL);
> >      if (bsel) {
> > @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >          notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
> >      }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> > +    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> >          DeviceClass *dc;
> >          PCIDeviceClass *pc;
> > -        PCIDevice *pdev = bus->devices[i];
> > -        int slot = PCI_SLOT(i);
> > +        PCIDevice *pdev = bus->devices[devfn];
> > +        int slot = PCI_SLOT(devfn);
> >          bool hotplug_enabled_dev;
> >          bool bridge_in_acpi;
> >          bool cold_plugged_bridge;  
> 
> I am a bit puzzled about why this is equivalent. so we used to scan just
> function 0 on each slot. now we are scanning them all.
> won't this generate a different AML code? in fact duplicate
> descriptions?
> I suspect you need to move the check for slot == 0 from the next patch
> to this one otherwise bisect will be broken.

this was my mistake when splitting it into separate patch,
I shouldn't have touched += PCI_FUNC_MAX here, point of the patch
was to s/i/devfn/m so it won't distract from what the next patch
is doing.

> Or just squash this part into next patch. up to you.
ok I'll squash it into next.
 
> 
> > @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml 
> > *parent_scope, PCIBus *bus,
> >          /* Notify about child bus events in any case */
> >          if (pcihp_bridge_en) {
> >              QLIST_FOREACH(sec, &bus->child, sibling) {
> > -                int32_t devfn = sec->parent_dev->devfn;
> > -
> >                  if (pci_bus_is_root(sec)) {
> >                      continue;
> >                  }
> >  
> > -                aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> > +                aml_append(method, aml_name("^S%.02X.PCNT",
> > +                                            sec->parent_dev->devfn));
> >              }
> >          }
> >    
> 
> 
> this is a refactor, sure.
> 
> > -- 
> > 2.27.0  
>