linux-kernel - Re: [PATCH 1/6] arm/virt: Wire up GPIO error source for ACPI / GHES

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFEAcA-PYnZ-32MRX+PgvzhnoAV80zBKMYg61j2f=oHaGfwSsg@mail.gmail.com>
Date: Thu, 11 Jul 2024 12:59:20 +0100
From: Peter Maydell <peter.maydell@...aro.org>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, 
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, Philippe Mathieu-Daudé <philmd@...aro.org>, 
	Ani Sinha <anisinha@...hat.com>, Eduardo Habkost <eduardo@...kost.net>, 
	Igor Mammedov <imammedo@...hat.com>, Marcel Apfelbaum <marcel.apfelbaum@...il.com>, 
	Shannon Zhao <shannon.zhaosl@...il.com>, Yanan Wang <wangyanan55@...wei.com>, linux-edac@...nel.org, 
	linux-kernel@...r.kernel.org, qemu-arm@...gnu.org, qemu-devel@...gnu.org
Subject: Re: [PATCH 1/6] arm/virt: Wire up GPIO error source for ACPI / GHES

On Thu, 11 Jul 2024 at 12:54, Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Thu, Jul 11, 2024 at 11:52:03AM +0200, Mauro Carvalho Chehab wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> >
> > Creates a GED - Generic Event Device and set a GPIO to
> > be used or error injection.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > ---
> >  hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> >  hw/arm/virt.c            | 12 +++++++++++-
> >  include/hw/boards.h      |  1 +
> >  3 files changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index e10cad86dd73..b6f2e55014a2 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -63,6 +63,7 @@
> >
> >  #define ARM_SPI_BASE 32
> >
> > +#define ACPI_GENERIC_EVENT_DEVICE "GEDD"
> >  #define ACPI_BUILD_TABLE_SIZE             0x20000
> >
> >  static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
> > @@ -155,9 +156,14 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
> >
> >      Aml *aei = aml_resource_template();
> >      /* Pin 3 for power button */
> > -    const uint32_t pin_list[1] = {3};
> > +    uint32_t pin = 3;
> >      aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> > -                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
> > +                                 AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1,
> > +                                 "GPO0", NULL, 0));
> > +    pin = 6;
> > +    /* Pin 8 for generic error */
>
> For real? Code says 6, comment says 8.
>
> Comments must come before the code they comment, not after it,
> then this kind of thing won't happen.

It might also be nice to have a symbolic constant for the
pin number somewhere, so we don't rely on the magic number here and...

> > @@ -1014,6 +1021,8 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
> >  {
> >      gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> >                                          qdev_get_gpio_in(pl061_dev, 3));
> > +    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
> > +                                          qdev_get_gpio_in(pl061_dev, 6));

...here being the same.

Then if the code says "pin = VIRT_GPIO_ERROR_PIN" or something
similar the comment isn't required because the code is clear
without it.

(This is already a problem for the power-button pin 3, so we could
do an initial cleanup patch to sort out pin 3 first).

thanks
-- PMM