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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 26 Jul 2021 18:26:28 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> 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.

So the first thing is to confirm if NULL return value is valid or not.
When it clear we can decide whether copy existing check like you do or
replace all such checks with asserts.

> 
> 
> >  
> > > +
> > >      bus = PCI_HOST_BRIDGE(host)->bus;
> > >      if (bus) {
> > >          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */  
> >
> >  
>