On Mon, 26 Jul 2021, Igor Mammedov wrote:
> On Wed, 21 Jul 2021 19:46:10 +0530
> Ani Sinha <a...@anisinha.ca> wrote:
>
> > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > return from this function call. This change brings the same check to
> > acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> > why we unconditionally pass a truth value to the last argument when calling
> > acpi_pcihp_reset() from ich9 platform.
> >
> > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >
> > Signed-off-by: Ani Sinha <a...@anisinha.ca>
> > ---
> > hw/acpi/ich9.c | 1 +
> > hw/acpi/pcihp.c | 5 +++++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 778e27b659..58d8430eb9 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
> > pm->smi_en_wmask = ~0;
> >
> > if (pm->use_acpi_hotplug_bridge) {
> > + /* on root PCIE bus, we always use native or SHPC based hotplug */
> I had an impression that root bus doesn't support hotplug at all,
> and to have hotplug there one should attach a root-port to root bus at
> start up time.
I think you are right. However, I will let others confirm this and update
the comment accordingly in a separate patch.
>
> > acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
> > }
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f4d706e47d..856c6e1b47 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
> > return;
> > }
> >
> > + if (!host) {
> > + root_hp_disabled = true;
> > + return;
> > + }
> It should be a separate patch,
> when this could return NULL?
> If it should never be null then assert here would be better.
I have sent a v2 without the comment addition. I left the code this way
because everywhere else, the code checking host for NULL value is similar.
I wanted to keep the symmetry. However, if you strongly feel about the
assertion, I will send a v3.
>
> > +
> > bus = PCI_HOST_BRIDGE(host)->bus;
> > if (bus) {
> > /* setting the hotplug handler to NULL makes the bus
> > non-hotpluggable */
>
>
