[PULL,7/9] hw/i386/acpi-build: Fix maybe-uninitialized error when ACPI hotplug off

Commit Message

Alex Bennée Nov. 16, 2020, 12:24 p.m. UTC

From: Philippe Mathieu-Daudé <[e-mail]>

GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
is already in the "if (bsel || pcihp_bridge_en)" block statement,
but it isn't smart enough to figure it out.

Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
block statement to fix (on Ubuntu):

  ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
  ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]
    496 |         aml_append(parent_scope, method);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors

Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off globally")
Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
Signed-off-by: Alex Bennée <[e-mail]>
Reviewed-by: Igor Mammedov <[e-mail]>
Message-Id: <[e-mail]>
Message-Id: <[e-mail]>

Comments

Michael S. Tsirkin Nov. 16, 2020, 12:27 p.m. UTC | #1

On Mon, Nov 16, 2020 at 12:24:15PM +0000, Alex Bennée wrote:
> From: Philippe Mathieu-Daudé <[e-mail]>
> 
> GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
> is already in the "if (bsel || pcihp_bridge_en)" block statement,
> but it isn't smart enough to figure it out.
> 
> Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
> block statement to fix (on Ubuntu):
> 
>   ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
>   ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
>   in this function [-Werror=maybe-uninitialized]
>     496 |         aml_append(parent_scope, method);
>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
> 
> Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off globally")
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> Signed-off-by: Alex Bennée <[e-mail]>
> Reviewed-by: Igor Mammedov <[e-mail]>
> Message-Id: <[e-mail]>
> Message-Id: <[e-mail]>

