Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

On Mon, 26 Jul 2021, Igor Mammedov wrote:

> On Wed, 21 Jul 2021 19:46:10 +0530
> Ani Sinha <ani@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 <ani@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 */
>
>