| Message ID | 20170801133608.21017-3-punit.agrawal@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Punit Agrawal <punit.agrawal@arm.com> writes: > During the driver init, the ghes driver initialises some data structures > 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@arm.com> I managed to get my email wrong in the signed-off-by. And git send-email usage propagated it to the senders. Corrected it locally but will wait for comments on the code before re-sending. > Cc: Borislav Petkov <bp@suse.de> > Cc: James Morse <james.morse@arm.com> > --- > 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; > + > + ghes_nmi_init_cxt(); > + > + rc = ghes_ioremap_init(); > + if (rc) > + goto err; > + > + rc = ghes_estatus_pool_init(); > + if (rc) > + goto err_ioremap_exit; > + > + rc = apei_osc_setup(); > + if (rc == 0 && osc_sb_apei_support_acked) > + pr_info(GHES_PFX "APEI firmware first mode is enabled by APEI bit and WHEA _OSC.\n"); > + else if (rc == 0 && !osc_sb_apei_support_acked) > + pr_info(GHES_PFX "APEI firmware first mode is enabled by WHEA _OSC.\n"); > + else if (rc && osc_sb_apei_support_acked) > + 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"); > + > + initialised = true; > + return 0; > + > +err_ioremap_exit: > + ghes_ioremap_exit(); > +err: > + return rc; > +} > + > static int ghes_probe(struct platform_device *ghes_dev) > { > struct acpi_hest_generic *generic; > struct ghes *ghes = NULL; > > - int rc = -EINVAL; > + int rc; > + > + rc = ghes_common_init(); > + if (rc) > + return rc; > > generic = *(struct acpi_hest_generic **)ghes_dev->dev.platform_data; > if (!generic->enabled) > @@ -1268,8 +1309,6 @@ static struct platform_driver ghes_platform_driver = { > > static int __init ghes_init(void) > { > - int rc; > - > if (acpi_disabled) > return -ENODEV; > > @@ -1283,36 +1322,6 @@ static int __init ghes_init(void) > return -EINVAL; > } > > - ghes_nmi_init_cxt(); > - > - rc = ghes_ioremap_init(); > - if (rc) > - goto err; > - > - rc = ghes_estatus_pool_init(); > - if (rc) > - goto err_ioremap_exit; > - > - rc = platform_driver_register(&ghes_platform_driver); > - if (rc) > - goto err_pool_exit; > - > - rc = apei_osc_setup(); > - if (rc == 0 && osc_sb_apei_support_acked) > - pr_info(GHES_PFX "APEI firmware first mode is enabled by APEI bit and WHEA _OSC.\n"); > - else if (rc == 0 && !osc_sb_apei_support_acked) > - pr_info(GHES_PFX "APEI firmware first mode is enabled by WHEA _OSC.\n"); > - else if (rc && osc_sb_apei_support_acked) > - 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_pool_exit: > - ghes_estatus_pool_exit(); > -err_ioremap_exit: > - ghes_ioremap_exit(); > -err: > - return rc; > + return platform_driver_register(&ghes_platform_driver); > } > device_initcall(ghes_init); -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@arm.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: James Morse <james.morse@arm.com> > --- > 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 ...
On Tue, Aug 15, 2017 at 11:10:05AM +0100, Punit Agrawal wrote: > In the absence of any GHES entries these are wasted. I know. This whole APEI init code would need a proper cleanup, like acpi_pci_root_init() calls acpi_hest_init() and it shouldn't have to communicate through a variable with GHES whether to init or not but it should initialize GHES itself. And ghes_init() being a device_initcall() is just yuck. Something for the todo list I guess... > The system does not provide the Hardware Error Source Table (HEST) which > is checked in the hest driver (drivers/acpi/apei/hest.c). > > I think I'll go with your original suggestion to change hest_disable > from a boolean to something with more states. Re-checking for the HEST > table again feels like duplication. > > Makes sense? Right, and then depending on the setting of hest_disable, either issue the "HEST is not enabled" message or not. Thanks.
Hi Boris, Borislav Petkov <bp@suse.de> writes: > 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@arm.com> >> Cc: Borislav Petkov <bp@suse.de> >> Cc: James Morse <james.morse@arm.com> >> --- >> 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(). I can see your point. My rationale for the change was that the allocated resources are only ever useful if there is an actual GHES device in the system - which we can't know until we hit probe. In the absence of any GHES entries these are wasted. > > 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 ... The system does not provide the Hardware Error Source Table (HEST) which is checked in the hest driver (drivers/acpi/apei/hest.c). I think I'll go with your original suggestion to change hest_disable from a boolean to something with more states. Re-checking for the HEST table again feels like duplication. Makes sense? Thanks for taking a look at the patches. Punit > > -- > Regards/Gruss, > Boris. > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ thanks for dropping the bad email! ] Borislav Petkov <bp@suse.de> writes: > On Tue, Aug 15, 2017 at 11:10:05AM +0100, Punit Agrawal wrote: >> In the absence of any GHES entries these are wasted. > > I know. This whole APEI init code would need a proper cleanup, like > acpi_pci_root_init() calls acpi_hest_init() and it shouldn't have to > communicate through a variable with GHES whether to init or not but it > should initialize GHES itself. And ghes_init() being a device_initcall() > is just yuck. I'd seen the link between acpi_pci_root_init() and acpi_hest_init() but didn't want to open another can of worms... :) $SUBJECT was my attempt (small) at improving the situation but I guess I didn't go far enough. > > Something for the todo list I guess... > >> The system does not provide the Hardware Error Source Table (HEST) which >> is checked in the hest driver (drivers/acpi/apei/hest.c). >> >> I think I'll go with your original suggestion to change hest_disable >> from a boolean to something with more states. Re-checking for the HEST >> table again feels like duplication. >> >> Makes sense? > > Right, and then depending on the setting of hest_disable, either issue > the "HEST is not enabled" message or not. Ack! > > Thanks. > > -- > Regards/Gruss, > Boris. > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 15, 2017 at 11:31:57AM +0100, Punit Agrawal wrote: > I'd seen the link between acpi_pci_root_init() and acpi_hest_init() but > didn't want to open another can of worms... :) > > $SUBJECT was my attempt (small) at improving the situation but I guess I > didn't go far enough. Well, one fine day, if you feel bored and your hands are itching to do some hacking, I won't be mad atcha if you did a second try... :-)))
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; + + ghes_nmi_init_cxt(); + + rc = ghes_ioremap_init(); + if (rc) + goto err; + + rc = ghes_estatus_pool_init(); + if (rc) + goto err_ioremap_exit; + + rc = apei_osc_setup(); + if (rc == 0 && osc_sb_apei_support_acked) + pr_info(GHES_PFX "APEI firmware first mode is enabled by APEI bit and WHEA _OSC.\n"); + else if (rc == 0 && !osc_sb_apei_support_acked) + pr_info(GHES_PFX "APEI firmware first mode is enabled by WHEA _OSC.\n"); + else if (rc && osc_sb_apei_support_acked) + 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"); + + initialised = true; + return 0; + +err_ioremap_exit: + ghes_ioremap_exit(); +err: + return rc; +} + static int ghes_probe(struct platform_device *ghes_dev) { struct acpi_hest_generic *generic; struct ghes *ghes = NULL; - int rc = -EINVAL; + int rc; + + rc = ghes_common_init(); + if (rc) + return rc; generic = *(struct acpi_hest_generic **)ghes_dev->dev.platform_data; if (!generic->enabled) @@ -1268,8 +1309,6 @@ static struct platform_driver ghes_platform_driver = { static int __init ghes_init(void) { - int rc; - if (acpi_disabled) return -ENODEV; @@ -1283,36 +1322,6 @@ static int __init ghes_init(void) return -EINVAL; } - ghes_nmi_init_cxt(); - - rc = ghes_ioremap_init(); - if (rc) - goto err; - - rc = ghes_estatus_pool_init(); - if (rc) - goto err_ioremap_exit; - - rc = platform_driver_register(&ghes_platform_driver); - if (rc) - goto err_pool_exit; - - rc = apei_osc_setup(); - if (rc == 0 && osc_sb_apei_support_acked) - pr_info(GHES_PFX "APEI firmware first mode is enabled by APEI bit and WHEA _OSC.\n"); - else if (rc == 0 && !osc_sb_apei_support_acked) - pr_info(GHES_PFX "APEI firmware first mode is enabled by WHEA _OSC.\n"); - else if (rc && osc_sb_apei_support_acked) - 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_pool_exit: - ghes_estatus_pool_exit(); -err_ioremap_exit: - ghes_ioremap_exit(); -err: - return rc; + return platform_driver_register(&ghes_platform_driver); } device_initcall(ghes_init);
During the driver init, the ghes driver initialises some data structures 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@arm.com> Cc: Borislav Petkov <bp@suse.de> Cc: James Morse <james.morse@arm.com> --- drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 34 deletions(-)