| Message ID | 20210722105945.2080428-2-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: pcihp: fix hotplug when bridge is wired to function > 0 | expand |
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. Or just squash this part into next patch. up to you. > @@ -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
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 >
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; @@ -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)); } }
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-build.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)