Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703458 > unrolled thread

[PATCH v2 0/7] enable ghes_edac on selected platforms

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/7] enable ghes_edac on selected platforms Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
    [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
      Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist()  interface Borislav Petkov <bp@alien8.de> - 2017-08-04 05:50 +0200
        Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist()  interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 22:40 +0200
          Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist()  interface Borislav Petkov <bp@alien8.de> - 2017-08-05 07:20 +0200
            Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist()  interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-07 16:50 +0200
    [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
      Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() Borislav Petkov <bp@alien8.de> - 2017-08-04 06:10 +0200
        Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 23:10 +0200
          Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() Borislav Petkov <bp@alien8.de> - 2017-08-05 07:20 +0200
            Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-07 20:00 +0200
              Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk() Borislav Petkov <bp@alien8.de> - 2017-08-11 11:10 +0200
    [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
      Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner Borislav Petkov <bp@alien8.de> - 2017-08-04 10:40 +0200
        Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 23:40 +0200
          Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner Borislav Petkov <bp@alien8.de> - 2017-08-05 07:50 +0200
            Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-07 17:00 +0200
      Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner kbuild test robot <lkp@intel.com> - 2017-08-04 15:10 +0200
        Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 17:30 +0200
    [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
      Re: [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-04 10:40 +0200
        Re: [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 23:10 +0200
          Re: [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-05 07:40 +0200
            Re: [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-07 17:00 +0200
    [PATCH v2 3/7] ACPI / APEI: add OSC APEI bit check for ghes_edac Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200
      Re: [PATCH v2 3/7] ACPI / APEI: add OSC APEI bit check for ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-04 05:50 +0200
        Re: [PATCH v2 3/7] ACPI / APEI: add OSC APEI bit check for ghes_edac "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-04 23:00 +0200
          Re: [PATCH v2 3/7] ACPI / APEI: add OSC APEI bit check for ghes_edac Borislav Petkov <bp@alien8.de> - 2017-08-05 07:20 +0200
            Re: [PATCH v2 3/7] ACPI / APEI: add OSC APEI bit check for ghes_edac "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-08-07 17:00 +0200
    [PATCH v2 2/7] intel_pstate: convert to use acpi_match_oemlist() Toshi Kani <toshi.kani@hpe.com> - 2017-08-04 00:10 +0200

Page 1 of 2  [1] 2  Next page →

#1703458 — [PATCH v2 0/7] enable ghes_edac on selected platforms

The ghes_edac driver was introduced in 2013 [1], but it has not been
enabled by any distro yet.  This is because the driver obtains error
info from firmware interfaces, which are not properly implemented on
many platforms.

To get out from this situation, add a platform check to selectively
enable the driver on the platforms that are known to have proper
firmware implementation.  Platform vendors can add their platforms to
the list when they support ghes_edac.

Patch 1-2 introduces a common function for platform check.
Patch 3 improves GHES check for ghes_edac.
Patch 4 optimizes ghes_edac init code.
Patch 5 introduces platform check to ghes_edac.
Patch 6-7 optimizes regular edac driver's init code when ghes_edac is used.

v2:
 - Address review comments (patch 1).
 - Add OSC APEI check (patch 3).
 - Avoid multiple dmi_walk (patch 4).
 - Add EDAC MC owner check (patch 6,7)

---
Toshi Kani (7):
 1/7 ACPI / blacklist: add acpi_match_oemlist() interface
 2/7 intel_pstate: convert to use acpi_match_oemlist()
 3/7 ACPI / APEI: add OSC APEI bit check for ghes_edac
 4/7 ghes_edac: avoid multiple calls to dmi_walk()
 5/7 ghes_edac: add platform check to enable ghes_edac
 6/7 EDAC: add edac_check_mc_owner() to check MC owner
 7/7 edac drivers: add MC owner check in init

---
 drivers/acpi/apei/ghes.c       |  8 ++--
 drivers/acpi/blacklist.c       | 83 +++++++-----------------------------------
 drivers/acpi/utils.c           | 40 ++++++++++++++++++++
 drivers/cpufreq/intel_pstate.c | 64 +++++++++++++-------------------
 drivers/edac/amd64_edac.c      |  3 ++
 drivers/edac/edac_mc.c         | 15 +++++++-
 drivers/edac/edac_mc.h         | 12 ++++++
 drivers/edac/ghes_edac.c       | 36 ++++++++++++++----
 drivers/edac/pnd2_edac.c       |  7 +++-
 drivers/edac/sb_edac.c         |  7 +++-
 drivers/edac/skx_edac.c        |  6 ++-
 include/linux/acpi.h           | 19 ++++++++++
 12 files changed, 176 insertions(+), 124 deletions(-)

[toc] | [next] | [standalone]

#1703459 — [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface

ACPI OEM ID / OEM Table ID / Revision can be used to identify
a platform based on ACPI firmware info.  acpi_blacklisted(),
intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
have been using similar check to detect a list of platforms
that require special handlings.

Move the platform check in acpi_blacklisted() to a new common
utility function, acpi_match_oemlist(), so that other drivers
do not have to implement their own version.

There is no change in functionality.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Borislav Petkov <bp@alien8.de>
---
 drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
 drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
 include/linux/acpi.h     |   19 +++++++++++
 3 files changed, 73 insertions(+), 69 deletions(-)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index bb542ac..bf44573 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -30,30 +30,13 @@
 
 #include "internal.h"
 
-enum acpi_blacklist_predicates {
-	all_versions,
-	less_than_or_equal,
-	equal,
-	greater_than_or_equal,
-};
-
-struct acpi_blacklist_item {
-	char oem_id[7];
-	char oem_table_id[9];
-	u32 oem_revision;
-	char *table;
-	enum acpi_blacklist_predicates oem_revision_predicate;
-	char *reason;
-	u32 is_critical_error;
-};
-
 static struct dmi_system_id acpi_rev_dmi_table[] __initdata;
 
 /*
  * POLICY: If *anything* doesn't work, put it on the blacklist.
  *	   If they are critical errors, mark it critical, and abort driver load.
  */
-static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
+static struct acpi_oemlist acpi_blacklist[] __initdata = {
 	/* Compaq Presario 1700 */
 	{"PTLTD ", "  DSDT  ", 0x06040000, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Multiple problems", 1},
@@ -67,65 +50,27 @@ static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
 	{"IBM   ", "TP600E  ", 0x00000105, ACPI_SIG_DSDT, less_than_or_equal,
 	 "Incorrect _ADR", 1},
 
-	{""}
+	{ }
 };
 
 int __init acpi_blacklisted(void)
 {
-	int i = 0;
+	int i;
 	int blacklisted = 0;
-	struct acpi_table_header table_header;
-
-	while (acpi_blacklist[i].oem_id[0] != '\0') {
-		if (acpi_get_table_header(acpi_blacklist[i].table, 0, &table_header)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp(acpi_blacklist[i].oem_id, table_header.oem_id, 6)) {
-			i++;
-			continue;
-		}
-
-		if (strncmp
-		    (acpi_blacklist[i].oem_table_id, table_header.oem_table_id,
-		     8)) {
-			i++;
-			continue;
-		}
-
-		if ((acpi_blacklist[i].oem_revision_predicate == all_versions)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			less_than_or_equal
-			&& table_header.oem_revision <=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate ==
-			greater_than_or_equal
-			&& table_header.oem_revision >=
-			acpi_blacklist[i].oem_revision)
-		    || (acpi_blacklist[i].oem_revision_predicate == equal
-			&& table_header.oem_revision ==
-			acpi_blacklist[i].oem_revision)) {
 
-			printk(KERN_ERR PREFIX
-			       "Vendor \"%6.6s\" System \"%8.8s\" "
-			       "Revision 0x%x has a known ACPI BIOS problem.\n",
-			       acpi_blacklist[i].oem_id,
-			       acpi_blacklist[i].oem_table_id,
-			       acpi_blacklist[i].oem_revision);
+	i = acpi_match_oemlist(acpi_blacklist);
+	if (i >= 0) {
+		pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" Revision 0x%x has a known ACPI BIOS problem.\n",
+		       acpi_blacklist[i].oem_id,
+		       acpi_blacklist[i].oem_table_id,
+		       acpi_blacklist[i].oem_revision);
 
-			printk(KERN_ERR PREFIX
-			       "Reason: %s. This is a %s error\n",
-			       acpi_blacklist[i].reason,
-			       (acpi_blacklist[i].
-				is_critical_error ? "non-recoverable" :
-				"recoverable"));
+		pr_err(PREFIX "Reason: %s. This is a %s error\n",
+		       acpi_blacklist[i].reason,
+		       (acpi_blacklist[i].data ?
+			"non-recoverable" : "recoverable"));
 
-			blacklisted = acpi_blacklist[i].is_critical_error;
-			break;
-		} else {
-			i++;
-		}
+		blacklisted = acpi_blacklist[i].data;
 	}
 
 	(void)early_acpi_osi_init();
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index b9d956c..78b9422 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -816,3 +816,43 @@ static int __init acpi_backlight(char *str)
 	return 1;
 }
 __setup("acpi_backlight=", acpi_backlight);
+
+/**
+ * acpi_match_oemlist - Check if the system matches with an oem list
+ * @oem: pointer to acpi_oemlist table terminated by a NULL entry
+ *
+ * Return the matched index if the system is found in the oem list.
+ * Otherwise, return a negative error code.
+ */
+int acpi_match_oemlist(const struct acpi_oemlist *oem)
+{
+	struct acpi_table_header hdr;
+	int idx = 0;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	for (; oem->oem_id[0]; oem++, idx++) {
+		if (ACPI_FAILURE(acpi_get_table_header(oem->table, 0, &hdr)))
+			continue;
+
+		if (strncmp(oem->oem_id, hdr.oem_id, ACPI_OEM_ID_SIZE))
+			continue;
+
+		if (strncmp(oem->oem_table_id, hdr.oem_table_id,
+			    ACPI_OEM_TABLE_ID_SIZE))
+			continue;
+
+		if ((oem->pred == all_versions) ||
+		    (oem->pred == less_than_or_equal
+			&& hdr.oem_revision <= oem->oem_revision) ||
+		    (oem->pred == greater_than_or_equal
+			&& hdr.oem_revision >= oem->oem_revision) ||
+		    (oem->pred == equal
+			&& hdr.oem_revision == oem->oem_revision))
+			return idx;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(acpi_match_oemlist);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c749eef..6053acb 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -556,6 +556,25 @@ extern acpi_status acpi_pci_osc_control_set(acpi_handle handle,
 #define ACPI_OST_SC_DRIVER_LOAD_FAILURE		0x81
 #define ACPI_OST_SC_INSERT_NOT_SUPPORTED	0x82
 
+enum acpi_predicate {
+	all_versions,
+	less_than_or_equal,
+	equal,
+	greater_than_or_equal,
+};
+
+/* Table must be terminted by a NULL entry */
+struct acpi_oemlist {
+	char	oem_id[ACPI_OEM_ID_SIZE];
+	char	oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
+	u32	oem_revision;
+	char	*table;
+	enum acpi_predicate pred;
+	char	*reason;
+	u32	data;
+};
+int acpi_match_oemlist(const struct acpi_oemlist *oem);
+
 extern void acpi_early_init(void);
 extern void acpi_subsystem_init(void);
 

[toc] | [prev] | [next] | [standalone]

#1703628 — Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface

On Thu, Aug 03, 2017 at 03:57:47PM -0600, Toshi Kani wrote:
> ACPI OEM ID / OEM Table ID / Revision can be used to identify
> a platform based on ACPI firmware info.  acpi_blacklisted(),
> intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> have been using similar check to detect a list of platforms
> that require special handlings.
> 
> Move the platform check in acpi_blacklisted() to a new common
> utility function, acpi_match_oemlist(), so that other drivers
> do not have to implement their own version.
> 
> There is no change in functionality.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Borislav Petkov <bp@alien8.de>
> ---
>  drivers/acpi/blacklist.c |   83 ++++++++--------------------------------------
>  drivers/acpi/utils.c     |   40 ++++++++++++++++++++++
>  include/linux/acpi.h     |   19 +++++++++++
>  3 files changed, 73 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
> index bb542ac..bf44573 100644
> --- a/drivers/acpi/blacklist.c
> +++ b/drivers/acpi/blacklist.c
> @@ -30,30 +30,13 @@
>  
>  #include "internal.h"
>  
> -enum acpi_blacklist_predicates {
> -	all_versions,
> -	less_than_or_equal,
> -	equal,
> -	greater_than_or_equal,
> -};
> -
> -struct acpi_blacklist_item {
> -	char oem_id[7];
> -	char oem_table_id[9];
> -	u32 oem_revision;
> -	char *table;
> -	enum acpi_blacklist_predicates oem_revision_predicate;
> -	char *reason;
> -	u32 is_critical_error;
> -};
> -
>  static struct dmi_system_id acpi_rev_dmi_table[] __initdata;
>  
>  /*
>   * POLICY: If *anything* doesn't work, put it on the blacklist.
>   *	   If they are critical errors, mark it critical, and abort driver load.
>   */
> -static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
> +static struct acpi_oemlist acpi_blacklist[] __initdata = {

All that wasted energy to try to explain to you that "oemlist" is wrong
and that whole rename is pointless, went for nothing.

So NAK.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1704208 — Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface

On Fri, 2017-08-04 at 05:42 +0200, Borislav Petkov wrote:
> On Thu, Aug 03, 2017 at 03:57:47PM -0600, Toshi Kani wrote:
> > ACPI OEM ID / OEM Table ID / Revision can be used to identify
> > a platform based on ACPI firmware info.  acpi_blacklisted(),
> > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> > have been using similar check to detect a list of platforms
> > that require special handlings.
> > 
> > Move the platform check in acpi_blacklisted() to a new common
> > utility function, acpi_match_oemlist(), so that other drivers
> > do not have to implement their own version.
> > 
> > There is no change in functionality.
 :
> >  /*
> >   * POLICY: If *anything* doesn't work, put it on the blacklist.
> >   *	   If they are critical errors, mark it critical, and
> > abort driver load.
> >   */
> > -static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
> > +static struct acpi_oemlist acpi_blacklist[] __initdata = {
> 
> All that wasted energy to try to explain to you that "oemlist" is
> wrong and that whole rename is pointless, went for nothing.
>
> So NAK.

Well, we did talk a lot about your suggested name, "acpi_blacklist",
and I explained that it did not work since it'd be used for both black
and white-list.  We've agreed on that.

You then suggested it's "platform", not "OEM".  Since this is an ACPI
structure defined in "acpi.h", its terminology generally follows ACPI
spec, which I did.

I understand that you feel strongly against "OEM" (sorry about that). 
How about "acpi_platform_list"?  Does it work for you?

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1704567 — Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface

On Fri, Aug 04, 2017 at 08:39:35PM +0000, Kani, Toshimitsu wrote:
> Well, we did talk a lot about your suggested name, "acpi_blacklist",
> and I explained that it did not work since it'd be used for both black
> and white-list.  We've agreed on that.
> 
> You then suggested it's "platform", not "OEM".  Since this is an ACPI
> structure defined in "acpi.h", its terminology generally follows ACPI
> spec, which I did.
> 
> I understand that you feel strongly against "OEM" (sorry about that). 
> How about "acpi_platform_list"?  Does it work for you?

Yes, I thought we agreed on all that.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1705598 — Re: [PATCH v2 1/7] ACPI / blacklist: add acpi_match_oemlist() interface

On Sat, 2017-08-05 at 07:12 +0200, Borislav Petkov wrote:
> On Fri, Aug 04, 2017 at 08:39:35PM +0000, Kani, Toshimitsu wrote:
> > Well, we did talk a lot about your suggested name,
> > "acpi_blacklist", and I explained that it did not work since it'd
> > be used for both black and white-list.  We've agreed on that.
> > 
> > You then suggested it's "platform", not "OEM".  Since this is an
> > ACPI structure defined in "acpi.h", its terminology generally
> > follows ACPI spec, which I did.
> > 
> > I understand that you feel strongly against "OEM" (sorry about
> > that). How about "acpi_platform_list"?  Does it work for you?
> 
> Yes, I thought we agreed on all that.

Sounds good.  Will change to "acpi_platform_list".

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1703462 — [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

ghes_edac_register() is called for each GHES platform device
instantiated per a GHES entry in ACPI HEST table.  dmi_walk()
counts the number of DIMMs on the system, and there is no need
to call it multiple times.

Change ghes_edac_register() to call dmi_walk() only when
'num_dimm' is uninitialized.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 drivers/edac/ghes_edac.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 4e61a62..2e9ce9c 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -407,15 +407,18 @@ EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
 
 int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
-	bool fake = false;
-	int rc, num_dimm = 0;
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_pvt *pvt;
 	struct ghes_edac_dimm_fill dimm_fill;
+	int rc;
+
+	static int num_dimm;
+	static bool fake;
 
 	/* Get the number of DIMMs */
-	dmi_walk(ghes_edac_count_dimms, &num_dimm);
+	if (num_dimm == 0)
+		dmi_walk(ghes_edac_count_dimms, &num_dimm);
 
 	/* Check if we've got a bogus BIOS */
 	if (num_dimm == 0) {

[toc] | [prev] | [next] | [standalone]

#1703635 — Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Thu, Aug 03, 2017 at 03:57:50PM -0600, Toshi Kani wrote:
> ghes_edac_register() is called for each GHES platform device
> instantiated per a GHES entry in ACPI HEST table.  dmi_walk()
> counts the number of DIMMs on the system, and there is no need
> to call it multiple times.
> 
> Change ghes_edac_register() to call dmi_walk() only when
> 'num_dimm' is uninitialized.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
>  drivers/edac/ghes_edac.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 4e61a62..2e9ce9c 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -407,15 +407,18 @@ EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
>  
>  int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  {
> -	bool fake = false;
> -	int rc, num_dimm = 0;
>  	struct mem_ctl_info *mci;
>  	struct edac_mc_layer layers[1];
>  	struct ghes_edac_pvt *pvt;
>  	struct ghes_edac_dimm_fill dimm_fill;
> +	int rc;
> +
> +	static int num_dimm;
> +	static bool fake;
>  
>  	/* Get the number of DIMMs */
> -	dmi_walk(ghes_edac_count_dimms, &num_dimm);
> +	if (num_dimm == 0)
> +		dmi_walk(ghes_edac_count_dimms, &num_dimm);

So the problem is that ghes_edac_register() gets called multiple times
depending on how many GHES platform devices are on the system. But yet
they all scan *all* DIMMs. So instead you should return if the DIMMs
have been counted already and not register a second time.

Which makes that whole mc counting kinda useless. So you could rip that
out too.

Unless I'm missing something...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1704220 — Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Fri, 2017-08-04 at 06:05 +0200, Borislav Petkov wrote:
> On Thu, Aug 03, 2017 at 03:57:50PM -0600, Toshi Kani wrote:
> > ghes_edac_register() is called for each GHES platform device
> > instantiated per a GHES entry in ACPI HEST table.  dmi_walk()
> > counts the number of DIMMs on the system, and there is no need
> > to call it multiple times.
> > 
> > Change ghes_edac_register() to call dmi_walk() only when
> > 'num_dimm' is uninitialized.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Suggested-by: Borislav Petkov <bp@alien8.de>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > ---
> >  drivers/edac/ghes_edac.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index 4e61a62..2e9ce9c 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -407,15 +407,18 @@
> > EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
> >  
> >  int ghes_edac_register(struct ghes *ghes, struct device *dev)
> >  {
> > -	bool fake = false;
> > -	int rc, num_dimm = 0;
> >  	struct mem_ctl_info *mci;
> >  	struct edac_mc_layer layers[1];
> >  	struct ghes_edac_pvt *pvt;
> >  	struct ghes_edac_dimm_fill dimm_fill;
> > +	int rc;
> > +
> > +	static int num_dimm;
> > +	static bool fake;
> >  
> >  	/* Get the number of DIMMs */
> > -	dmi_walk(ghes_edac_count_dimms, &num_dimm);
> > +	if (num_dimm == 0)
> > +		dmi_walk(ghes_edac_count_dimms, &num_dimm);
> 
> So the problem is that ghes_edac_register() gets called multiple
> times depending on how many GHES platform devices are on the system.
> But yet they all scan *all* DIMMs. So instead you should return if
> the DIMMs have been counted already and not register a second time.
> 
> Which makes that whole mc counting kinda useless. So you could rip
> that out too.
> 
> Unless I'm missing something...

GHES platform devices correspond to GHES entries, which define firmware
interfaces to report generic memory errors to the OS, such as NMI and
SCI.  These devices are associated with all DIMMs, not a particular
DIMM.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1704569 — Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Fri, Aug 04, 2017 at 09:02:17PM +0000, Kani, Toshimitsu wrote:
> GHES platform devices correspond to GHES entries, which define firmware
> interfaces to report generic memory errors to the OS, such as NMI and
> SCI.  These devices are associated with all DIMMs, not a particular
> DIMM.

And? Stating the obvious brings you what exactly?

IOW, you still haven't answered my question.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1705764 — Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Sat, 2017-08-05 at 07:16 +0200, Borislav Petkov wrote:
> On Fri, Aug 04, 2017 at 09:02:17PM +0000, Kani, Toshimitsu wrote:
> > GHES platform devices correspond to GHES entries, which define
> > firmware interfaces to report generic memory errors to the OS, such
> > as NMI and SCI.  These devices are associated with all DIMMs, not a
> > particular DIMM.
> 
> And? Stating the obvious brings you what exactly?
> 
> IOW, you still haven't answered my question.

Sorry about that.  Let me copy your original comments to make sure I
answer your questions this time.

> So the problem is that ghes_edac_register() gets called multiple
> times depending on how many GHES platform devices are on the system.
> But yet they all scan *all* DIMMs. 

Right, and this patch changes ghes_edac_register() to scan all DIMMs at
the first time and do not scan them next times.

> So instead you should return if
> the DIMMs have been counted already and not register a second time.
> Which makes that whole mc counting kinda useless. So you could rip
>
that out too.

Oh, I see.  So, ghes_edac_register() should return no-op a second time,
and does not call edac_mc_add_mc() to register with a separate mci.

I think we should keep the current scheme, which registers an mci for
each GHES entry.  ghes_edac_report_mem_error() expects that error-
reporting is serialized per a GHES entry.  Sharing a single mci among
all GHES entries / error interfaces might lead to a race condition.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1709387 — Re: [PATCH v2 4/7] ghes_edac: avoid multiple calls to dmi_walk()

On Mon, Aug 07, 2017 at 05:59:15PM +0000, Kani, Toshimitsu wrote:
> I think we should keep the current scheme, which registers an mci for

No we shouldn't.

> each GHES entry.  ghes_edac_report_mem_error() expects that error-
> reporting is serialized per a GHES entry.  Sharing a single mci among
> all GHES entries / error interfaces might lead to a race condition.

See how I solved it in my patchset and feel free to reuse it.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1703464 — [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

Only a single edac driver can be enabled for EDAC MC.  When ghes_edac
is enabled, a regular edac driver for the CPU type / platform still
attempts to register itself and fails in edac_mc_add_mc().

Add edac_check_mc_owner() so that regular edac drivers can check
the owner of EDAC MC at the beginning of initialization.

Also change the owner check to string comparison from address check.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/edac_mc.c |   15 ++++++++++++++-
 drivers/edac/edac_mc.h |   12 ++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4800721..52d8d5e 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -701,6 +701,19 @@ struct mem_ctl_info *edac_mc_find(int idx)
 }
 EXPORT_SYMBOL(edac_mc_find);
 
+/*
+ * Returns:
+ *	1 when EDAC MC is free or owned by the module name
+ *	0 when EDAC MC is owned by other module
+ */
+int edac_check_mc_owner(const char *mod_name)
+{
+	if (edac_mc_owner && strcmp(edac_mc_owner, mod_name))
+		return 0;
+
+	return 1;
+}
+EXPORT_SYMBOL(edac_check_mc_owner);
 
 /* FIXME - should a warning be printed if no error detection? correction? */
 int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
@@ -742,7 +755,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 #endif
 	mutex_lock(&mem_ctls_mutex);
 
-	if (edac_mc_owner && edac_mc_owner != mci->mod_name) {
+	if (!edac_check_mc_owner(mci->mod_name)) {
 		ret = -EPERM;
 		goto fail0;
 	}
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 5357800..0e95eba 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -128,6 +128,18 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
 				   unsigned sz_pvt);
 
 /**
+ *
+ * edac_check_mc_owner - Check the owner of EDAC MC
+ *
+ * @mod_name: pointer to the module name
+ *
+ * Returns:
+ *	1 when EDAC MC is free or owned by the module name
+ *	0 when EDAC MC is owned by other module
+ */
+extern int edac_check_mc_owner(const char *mod_name);
+
+/*
  * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci
  *	global list and create sysfs entries associated with @mci structure.
  *

[toc] | [prev] | [next] | [standalone]

#1703792 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

On Thu, Aug 03, 2017 at 03:57:52PM -0600, Toshi Kani wrote:
> Only a single edac driver can be enabled for EDAC MC.  When ghes_edac
> is enabled, a regular edac driver for the CPU type / platform still
> attempts to register itself and fails in edac_mc_add_mc().
> 
> Add edac_check_mc_owner() so that regular edac drivers can check
> the owner of EDAC MC at the beginning of initialization.
> 
> Also change the owner check to string comparison from address check.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/edac/edac_mc.c |   15 ++++++++++++++-
>  drivers/edac/edac_mc.h |   12 ++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 4800721..52d8d5e 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -701,6 +701,19 @@ struct mem_ctl_info *edac_mc_find(int idx)
>  }
>  EXPORT_SYMBOL(edac_mc_find);
>  
> +/*
> + * Returns:
> + *	1 when EDAC MC is free or owned by the module name

1 for free OR owned?!? WTF?

> + *	0 when EDAC MC is owned by other module
> + */
> +int edac_check_mc_owner(const char *mod_name)
> +{
> +	if (edac_mc_owner && strcmp(edac_mc_owner, mod_name))

strncmp() of course, with sensible maximal string length.

> +		return 0;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL(edac_check_mc_owner);

EXPORT_SYMBOL_GPL

>  
>  /* FIXME - should a warning be printed if no error detection? correction? */
>  int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
> @@ -742,7 +755,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
>  #endif
>  	mutex_lock(&mem_ctls_mutex);
>  
> -	if (edac_mc_owner && edac_mc_owner != mci->mod_name) {
> +	if (!edac_check_mc_owner(mci->mod_name)) {
>  		ret = -EPERM;
>  		goto fail0;
>  	}
> diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
> index 5357800..0e95eba 100644
> --- a/drivers/edac/edac_mc.h
> +++ b/drivers/edac/edac_mc.h
> @@ -128,6 +128,18 @@ struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
>  				   unsigned sz_pvt);
>  
>  /**
> + *
> + * edac_check_mc_owner - Check the owner of EDAC MC
> + *
> + * @mod_name: pointer to the module name
> + *
> + * Returns:
> + *	1 when EDAC MC is free or owned by the module name
> + *	0 when EDAC MC is owned by other module
> + */

Documenting that function only once is enough.

> +extern int edac_check_mc_owner(const char *mod_name);
> +
> +/*
>   * edac_mc_add_mc_with_groups() - Insert the @mci structure into the mci
>   *	global list and create sysfs entries associated with @mci structure.
>   *

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1704228 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

On Fri, 2017-08-04 at 10:30 +0200, Borislav Petkov wrote:
> On Thu, Aug 03, 2017 at 03:57:52PM -0600, Toshi Kani wrote:
> > Only a single edac driver can be enabled for EDAC MC.  When
> > ghes_edac is enabled, a regular edac driver for the CPU type /
> > platform still attempts to register itself and fails in
> > edac_mc_add_mc().
> > 
> > Add edac_check_mc_owner() so that regular edac drivers can check
> > the owner of EDAC MC at the beginning of initialization.
> > 
> > Also change the owner check to string comparison from address
> > check.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > ---
> >  drivers/edac/edac_mc.c |   15 ++++++++++++++-
> >  drivers/edac/edac_mc.h |   12 ++++++++++++
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> > index 4800721..52d8d5e 100644
> > --- a/drivers/edac/edac_mc.c
> > +++ b/drivers/edac/edac_mc.c
> > @@ -701,6 +701,19 @@ struct mem_ctl_info *edac_mc_find(int idx)
> >  }
> >  EXPORT_SYMBOL(edac_mc_find);
> >  
> > +/*
> > + * Returns:
> > + *	1 when EDAC MC is free or owned by the module name
> 
> 1 for free OR owned?!? WTF?

1 means the caller's init function can continue its initialization --
such conditions are free or owned by itself.

> > + *	0 when EDAC MC is owned by other module
> > + */
> > +int edac_check_mc_owner(const char *mod_name)
> > +{
> > +	if (edac_mc_owner && strcmp(edac_mc_owner, mod_name))
> 
> strncmp() of course, with sensible maximal string length.

strcmp() breaks when either edac_mc_owner or mod_name string ends. 
strncmp() assumes mod_name string is valid for a given length.  Hence,
the caller needs to supply the length to this function by adding a new
arg 'length', which does not make it any safer.  I think using
strncmp() would require all edac drivers to declare a pre-defined
length of module name...

> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +EXPORT_SYMBOL(edac_check_mc_owner);
> 
> EXPORT_SYMBOL_GPL

Will do.

> >  
> >  /* FIXME - should a warning be printed if no error detection?
> > correction? */
> >  int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
> > @@ -742,7 +755,7 @@ int edac_mc_add_mc_with_groups(struct
> > mem_ctl_info *mci,
> >  #endif
> >  	mutex_lock(&mem_ctls_mutex);
> >  
> > -	if (edac_mc_owner && edac_mc_owner != mci->mod_name) {
> > +	if (!edac_check_mc_owner(mci->mod_name)) {
> >  		ret = -EPERM;
> >  		goto fail0;
> >  	}
> > diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
> > index 5357800..0e95eba 100644
> > --- a/drivers/edac/edac_mc.h
> > +++ b/drivers/edac/edac_mc.h
> > @@ -128,6 +128,18 @@ struct mem_ctl_info *edac_mc_alloc(unsigned
> > mc_num,
> >  				   unsigned sz_pvt);
> >  
> >  /**
> > + *
> > + * edac_check_mc_owner - Check the owner of EDAC MC
> > + *
> > + * @mod_name: pointer to the module name
> > + *
> > + * Returns:
> > + *	1 when EDAC MC is free or owned by the module name
> > + *	0 when EDAC MC is owned by other module
> > + */
> 
> Documenting that function only once is enough.

I personally prefer to document in .c, but since other funcs documented
in dac_mc.h, I will keep the same style.  Will remove the document in
edac_mc.c. 

> > +extern int edac_check_mc_owner(const char *mod_name);
> > +
> > +/*
> >   * edac_mc_add_mc_with_groups() - Insert the @mci structure into
> > the mci
> >   *	global list and create sysfs entries associated with
> > @mci structure.
> >   *

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1704573 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

On Fri, Aug 04, 2017 at 09:35:05PM +0000, Kani, Toshimitsu wrote:
> 1 means the caller's init function can continue its initialization --
> such conditions are free or owned by itself.

Make that:

	edac_get_owner(void)

to return the owner string or NULL if there's no owner.

The caller can then do the string checking and do strncmp() to limit the
string length being checked.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1705608 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

On Sat, 2017-08-05 at 07:44 +0200, Borislav Petkov wrote:
> On Fri, Aug 04, 2017 at 09:35:05PM +0000, Kani, Toshimitsu wrote:
> > 1 means the caller's init function can continue its initialization
> > --
> > such conditions are free or owned by itself.
> 
> Make that:
> 
> 	edac_get_owner(void)
> 
> to return the owner string or NULL if there's no owner.
> 
> The caller can then do the string checking and do strncmp() to limit
> the string length being checked.

Good idea!  Will do.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1703955 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

[Multipart message — attachments visible in raw view] — view raw

Hi Toshi,

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.13-rc3]
[cannot apply to edac/linux_next bp/for-next next-20170804]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Toshi-Kani/enable-ghes_edac-on-selected-platforms/20170804-190846
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   include/linux/init.h:1: warning: no structured comments found
   include/linux/mod_devicetable.h:687: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id'
   include/linux/mod_devicetable.h:687: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id'
   include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create'
   kernel/sys.c:1: warning: no structured comments found
   include/linux/device.h:968: warning: No description found for parameter 'dma_ops'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
>> drivers/edac/edac_mc.h:131: warning: Cannot understand  *
    on line 131 - I thought it was a doc line
   include/linux/iio/iio.h:603: warning: No description found for parameter 'trig_readonly'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig'
   include/linux/device.h:969: warning: No description found for parameter 'dma_ops'
   drivers/ata/libata-eh.c:1449: warning: No description found for parameter 'link'
   drivers/ata/libata-eh.c:1449: warning: Excess function parameter 'ap' description in 'ata_eh_done'
   drivers/ata/libata-eh.c:1590: warning: No description found for parameter 'qc'
   drivers/ata/libata-eh.c:1590: warning: Excess function parameter 'dev' description in 'ata_eh_request_sense'
   drivers/mtd/nand/nand_base.c:2751: warning: Excess function parameter 'cached' description in 'nand_write_page'
   drivers/mtd/nand/nand_base.c:2751: warning: Excess function parameter 'cached' description in 'nand_write_page'
   arch/s390/include/asm/cmb.h:1: warning: no structured comments found
   drivers/scsi/scsi_lib.c:1116: warning: No description found for parameter 'rq'
   drivers/scsi/constants.c:1: warning: no structured comments found
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_zlp_not_supp'
   fs/inode.c:1666: warning: No description found for parameter 'rcu'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_transaction'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_next_transaction'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_list'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_vfs_inode'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_flags'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_rsv_handle'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_reserved'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_type'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_line_no'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_start_jiffies'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_requested_credits'
   include/linux/jbd2.h:497: warning: No description found for parameter 'saved_alloc_context'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_chkpt_bhs'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_devname'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_average_commit_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_min_batch_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_max_batch_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_commit_callback'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_failed_commit'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_chksum_driver'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_csum_seed'
   fs/jbd2/transaction.c:511: warning: No description found for parameter 'type'
   fs/jbd2/transaction.c:511: warning: No description found for parameter 'line_no'
   fs/jbd2/transaction.c:641: warning: No description found for parameter 'gfp_mask'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'set_busid'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'debugfs_init'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_open_object'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_close_object'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'prime_handle_to_fd'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'prime_fd_to_handle'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_export'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_import'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_pin'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_unpin'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_res_obj'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_get_sg_table'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_import_sg_table'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_vmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_vunmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_mmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_vm_ops'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'major'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'minor'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'patchlevel'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'fops'
   include/drm/drm_color_mgmt.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_syncobj.c:341: warning: Excess function parameter 'dev' description in 'drm_syncobj_open'
   drivers/gpu/drm/drm_syncobj.c:366: warning: Excess function parameter 'dev' description in 'drm_syncobj_release'
   include/drm/drm_syncobj.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_syncobj.c:342: warning: Excess function parameter 'dev' description in 'drm_syncobj_open'
   drivers/gpu/drm/drm_syncobj.c:367: warning: Excess function parameter 'dev' description in 'drm_syncobj_release'
   drivers/gpu/host1x/bus.c:50: warning: Excess function parameter 'driver' description in 'host1x_subdev_add'
   Documentation/doc-guide/sphinx.rst:121: ERROR: Unknown target name: "sphinx c domain".
   kernel/sched/fair.c:7584: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1200: ERROR: Unexpected indentation.
   kernel/time/timer.c:1202: ERROR: Unexpected indentation.
   kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:108: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:111: ERROR: Unexpected indentation.
   include/linux/wait.h:113: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:991: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:323: WARNING: Inline literal start-string without end-string.
   kernel/rcu/tree.c:3187: ERROR: Unexpected indentation.
   kernel/rcu/tree.c:3214: ERROR: Unexpected indentation.
   kernel/rcu/tree.c:3215: WARNING: Bullet list ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:219: ERROR: Unexpected indentation.
   include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/iio/industrialio-core.c:633: ERROR: Unknown target name: "iio_val".
   drivers/iio/industrialio-core.c:640: ERROR: Unknown target name: "iio_val".
   drivers/ata/libata-core.c:5906: ERROR: Unknown target name: "hw".
   drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1897: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/pci/pci.c:3470: ERROR: Unexpected indentation.

vim +131 drivers/edac/edac_mc.h

    97	
    98	/**
    99	 * edac_mc_alloc() - Allocate and partially fill a struct &mem_ctl_info.
   100	 *
   101	 * @mc_num:		Memory controller number
   102	 * @n_layers:		Number of MC hierarchy layers
   103	 * @layers:		Describes each layer as seen by the Memory Controller
   104	 * @sz_pvt:		size of private storage needed
   105	 *
   106	 *
   107	 * Everything is kmalloc'ed as one big chunk - more efficient.
   108	 * Only can be used if all structures have the same lifetime - otherwise
   109	 * you have to allocate and initialize your own structures.
   110	 *
   111	 * Use edac_mc_free() to free mc structures allocated by this function.
   112	 *
   113	 * .. note::
   114	 *
   115	 *   drivers handle multi-rank memories in different ways: in some
   116	 *   drivers, one multi-rank memory stick is mapped as one entry, while, in
   117	 *   others, a single multi-rank memory stick would be mapped into several
   118	 *   entries. Currently, this function will allocate multiple struct dimm_info
   119	 *   on such scenarios, as grouping the multiple ranks require drivers change.
   120	 *
   121	 * Returns:
   122	 *	On success, return a pointer to struct mem_ctl_info pointer;
   123	 *	%NULL otherwise
   124	 */
   125	struct mem_ctl_info *edac_mc_alloc(unsigned mc_num,
   126					   unsigned n_layers,
   127					   struct edac_mc_layer *layers,
   128					   unsigned sz_pvt);
   129	
   130	/**
 > 131	 *
   132	 * edac_check_mc_owner - Check the owner of EDAC MC
   133	 *
   134	 * @mod_name: pointer to the module name
   135	 *
   136	 * Returns:
   137	 *	1 when EDAC MC is free or owned by the module name
   138	 *	0 when EDAC MC is owned by other module
   139	 */
   140	extern int edac_check_mc_owner(const char *mod_name);
   141	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1704080 — Re: [PATCH v2 6/7] EDAC: add edac_check_mc_owner() to check MC owner

On Fri, 2017-08-04 at 21:06 +0800, kbuild test robot wrote:
> Hi Toshi,
> 
> [auto build test WARNING on pm/linux-next]
> [also build test WARNING on v4.13-rc3]
> [cannot apply to edac/linux_next bp/for-next next-20170804]
> [if your patch is applied to the wrong git tree, please drop us a
> note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Toshi-Kani/enable-gh
> es_edac-on-selected-platforms/20170804-190846
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-
> pm.git linux-next
> reproduce: make htmldocs
> 
> All warnings (new ones prefixed by >>):
 :
> 	   unsigned sz_pvt);
>    129	
>    130	/**
>  > 131	 *

Removed this extra commented line.

Thanks!
-Toshi


>    132	 * edac_check_mc_owner - Check the owner of EDAC MC
>    133	 *
>    134	 * @mod_name: pointer to the module name
>    135	 *
>    136	 * Returns:
>    137	 *	1 when EDAC MC is free or owned by the module
> name
>    138	 *	0 when EDAC MC is owned by other module
>    139	 */
>    140	extern int edac_check_mc_owner(const char *mod_name);
>    141	

[toc] | [prev] | [next] | [standalone]

#1703465 — [PATCH v2 5/7] ghes_edac: add platform check to enable ghes_edac

The ghes_edac driver was introduced in 2013 [1], but it has not
been enabled by any distro yet.  This driver obtains error info
from firmware interfaces, which are not properly implemented on
many platforms, as the driver always emits the messages below:

 This EDAC driver relies on BIOS to enumerate memory and get error reports.
 Unfortunately, not all BIOSes reflect the memory layout correctly
 So, the end result of using this driver varies from vendor to vendor
 If you find incorrect reports, please contact your hardware vendor
 to correct its BIOS.

To get out from this situation, add a platform check to selectively
enable the driver on the platforms that are known to have proper
firmware implementation.  Platform vendors can add their platforms
to the list when they support ghes_edac.

"ghes_edac.any_oem=1" skips this platform check.

[1]: https://lwn.net/Articles/538438/
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
---
 drivers/edac/ghes_edac.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 2e9ce9c..72bab02 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -34,6 +34,9 @@ static LIST_HEAD(ghes_reglist);
 static DEFINE_MUTEX(ghes_edac_lock);
 static int ghes_edac_mc_num;
 
+/* Set 1 to skip the platform check */
+static bool __read_mostly ghes_edac_any_oem;
+module_param_named(any_oem, ghes_edac_any_oem, bool, 0);
 
 /* Memory Device - Type 17 of SMBIOS spec */
 struct memdev_dmi_entry {
@@ -405,17 +408,31 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
 }
 EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error);
 
+/*
+ * Known systems that are safe to enable this module.
+ * "ghes_edac.any_oem=1" skips this check if necessary.
+ */
+static struct acpi_oemlist oemlist[] = {
+	{"HPE   ", "Server  ", 0, ACPI_SIG_FADT, all_versions},
+	{ } /* End */
+};
+
 int ghes_edac_register(struct ghes *ghes, struct device *dev)
 {
 	struct mem_ctl_info *mci;
 	struct edac_mc_layer layers[1];
 	struct ghes_edac_pvt *pvt;
 	struct ghes_edac_dimm_fill dimm_fill;
-	int rc;
+	int rc, idx;
 
 	static int num_dimm;
 	static bool fake;
 
+	/* Check if safe to enable on this system */
+	idx = acpi_match_oemlist(oemlist);
+	if (!ghes_edac_any_oem && idx < 0)
+		return 0;
+
 	/* Get the number of DIMMs */
 	if (num_dimm == 0)
 		dmi_walk(ghes_edac_count_dimms, &num_dimm);
@@ -459,7 +476,11 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 	mci->dev_name = "ghes";
 
 	if (!ghes_edac_mc_num) {
-		if (!fake) {
+		if (fake) {
+			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
+			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
+			pr_info("work on such system. Use this driver with caution\n");
+		} else if (idx < 0) {
 			pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
 			pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
 			pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
@@ -467,10 +488,6 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
 			pr_info("to correct its BIOS.\n");
 			pr_info("This system has %d DIMM sockets.\n",
 				num_dimm);
-		} else {
-			pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
-			pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
-			pr_info("work on such system. Use this driver with caution\n");
 		}
 	}
 

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web