Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Erik Kaneda <erik.kaneda@intel.com>,
        Bob Moore <robert.moore@intel.com>
References: <1748021.N9i9sLPJ40@kreacher> <9373262.piL2bvXoCD@kreacher>
From:   Matthieu Baerts <matthieu.baerts@tessares.net>
Subject: Re: [PATCH 4/6] ACPICA: Add support for using logical addresses of
 GPE blocks
Message-ID: <3b69e0d0-fb8a-92b4-42fd-f2a8fcdd642b@tessares.net>
Date:   Fri, 16 Oct 2020 16:30:55 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.2
MIME-Version: 1.0
In-Reply-To: <9373262.piL2bvXoCD@kreacher>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Rafael,

On 04/09/2020 19:24, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> The logical address of every GPE block in system memory must be
> known before passing it to acpi_ev_initialize_gpe_block(), because
> memory cannot be mapped on the fly from an interrupt handler.
> Accordingly, the host OS must map every GPE block in system
> memory upfront and it can store the logical addresses of GPE
> blocks for future use.

(...)

> diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
> index a0e71f34c77a..37bb67ef3232 100644
> --- a/drivers/acpi/acpica/hwgpe.c
> +++ b/drivers/acpi/acpica/hwgpe.c
> @@ -46,8 +46,13 @@ acpi_status acpi_hw_gpe_read(u64 *value, struct acpi_gpe_address *reg)
>   	u32 value32;
>   
>   	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +#ifdef ACPI_GPE_USE_LOGICAL_ADDRESSES
> +		*value = (u64)ACPI_GET8(reg->address);

Thank you for the patch!

When compiling net-next repo, recently sync with Linus repo, I got an 
error when using i386 arch because of this line above.

Here are the commands I used:


================================================
$ make defconfig KBUILD_DEFCONFIG=i386_defconfig
*** Default configuration is based on 'i386_defconfig'
#
# configuration written to .config
#
$ scripts/config --disable DRM --disable PCCARD --disable ATA --disable 
MD --disable PPS --disable SOUND --disable USB --disable IOMMU_SUPPORT 
--disable INPUT_LEDS --disable AGP --disable VGA_ARB --disable EFI 
--disable WLAN --disable WIRELESS --disable LOGO --disable NFS_FS 
--disable XFRM_USER --disable INET6_AH --disable INET6_ESP --disable 
NETDEVICES -e KUNIT -d KUNIT_DEBUGFS -d KUNIT_TEST -d KUNIT_EXAMPLE_TEST 
-d EXT4_KUNIT_TESTS -d SYSCTL_KUNIT_TEST -d LIST_KUNIT_TEST -d 
LINEAR_RANGES_TEST -d BITS_TEST -d KUNIT_ALL_TESTS -e INET_DIAG -d 
INET_UDP_DIAG -d INET_RAW_DIAG -d INET_DIAG_DESTROY -e MPTCP -e 
MPTCP_IPV6 -e MPTCP_KUNIT_TESTS
$ KCFLAGS=-Werror make -j8 -l8
scripts/kconfig/conf  --syncconfig Kconfig
(...)
   CC      drivers/acpi/acpica/hwgpe.o
In file included from ./include/acpi/acpi.h:24,
                  from drivers/acpi/acpica/hwgpe.c:10:
drivers/acpi/acpica/hwgpe.c: In function 'acpi_hw_gpe_read':
./include/acpi/actypes.h:501:48: error: cast to pointer from integer of 
different size [-Werror=int-to-pointer-cast]
   501 | #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) 
(p))
       |                                                ^
drivers/acpi/acpica/acmacros.h:18:41: note: in expansion of macro 
'ACPI_CAST_PTR'
    18 | #define ACPI_CAST8(ptr)                 ACPI_CAST_PTR (u8, (ptr))
       |                                         ^~~~~~~~~~~~~
drivers/acpi/acpica/acmacros.h:22:43: note: in expansion of macro 
'ACPI_CAST8'
    22 | #define ACPI_GET8(ptr)                  (*ACPI_CAST8 (ptr))
       |                                           ^~~~~~~~~~
drivers/acpi/acpica/hwgpe.c:50:17: note: in expansion of macro 'ACPI_GET8'
    50 |   *value = (u64)ACPI_GET8(reg->address);
       |                 ^~~~~~~~~
drivers/acpi/acpica/hwgpe.c: In function 'acpi_hw_gpe_write':
./include/acpi/actypes.h:501:48: error: cast to pointer from integer of 
different size [-Werror=int-to-pointer-cast]
   501 | #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) 
(p))
       |                                                ^
drivers/acpi/acpica/acmacros.h:18:41: note: in expansion of macro 
'ACPI_CAST_PTR'
    18 | #define ACPI_CAST8(ptr)                 ACPI_CAST_PTR (u8, (ptr))
       |                                         ^~~~~~~~~~~~~
drivers/acpi/acpica/acmacros.h:26:43: note: in expansion of macro 
'ACPI_CAST8'
    26 | #define ACPI_SET8(ptr, val)             (*ACPI_CAST8 (ptr) = 
(u8) (val))
       |                                           ^~~~~~~~~~
drivers/acpi/acpica/hwgpe.c:85:3: note: in expansion of macro 'ACPI_SET8'
    85 |   ACPI_SET8(reg->address, value);
       |   ^~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:283: drivers/acpi/acpica/hwgpe.o] 
Error 1
make[2]: *** [scripts/Makefile.build:500: drivers/acpi/acpica] Error 2
make[1]: *** [scripts/Makefile.build:500: drivers/acpi] Error 2
make: *** [Makefile:1777: drivers] Error 2
================================================


> +		return_ACPI_STATUS(AE_OK);
> +#else
>   		return acpi_os_read_memory((acpi_physical_address)reg->address,
>   					    value, ACPI_GPE_REGISTER_WIDTH);
> +#endif
>   	}
>   
>   	status = acpi_os_read_port((acpi_io_address)reg->address,
> @@ -76,8 +81,13 @@ acpi_status acpi_hw_gpe_read(u64 *value, struct acpi_gpe_address *reg)
>   acpi_status acpi_hw_gpe_write(u64 value, struct acpi_gpe_address *reg)
>   {
>   	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +#ifdef ACPI_GPE_USE_LOGICAL_ADDRESSES
> +		ACPI_SET8(reg->address, value);

(and also because of this line)

By chance, do you already have a fix for that? I didn't see any other 
email related to this issue, I am surprised no bot already reported the 
problem but maybe I didn't look everywhere :)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net