Re: [PATCH v7 1/2] ACPI: APEI: explicit init HEST and GHES in apci_init

[Shuai Xue <xueshuai@linux.alibaba.com>]

Hi, Nathan and Rafael,

在 2022/2/22 AM2:18, Nathan Chancellor 写道:
> Hi Shuai,
> 
> On Sat, Jan 22, 2022 at 01:26:17PM +0800, Shuai Xue wrote:
>> From commit e147133a42cb ("ACPI / APEI: Make hest.c manage the estatus
>> memory pool") was merged, ghes_init() relies on acpi_hest_init() to manage
>> the estatus memory pool. On the other hand, ghes_init() relies on
>> sdei_init() to detect the SDEI version and (un)register events. The
>> dependencies are as follows:
>>
>>     ghes_init() => acpi_hest_init() => acpi_bus_init() => acpi_init()
>>     ghes_init() => sdei_init()
>>
>> HEST is not PCI-specific and initcall ordering is implicit and not
>> well-defined within a level.
>>
>> Based on above, remove acpi_hest_init() from acpi_pci_root_init() and
>> convert ghes_init() and sdei_init() from initcalls to explicit calls in the
>> following order:
>>
>>     acpi_hest_init()
>>     ghes_init()
>>         sdei_init()
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>  drivers/acpi/apei/ghes.c    | 19 ++++++++-----------
>>  drivers/acpi/bus.c          |  2 ++
>>  drivers/acpi/pci_root.c     |  3 ---
>>  drivers/firmware/Kconfig    |  1 +
>>  drivers/firmware/arm_sdei.c | 13 ++-----------
>>  include/acpi/apei.h         |  4 +++-
>>  include/linux/arm_sdei.h    |  2 ++
>>  7 files changed, 18 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 0c5c9acc6254..aadc0a972f18 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -1457,33 +1457,35 @@ static struct platform_driver ghes_platform_driver = {
>>  	.remove		= ghes_remove,
>>  };
>>  
>> -static int __init ghes_init(void)
>> +void __init ghes_init(void)
>>  {
>>  	int rc;
>>  
>> +	sdei_init();
>> +
>>  	if (acpi_disabled)
>> -		return -ENODEV;
>> +		return;
>>  
>>  	switch (hest_disable) {
>>  	case HEST_NOT_FOUND:
>> -		return -ENODEV;
>> +		return;
>>  	case HEST_DISABLED:
>>  		pr_info(GHES_PFX "HEST is not enabled!\n");
>> -		return -EINVAL;
>> +		return;
>>  	default:
>>  		break;
>>  	}
>>  
>>  	if (ghes_disable) {
>>  		pr_info(GHES_PFX "GHES is not enabled!\n");
>> -		return -EINVAL;
>> +		return;
>>  	}
>>  
>>  	ghes_nmi_init_cxt();
>>  
>>  	rc = platform_driver_register(&ghes_platform_driver);
>>  	if (rc)
>> -		goto err;
>> +		return;
>>  
>>  	rc = apei_osc_setup();
>>  	if (rc == 0 && osc_sb_apei_support_acked)
>> @@ -1494,9 +1496,4 @@ static int __init ghes_init(void)
>>  		pr_info(GHES_PFX "APEI firmware first mode is enabled by APEI bit.\n");
>>  	else
>>  		pr_info(GHES_PFX "Failed to enable APEI firmware first mode.\n");
>> -
>> -	return 0;
>> -err:
>> -	return rc;
>>  }
>> -device_initcall(ghes_init);
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 07f604832fd6..3f403db20f69 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1331,6 +1331,8 @@ static int __init acpi_init(void)
>>  
>>  	pci_mmcfg_late_init();
>>  	acpi_iort_init();
>> +	acpi_hest_init();
>> +	ghes_init();
>>  	acpi_scan_init();
>>  	acpi_ec_init();
>>  	acpi_debugfs_init();
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index b76db99cced3..6f9e75d14808 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -22,8 +22,6 @@
>>  #include <linux/slab.h>
>>  #include <linux/dmi.h>
>>  #include <linux/platform_data/x86/apple.h>
>> -#include <acpi/apei.h>	/* for acpi_hest_init() */
>> -
>>  #include "internal.h"
>>  
>>  #define ACPI_PCI_ROOT_CLASS		"pci_bridge"
>> @@ -943,7 +941,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>  
>>  void __init acpi_pci_root_init(void)
>>  {
>> -	acpi_hest_init();
>>  	if (acpi_pci_disabled)
>>  		return;
>>  
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 75cb91055c17..ad114d9cdf8e 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -40,6 +40,7 @@ config ARM_SCPI_POWER_DOMAIN
>>  config ARM_SDE_INTERFACE
>>  	bool "ARM Software Delegated Exception Interface (SDEI)"
>>  	depends on ARM64
>> +	select ACPI_APEI_GHES
> 
> As the kernel test robot pointed out [1], you cannot do this.
> CONFIG_ACPI_APEI_GHES is a user selectable symbol that has dependencies,
> which 'select' completely overrides, resulting in build failures when
> CONFIG_ACPI_APEI is not enabled.
> 
> If CONFIG_ARM_SDE_INTERFACE truly requires CONFIG_ACPI_APEI_GHES, you
> should have "depends on ACPI_APEI_GHES".
> 
> If CONFIG_ARM_SDE_INTERFACE soft depends on CONFIG_ACPI_APEI_GHES for
> functionality but can work without it, you could use
> "imply ACPI_APEI_GHES", which will enable CONFIG_ACPI_APEI_GHES if its
> dependencies are met.
> 
> I noticed the same error with Alpine Linux's aarch64 configuration [2]
> if you wanted a quick configuration to test with.
> 
> [1]: https://lore.kernel.org/r/202202151504.jWpZGPaH-lkp@intel.com/
> [2]: https://git.alpinelinux.org/aports/plain/community/linux-edge/config-edge.aarch64

Thank you for explaining the difference among 'select', 'depends on' and 'imply'.
I was wrong to use 'select'. sdei_init() is called in ghes_init() now, in other words,
CONFIG_ARM_SDE_INTERFACE truly requires CONFIG_ACPI_APEI_GHES, so we should use
'depends on'. I will send a new patch set to fix this problem.

Best Regards,
Shuai