Patchwork [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM

login
register
mail settings
Submitter Borislav Petkov
Date Oct. 31, 2018, 6 p.m.
Message ID <20181031180033.GM15955@zn.tnic>
Download mbox | patch
Permalink /patch/647177/
State New
Headers show

Comments

Borislav Petkov - Oct. 31, 2018, 6 p.m.
On Wed, Oct 31, 2018 at 10:57:54AM -0700, Luck, Tony wrote:
> That make Kconfig happy, but leads to a couple of link errors:
> 
>   MODPOST vmlinux.o
> drivers/edac/skx_edac.o: In function `skx_mce_check_error':
> skx_edac.c:(.text+0xab): undefined reference to `adxl_decode'
> drivers/edac/skx_edac.o: In function `skx_init':
> skx_edac.c:(.init.text+0x863): undefined reference to `adxl_get_component_names'
> make: *** [Makefile:1036: vmlinux] Error 1
> 
> Perhaps Boris is right and we do need to make ACPI_ADXL user selectable,
> and have the skylake EDAC driver "depends on ACPI_ADXL" :-(

That seems to work:

---
Luck, Tony - Oct. 31, 2018, 6:08 p.m.
On Wed, Oct 31, 2018 at 07:00:33PM +0100, Borislav Petkov wrote:
> +#ifdef CONFIG_ACPI_ADXL
>  const char * const *adxl_get_component_names(void);
>  int adxl_decode(u64 addr, u64 component_values[]);
> +#else
> +static inline const char * const *adxl_get_component_names(void)  { return NULL; }
> +static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }
> +#endif
>  
>  #endif /* _LINUX_ADXL_H */

It's not exactly ideal to build the skx_edac.c driver with these
stubs. Sure, it make the kernel link without errors. But now you
silently end up with a driver that doesn't really do all you want
it to do.

Perhaps this isn't a huge issue. Only "randconfig" would try to
build a kernel without ACPI. The user will find other stuff is
broken in an ACPI=n kernel long before they notice the lack of
EDAC error reporting.

So:

Acked-by: Tony Luck <tony.luck@intel.com>

-Tony
Luck, Tony - Oct. 31, 2018, 6:12 p.m.
Oops. Return from this isn't "bool". Caller expects 0 = success.

> +static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }

This should return -EOPNOTSUPP;

-Tony
Borislav Petkov - Oct. 31, 2018, 6:16 p.m.
On Wed, Oct 31, 2018 at 11:08:01AM -0700, Luck, Tony wrote:
> It's not exactly ideal to build the skx_edac.c driver with these
> stubs. Sure, it make the kernel link without errors. But now you
> silently end up with a driver that doesn't really do all you want
> it to do.
> 
> Perhaps this isn't a huge issue. Only "randconfig" would try to
> build a kernel without ACPI. The user will find other stuff is
> broken in an ACPI=n kernel long before they notice the lack of
> EDAC error reporting.

Exactly!

> So:
> 
> Acked-by: Tony Luck <tony.luck@intel.com>

Thx. I'll add it to the lineup for Linus.

> Oops. Return from this isn't "bool". Caller expects 0 = success.
> 
> > +static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }
> 
> This should return -EOPNOTSUPP;

Done.

Thx.

Patch

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index ffd349c12479..68e479b4d9c9 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -234,7 +234,7 @@  config EDAC_SKX
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
 	depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y
 	select DMI
-	select ACPI_ADXL
+	select ACPI_ADXL if ACPI
 	help
 	  Support for error detection and correction the Intel
 	  Skylake server Integrated Memory Controllers. If your
diff --git a/include/linux/adxl.h b/include/linux/adxl.h
index 2a629acb4c3f..6d04991962b0 100644
--- a/include/linux/adxl.h
+++ b/include/linux/adxl.h
@@ -7,7 +7,12 @@ 
 #ifndef _LINUX_ADXL_H
 #define _LINUX_ADXL_H
 
+#ifdef CONFIG_ACPI_ADXL
 const char * const *adxl_get_component_names(void);
 int adxl_decode(u64 addr, u64 component_values[]);
+#else
+static inline const char * const *adxl_get_component_names(void)  { return NULL; }
+static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }
+#endif
 
 #endif /* _LINUX_ADXL_H */