Re: [PATCH 2/3] GHES: Move memory initialisation to ghes_probe()

[Borislav Petkov]

On Tue, Aug 01, 2017 at 02:36:07PM +0100, Punit Agrawal wrote:
> During the driver init, the ghes driver initialises some data structures

Let's stick to "GHES" in capital letters everywhere.

> which are only used if a GHES platform device is probed. Similarly, the
> init function also checks for support for firmware first mode.
> 
> Create a function, ghes_common_init(), that performs the initialisations
> and checks that are currently performed on driver init. The function is
> called when the GHES device is probed.
> 
> Delaying initialisation and checks until probe has the added benefit of
> reducing driver prints on systems that do not support ACPI APEI.
> 
> Signed-off-by: Punit Agrawal <punit.agrwal@xxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: James Morse <james.morse@xxxxxxx>
> ---
>  drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 007b38abcb34..befb18338acb 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -1088,12 +1088,53 @@ static inline void ghes_nmi_init_cxt(void)
>  }
>  #endif /* CONFIG_HAVE_ACPI_APEI_NMI */
>  
> +static int ghes_common_init(void)
> +{
> +	int rc;
> +	static bool initialised;
> +
> +	if (initialised)
> +		return 0;

Can't say that I like it. Especially for this "initialised" thing, which
practically says that this code doesn't conceptually belong here because
we have to explicitly force it to execute it only once. Even though we
have execute-once path in ghes_init().

What would be much better IMHO is if ghes_init() checked for some APEI
table which is missing on your system and exited early. That would save
us all the trouble and solve the situation properly ...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--