BTW it's in my pull request alredy.
Not sure why you are merging it too ...

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4f66642d88..1f5c211245 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -465,34 +465,31 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>       */
>      if (bsel || pcihp_bridge_en) {
>          method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
> -    }
> -    /* If bus supports hotplug select it and notify about local events */
> -    if (bsel) {
> -        uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
>  
> -        aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
> -        aml_append(method,
> -            aml_call2("DVNT", aml_name("PCIU"), aml_int(1) /* Device Check */)
> -        );
> -        aml_append(method,
> -            aml_call2("DVNT", aml_name("PCID"), aml_int(3)/* Eject Request */)
> -        );
> -    }
> +        /* If bus supports hotplug select it and notify about local events */
> +        if (bsel) {
> +            uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
>  
> -    /* 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;
> +            aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
> +            aml_append(method, aml_call2("DVNT", aml_name("PCIU"),
> +                                         aml_int(1))); /* Device Check */
> +            aml_append(method, aml_call2("DVNT", aml_name("PCID"),
> +                                         aml_int(3))); /* Eject Request */
> +        }
>  
> -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> -                continue;
> -            }
> +        /* 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) || pci_bus_is_express(sec)) {
> +                    continue;
> +                }
>  
> -            aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> +                aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> +            }
>          }
> -    }
>  
> -    if (bsel || pcihp_bridge_en) {
>          aml_append(parent_scope, method);
>      }
>      qobject_unref(bsel);
> -- 
> 2.20.1

Philippe Mathieu-Daudé Nov. 16, 2020, 1:11 p.m. UTC | #2

On 11/16/20 1:27 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2020 at 12:24:15PM +0000, Alex Bennée wrote:
>> From: Philippe Mathieu-Daudé <[e-mail]>
>>
>> GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
>> is already in the "if (bsel || pcihp_bridge_en)" block statement,
>> but it isn't smart enough to figure it out.
>>
>> Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
>> block statement to fix (on Ubuntu):
>>
>>   ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
>>   ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
>>   in this function [-Werror=maybe-uninitialized]
>>     496 |         aml_append(parent_scope, method);
>>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   cc1: all warnings being treated as errors
>>
>> Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off globally")
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> Signed-off-by: Alex Bennée <[e-mail]>
>> Reviewed-by: Igor Mammedov <[e-mail]>
>> Message-Id: <[e-mail]>
>> Message-Id: <[e-mail]>
> 
> BTW it's in my pull request alredy.
> Not sure why you are merging it too ...

I suppose to unbreak Gitlab-CI...

There is no policy w.r.t. CI so maintainer don't have to use it,
but this breaking it delay the workflow of others subsystems.

I'm not asking you to use it, just explaining why this patch is
in Alex's queue.

Regards,

Phil.

Michael S. Tsirkin Nov. 16, 2020, 1:27 p.m. UTC | #3

On Mon, Nov 16, 2020 at 02:11:35PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/16/20 1:27 PM, Michael S. Tsirkin wrote:
> > On Mon, Nov 16, 2020 at 12:24:15PM +0000, Alex Bennée wrote:
> >> From: Philippe Mathieu-Daudé <[e-mail]>
> >>
> >> GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
> >> is already in the "if (bsel || pcihp_bridge_en)" block statement,
> >> but it isn't smart enough to figure it out.
> >>
> >> Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
> >> block statement to fix (on Ubuntu):
> >>
> >>   ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
> >>   ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
> >>   in this function [-Werror=maybe-uninitialized]
> >>     496 |         aml_append(parent_scope, method);
> >>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>   cc1: all warnings being treated as errors
> >>
> >> Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off globally")
> >> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> >> Signed-off-by: Alex Bennée <[e-mail]>
> >> Reviewed-by: Igor Mammedov <[e-mail]>
> >> Message-Id: <[e-mail]>
> >> Message-Id: <[e-mail]>
> > 
> > BTW it's in my pull request alredy.
> > Not sure why you are merging it too ...
> 
> I suppose to unbreak Gitlab-CI...
> 
> There is no policy w.r.t. CI so maintainer don't have to use it,
> but this breaking it delay the workflow of others subsystems.
> 
> I'm not asking you to use it, just explaining why this patch is
> in Alex's queue.
> 
> Regards,
> 
> Phil.

Not sure I understand.
It's in my pull request from Nov 15. I'm not sure how does it
help anyone to also have it in another request from Nov 16...

Alex Bennée Nov. 16, 2020, 4:01 p.m. UTC | #4

Michael S. Tsirkin <[e-mail]> writes:

> On Mon, Nov 16, 2020 at 02:11:35PM +0100, Philippe Mathieu-Daudé wrote:
>> On 11/16/20 1:27 PM, Michael S. Tsirkin wrote:
>> > On Mon, Nov 16, 2020 at 12:24:15PM +0000, Alex Bennée wrote:
>> >> From: Philippe Mathieu-Daudé <[e-mail]>
>> >>
>> >> GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
>> >> is already in the "if (bsel || pcihp_bridge_en)" block statement,
>> >> but it isn't smart enough to figure it out.
>> >>
>> >> Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
>> >> block statement to fix (on Ubuntu):
>> >>
>> >>   ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
>> >>   ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
>> >>   in this function [-Werror=maybe-uninitialized]
>> >>     496 |         aml_append(parent_scope, method);
>> >>         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >>   cc1: all warnings being treated as errors
>> >>
>> >> Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off globally")
>> >> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> >> Signed-off-by: Alex Bennée <[e-mail]>
>> >> Reviewed-by: Igor Mammedov <[e-mail]>
>> >> Message-Id: <[e-mail]>
>> >> Message-Id: <[e-mail]>
>> > 
>> > BTW it's in my pull request alredy.
>> > Not sure why you are merging it too ...
>> 
>> I suppose to unbreak Gitlab-CI...
>> 
>> There is no policy w.r.t. CI so maintainer don't have to use it,
>> but this breaking it delay the workflow of others subsystems.
>> 
>> I'm not asking you to use it, just explaining why this patch is
>> in Alex's queue.
>> 
>> Regards,
>> 
>> Phil.
>
> Not sure I understand.
> It's in my pull request from Nov 15. I'm not sure how does it
> help anyone to also have it in another request from Nov 16...

I didn't see your PR had gone in, it shouldn't matter though because git
should be smart enough to deal with it being in two places at once.

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4f66642d88..1f5c211245 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -465,34 +465,31 @@  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
      */
     if (bsel || pcihp_bridge_en) {
         method = aml_method("PCNT", 0, AML_NOTSERIALIZED);
-    }
-    /* If bus supports hotplug select it and notify about local events */
-    if (bsel) {
-        uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
 
-        aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
-        aml_append(method,
-            aml_call2("DVNT", aml_name("PCIU"), aml_int(1) /* Device Check */)
-        );
-        aml_append(method,
-            aml_call2("DVNT", aml_name("PCID"), aml_int(3)/* Eject Request */)
-        );
-    }
+        /* If bus supports hotplug select it and notify about local events */
+        if (bsel) {
+            uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
 
-    /* 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;
+            aml_append(method, aml_store(aml_int(bsel_val), aml_name("BNUM")));
+            aml_append(method, aml_call2("DVNT", aml_name("PCIU"),
+                                         aml_int(1))); /* Device Check */
+            aml_append(method, aml_call2("DVNT", aml_name("PCID"),
+                                         aml_int(3))); /* Eject Request */
+        }
 
-            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
-                continue;
-            }
+        /* 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) || pci_bus_is_express(sec)) {
+                    continue;
+                }
 
-            aml_append(method, aml_name("^S%.02X.PCNT", devfn));
+                aml_append(method, aml_name("^S%.02X.PCNT", devfn));
+            }
         }
-    }
 
-    if (bsel || pcihp_bridge_en) {
         aml_append(parent_scope, method);
     }
     qobject_unref(bsel);