linux-kernel - Re: [PATCH v3 1/7] arm/virt: place power button pin number on a define

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240801151544.2f315598@foz.lan>
Date: Thu, 1 Aug 2024 15:15:44 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Igor Mammedov <imammedo@...hat.com>
Cc: Peter Maydell <peter.maydell@...aro.org>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, Shiju Jose <shiju.jose@...wei.com>, "Michael
 S. Tsirkin" <mst@...hat.com>, Ani Sinha <anisinha@...hat.com>, Shannon Zhao
 <shannon.zhaosl@...il.com>, linux-kernel@...r.kernel.org,
 qemu-arm@...gnu.org, qemu-devel@...gnu.org
Subject: Re: [PATCH v3 1/7] arm/virt: place power button pin number on a
 define

Em Tue, 30 Jul 2024 13:26:20 +0200
Igor Mammedov <imammedo@...hat.com> escreveu:

> On Tue, 30 Jul 2024 09:29:37 +0100
> Peter Maydell <peter.maydell@...aro.org> wrote:
> 
> > On Tue, 30 Jul 2024 at 08:26, Igor Mammedov <imammedo@...hat.com> wrote:  
> > >
> > > On Mon, 22 Jul 2024 08:45:53 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> > >    
> > > > Having magic numbers inside the code is not a good idea, as it
> > > > is error-prone. So, instead, create a macro with the number
> > > > definition.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>    
> >   
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index b0c68d66a345..c99c8b1713c6 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -1004,7 +1004,7 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
> > > >      if (s->acpi_dev) {
> > > >          acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> > > >      } else {
> > > > -        /* use gpio Pin 3 for power button event */
> > > > +        /* use gpio Pin for power button event */
> > > >          qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);    
> > >
> > > /me confused, it was saying Pin 3 but is passing 0 as argument where as elsewhere
> > > you are passing 3. Is this a bug?    
> > 
> > No. The gpio_key_dev is a gpio-key device which has one
> > input (which you assert to "press the key") and one output,
> > which goes high when the key is pressed and then falls
> > 100ms later. The virt board wires up the output of the
> > gpio-key device to input 3 on the PL061 GPIO controller.
> > (This happens in create_gpio_keys().) So the code is correct
> > to assert input 0 on the gpio-key device and the comment
> > isn't wrong that this results in GPIO pin 3 being asserted:
> > the link is just indirect.  
> 
> it's likely obvious to ARM folks, but maybe comment should
> clarify above for unaware.

Not sure if a comment here with the pin number is a good idea.
After all, this patch was originated because we were using
Pin 6 for GPIO error, while the comment was outdated (stating
that it was pin 8 instead) :-)

After this series, there will be two GPIO pins used inside arm/virt,
both defined at arm/virt.h:

	/* GPIO pins */
	#define GPIO_PIN_POWER_BUTTON  3
	#define GPIO_PIN_GENERIC_ERROR 6

Those macros are used when GPIOs are created:

	static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
	                             uint32_t phandle)
	{
	    gpio_key_dev = sysbus_create_simple("gpio-key", -1,
	                                        qdev_get_gpio_in(pl061_dev,
                                                         GPIO_PIN_POWER_BUTTON));
	    gpio_error_dev = sysbus_create_simple("gpio-key", -1,
	                                          qdev_get_gpio_in(pl061_dev,
	                                                           GPIO_PIN_GENERIC_ERROR));
So, at least for me, it is clear that gpio_key_dev is using pin 3.

Thanks,
Mauro