edac: Add an API for edac device to report for multiple errors

PATCH Superseded Hanna Hawa 3 participants 7 messages Sep 5, 2019 – Sep 10, 2019 EDAC-core

lore

Share & tools

lore

Hanna Hawa

author

2019-09-05 08:38 UTC #0 lore raw

PATCH 1/1 edac: Add an API for edac device to report for multiple errors

Add an API for edac device to report multiple errors with same type.

Signed-off-by Hanna Hawa <[email protected]>

---
 drivers/edac/edac_device.c | 66 +++++++++++++++++++++++++++++---------
 drivers/edac/edac_device.h | 31 ++++++++++++++++--
 2 files changed, 79 insertions(+), 18 deletions(-)

drivers/edac/edac_device.c +50 ~~-16~~

index 65cf2b9355c4..bf6a4fd9831b 100644
@@ -555,12 +555,15 @@ static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info
 	return edac_dev->panic_on_ue;
 }
 
-void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+static void __edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
+			   const char *msg)
 {
 	struct edac_device_instance *instance;
 	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);
+
 	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
 		edac_device_printk(edac_dev, KERN_ERR,
 				"INTERNAL ERROR: 'instance' out of range "

@@ -582,27 +585,44 @@ void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 
 	if (instance->nr_blocks > 0) {
 		block = instance->blocks + block_nr;
-		block->counters.ce_count++;
+		block->counters.ce_count += error_count;
 	}
 
 	/* Propagate the count up the 'totals' tree */
-	instance->counters.ce_count++;
-	edac_dev->counters.ce_count++;
+	instance->counters.ce_count += error_count;
+	edac_dev->counters.ce_count += error_count;
 
 	if (edac_device_get_log_ce(edac_dev))
 		edac_device_printk(edac_dev, KERN_WARNING,
-				"CE: %s instance: %s block: %s '%s'\n",
+				"CE: %s instance: %s block: %s count: %d '%s'\n",
 				edac_dev->ctl_name, instance->name,
-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, 1, inst_nr, block_nr, msg);
 }
 EXPORT_SYMBOL_GPL(edac_device_handle_ce);
 
-void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,
+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ce_count);
+
+static void __edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
+			   const char *msg)
 {
 	struct edac_device_instance *instance;
 	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);
+
 	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
 		edac_device_printk(edac_dev, KERN_ERR,
 				"INTERNAL ERROR: 'instance' out of range "

@@ -624,22 +644,36 @@ void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 
 	if (instance->nr_blocks > 0) {
 		block = instance->blocks + block_nr;
-		block->counters.ue_count++;
+		block->counters.ue_count += error_count;
 	}
 
 	/* Propagate the count up the 'totals' tree */
-	instance->counters.ue_count++;
-	edac_dev->counters.ue_count++;
+	instance->counters.ue_count += error_count;
+	edac_dev->counters.ue_count += error_count;
 
 	if (edac_device_get_log_ue(edac_dev))
 		edac_device_printk(edac_dev, KERN_EMERG,
-				"UE: %s instance: %s block: %s '%s'\n",
+				"UE: %s instance: %s block: %s count: %d '%s'\n",
 				edac_dev->ctl_name, instance->name,
-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
 
 	if (edac_device_get_panic_on_ue(edac_dev))
-		panic("EDAC %s: UE instance: %s block %s '%s'\n",
+		panic("EDAC %s: UE instance: %s block %s count: %d '%s'\n",
 			edac_dev->ctl_name, instance->name,
-			block ? block->name : "N/A", msg);
+			block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, 1, inst_nr, block_nr, msg);
 }
 EXPORT_SYMBOL_GPL(edac_device_handle_ue);
+
+void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ue_count);

drivers/edac/edac_device.h +29 -2

index 1aaba74ae411..c8dc83eda64f 100644
@@ -287,7 +287,7 @@ extern struct edac_device_ctl_info *edac_device_del_device(struct device *dev);
 
 /**
  * edac_device_handle_ue():
- *	perform a common output and handling of an 'edac_dev' UE event
+ *	perform a common output and handling of an 'edac_dev' single UE event
  *
  * @edac_dev: pointer to struct &edac_device_ctl_info
  * @inst_nr: number of the instance where the UE error happened

@@ -298,7 +298,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 				int inst_nr, int block_nr, const char *msg);
 /**
  * edac_device_handle_ce():
- *	perform a common output and handling of an 'edac_dev' CE event
+ *	perform a common output and handling of an 'edac_dev' single CE event
  *
  * @edac_dev: pointer to struct &edac_device_ctl_info
  * @inst_nr: number of the instance where the CE error happened

@@ -308,6 +308,33 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 				int inst_nr, int block_nr, const char *msg);
 
+/**
+ * edac_device_handle_ue_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *
+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the UE error happened
+ * @block_nr: number of the block where the UE error happened
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+/**
+ * edac_device_handle_ce_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *
+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the CE error happened
+ * @block_nr: number of the block where the CE error happened
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+
 /**
  * edac_device_alloc_index: Allocate a unique device index number
  *

Signature · 1 line

2.17.1

Robert Richter

2019-09-05 09:57 UTC #1 lore raw

Hi Hanna,

thanks for the update. See below.

Quoted 13 lines · Hanna Hawa

Add an API for edac device to report multiple errors with same type.
Signed-off-by Hanna Hawa <[email protected]>
---
 drivers/edac/edac_device.c | 66 +++++++++++++++++++++++++++++---------
 drivers/edac/edac_device.h | 31 ++++++++++++++++--
 2 files changed, 79 insertions(+), 18 deletions(-)
drivers/edac/edac_device.c +0 -0
index 65cf2b9355c4..bf6a4fd9831b 100644

drivers/edac/edac_device.c static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info · line 559

Quoted 7 lines

drivers/edac/edac_device.c +2 -2

@@ -555,12 +555,15 @@ static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info
 	return edac_dev->panic_on_ue;
 }
 
-void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+static void __edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,

Just curious, why u16, some register mask size? Maybe just use unsigned int?

I think the variable can be shortened to 'count', the meaning should still be clear.

drivers/edac/edac_device.c static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info · line 565

Quoted 6 lines

drivers/edac/edac_device.c +2 -0

+			   const char *msg)
 {
 	struct edac_device_instance *instance;
 	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);

Should return in this case.

Better use WARN_ON_ONCE() to avoid flooding.

The check should be moved to edac_device_handle_ce_count().

Quoted 23 lines

drivers/edac/edac_device.c +5 -4

+
 	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
 		edac_device_printk(edac_dev, KERN_ERR,
 				"INTERNAL ERROR: 'instance' out of range "

@@ -582,27 +585,44 @@ void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 
 	if (instance->nr_blocks > 0) {
 		block = instance->blocks + block_nr;
-		block->counters.ce_count++;
+		block->counters.ce_count += error_count;
 	}
 
 	/* Propagate the count up the 'totals' tree */
-	instance->counters.ce_count++;
-	edac_dev->counters.ce_count++;
+	instance->counters.ce_count += error_count;
+	edac_dev->counters.ce_count += error_count;
 
 	if (edac_device_get_log_ce(edac_dev))
 		edac_device_printk(edac_dev, KERN_WARNING,
-				"CE: %s instance: %s block: %s '%s'\n",
+				"CE: %s instance: %s block: %s count: %d '%s'\n",
 				edac_dev->ctl_name, instance->name,

drivers/edac/edac_device.c void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev, · line 607

Quoted 10 lines

drivers/edac/edac_device.c +7 -1

-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, 1, inst_nr, block_nr, msg);
 }
 EXPORT_SYMBOL_GPL(edac_device_handle_ce);

We could just export the __*() version of those functions and make everything else inline in the header file? Though, better do this with two patches to avoid an ABI breakage in case someone wants to backport it. Let's see what others say here.

Quoted 4 lines

drivers/edac/edac_device.c +1 -2

-void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,

drivers/edac/edac_device.c void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev, · line 619

Quoted 10 lines

drivers/edac/edac_device.c +10 -0

+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ce_count);
+
+static void __edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
+			   const char *msg)

All the above applies for this function too.

Quoted 96 lines

drivers/edac/edac_device.c +23 -7

 {
 	struct edac_device_instance *instance;
 	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);
+
 	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
 		edac_device_printk(edac_dev, KERN_ERR,
 				"INTERNAL ERROR: 'instance' out of range "

@@ -624,22 +644,36 @@ void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 
 	if (instance->nr_blocks > 0) {
 		block = instance->blocks + block_nr;
-		block->counters.ue_count++;
+		block->counters.ue_count += error_count;
 	}
 
 	/* Propagate the count up the 'totals' tree */
-	instance->counters.ue_count++;
-	edac_dev->counters.ue_count++;
+	instance->counters.ue_count += error_count;
+	edac_dev->counters.ue_count += error_count;
 
 	if (edac_device_get_log_ue(edac_dev))
 		edac_device_printk(edac_dev, KERN_EMERG,
-				"UE: %s instance: %s block: %s '%s'\n",
+				"UE: %s instance: %s block: %s count: %d '%s'\n",
 				edac_dev->ctl_name, instance->name,
-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
 
 	if (edac_device_get_panic_on_ue(edac_dev))
-		panic("EDAC %s: UE instance: %s block %s '%s'\n",
+		panic("EDAC %s: UE instance: %s block %s count: %d '%s'\n",
 			edac_dev->ctl_name, instance->name,
-			block ? block->name : "N/A", msg);
+			block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, 1, inst_nr, block_nr, msg);
 }
 EXPORT_SYMBOL_GPL(edac_device_handle_ue);
+
+void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ue_count);

drivers/edac/edac_device.h +19 -2

index 1aaba74ae411..c8dc83eda64f 100644
@@ -287,7 +287,7 @@ extern struct edac_device_ctl_info *edac_device_del_device(struct device *dev);
 
 /**
  * edac_device_handle_ue():
- *	perform a common output and handling of an 'edac_dev' UE event
+ *	perform a common output and handling of an 'edac_dev' single UE event
  *
  * @edac_dev: pointer to struct &edac_device_ctl_info
  * @inst_nr: number of the instance where the UE error happened

@@ -298,7 +298,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 				int inst_nr, int block_nr, const char *msg);
 /**
  * edac_device_handle_ce():
- *	perform a common output and handling of an 'edac_dev' CE event
+ *	perform a common output and handling of an 'edac_dev' single CE event
  *
  * @edac_dev: pointer to struct &edac_device_ctl_info
  * @inst_nr: number of the instance where the CE error happened

@@ -308,6 +308,33 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 				int inst_nr, int block_nr, const char *msg);
 
+/**
+ * edac_device_handle_ue_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *
+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the UE error happened
+ * @block_nr: number of the block where the UE error happened
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+/**
+ * edac_device_handle_ce_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *

drivers/edac/edac_device.h extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev, · line 337

Quoted 10 lines

drivers/edac/edac_device.h +10 -0

+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the CE error happened
+ * @block_nr: number of the block where the CE error happened
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+

Looks otherwise good to me.

Thanks,

-Robert

Quoted 6 lines

drivers/edac/edac_device.h +0 -0
 /**
  * edac_device_alloc_index: Allocate a unique device index number
  *
Signature · 1 line
2.17.1

Hawa, Hanna

author

2019-09-08 07:59 UTC #2 lore raw

Quoted 21 lines · Robert Richter

Hi Hanna,
thanks for the update. See below.
Quoted 16 lines · Hanna Hawa
Add an API for edac device to report multiple errors with same type.
Signed-off-by Hanna Hawa <[email protected]>
---
  drivers/edac/edac_device.c | 66 +++++++++++++++++++++++++++++---------
  drivers/edac/edac_device.h | 31 ++++++++++++++++--
  2 files changed, 79 insertions(+), 18 deletions(-)
drivers/edac/edac_device.c +0 -0
index 65cf2b9355c4..bf6a4fd9831b 100644
@@ -555,12 +555,15 @@ static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info
  	return edac_dev->panic_on_ue;
  }
 

Quoted 6 lines

drivers/edac/edac_device.c static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info · line 559
Quoted 4 lines
drivers/edac/edac_device.c +2 -2
-void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+static void __edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
Just curious, why u16, some register mask size? Maybe just use unsigned int?

Wanted to be aligned with edac MC.
I can change it to be u32.

Quoted 3 lines

I think the variable can be shortened to 'count', the meaning should still be clear.

I think more clear to include 'error'.
maybe shorter name 'err_count'?

Quoted 11 lines

drivers/edac/edac_device.c static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info · line 565
Quoted 7 lines
drivers/edac/edac_device.c +2 -0
+			   const char *msg)
  {
  	struct edac_device_instance *instance;
  	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);
Should return in this case.
Better use WARN_ON_ONCE() to avoid flooding.

In case of two drivers using this function with wrong error count, only the first WARN_ON_ONCE will catch in this case, and other will miss other wrong usage of other edac device drivers.

Quoted 2 lines

The check should be moved to edac_device_handle_ce_count().

I'll move it to edac_device_handle_ce_count.

Quoted 31 lines

drivers/edac/edac_device.c +11 -5

+
  	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
  		edac_device_printk(edac_dev, KERN_ERR,
  				"INTERNAL ERROR: 'instance' out of range "

@@ -582,27 +585,44 @@ void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
 
  	if (instance->nr_blocks > 0) {
  		block = instance->blocks + block_nr;
-		block->counters.ce_count++;
+		block->counters.ce_count += error_count;
  	}
 
  	/* Propagate the count up the 'totals' tree */
-	instance->counters.ce_count++;
-	edac_dev->counters.ce_count++;
+	instance->counters.ce_count += error_count;
+	edac_dev->counters.ce_count += error_count;
 
  	if (edac_device_get_log_ce(edac_dev))
  		edac_device_printk(edac_dev, KERN_WARNING,
-				"CE: %s instance: %s block: %s '%s'\n",
+				"CE: %s instance: %s block: %s count: %d '%s'\n",
  				edac_dev->ctl_name, instance->name,
-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{

Quoted 8 lines

drivers/edac/edac_device.c void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev, · line 607
Quoted 3 lines
drivers/edac/edac_device.c +1 -0
+	__edac_device_handle_ce(edac_dev, 1, inst_nr, block_nr, msg);
  }
  EXPORT_SYMBOL_GPL(edac_device_handle_ce);
We could just export the __*() version of those functions and make everything else inline in the header file? Though, better do this with two patches to avoid an ABI breakage in case someone wants to backport it. Let's see what others say here.

Waiting for other reviewers.

Quoted 118 lines

Quoted 5 lines

drivers/edac/edac_device.c +1 -2

-void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,

drivers/edac/edac_device.c void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev, · line 619

Quoted 10 lines

drivers/edac/edac_device.c +10 -0

+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ce_count);
+
+static void __edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
+			   const char *msg)

All the above applies for this function too.

Quoted 100 lines

drivers/edac/edac_device.c +23 -7

  {
  	struct edac_device_instance *instance;
  	struct edac_device_block *block = NULL;
 
+	WARN_ON(!error_count);
+
  	if ((inst_nr >= edac_dev->nr_instances) || (inst_nr < 0)) {
  		edac_device_printk(edac_dev, KERN_ERR,
  				"INTERNAL ERROR: 'instance' out of range "

@@ -624,22 +644,36 @@ void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
 
  	if (instance->nr_blocks > 0) {
  		block = instance->blocks + block_nr;
-		block->counters.ue_count++;
+		block->counters.ue_count += error_count;
  	}
 
  	/* Propagate the count up the 'totals' tree */
-	instance->counters.ue_count++;
-	edac_dev->counters.ue_count++;
+	instance->counters.ue_count += error_count;
+	edac_dev->counters.ue_count += error_count;
 
  	if (edac_device_get_log_ue(edac_dev))
  		edac_device_printk(edac_dev, KERN_EMERG,
-				"UE: %s instance: %s block: %s '%s'\n",
+				"UE: %s instance: %s block: %s count: %d '%s'\n",
  				edac_dev->ctl_name, instance->name,
-				block ? block->name : "N/A", msg);
+				block ? block->name : "N/A", error_count, msg);
 
  	if (edac_device_get_panic_on_ue(edac_dev))
-		panic("EDAC %s: UE instance: %s block %s '%s'\n",
+		panic("EDAC %s: UE instance: %s block %s count: %d '%s'\n",
  			edac_dev->ctl_name, instance->name,
-			block ? block->name : "N/A", msg);
+			block ? block->name : "N/A", error_count, msg);
+}
+
+void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, 1, inst_nr, block_nr, msg);
  }
  EXPORT_SYMBOL_GPL(edac_device_handle_ue);
+
+void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+				 u16 error_count, int inst_nr, int block_nr,
+				 const char *msg)
+{
+	__edac_device_handle_ue(edac_dev, error_count, inst_nr, block_nr, msg);
+}
+EXPORT_SYMBOL_GPL(edac_device_handle_ue_count);

drivers/edac/edac_device.h +23 -2

index 1aaba74ae411..c8dc83eda64f 100644
@@ -287,7 +287,7 @@ extern struct edac_device_ctl_info *edac_device_del_device(struct device *dev);
 
  /**
   * edac_device_handle_ue():
- *	perform a common output and handling of an 'edac_dev' UE event
+ *	perform a common output and handling of an 'edac_dev' single UE event
   *
   * @edac_dev: pointer to struct &edac_device_ctl_info
   * @inst_nr: number of the instance where the UE error happened

@@ -298,7 +298,7 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
  				int inst_nr, int block_nr, const char *msg);
  /**
   * edac_device_handle_ce():
- *	perform a common output and handling of an 'edac_dev' CE event
+ *	perform a common output and handling of an 'edac_dev' single CE event
   *
   * @edac_dev: pointer to struct &edac_device_ctl_info
   * @inst_nr: number of the instance where the CE error happened

@@ -308,6 +308,33 @@ extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev,
  extern void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
  				int inst_nr, int block_nr, const char *msg);
 
+/**
+ * edac_device_handle_ue_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *
+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the UE error happened
+ * @block_nr: number of the block where the UE error happened
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ue_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+/**
+ * edac_device_handle_ce_count():
+ *	perform a common output and handling of an 'edac_dev'
+ *
+ * @edac_dev: pointer to struct &edac_device_ctl_info
+ * @error_count: number of errors of the same type
+ * @inst_nr: number of the instance where the CE error happened
+ * @block_nr: number of the block where the CE error happened

Quoted 8 lines

drivers/edac/edac_device.h extern void edac_device_handle_ue(struct edac_device_ctl_info *edac_dev, · line 337
Quoted 6 lines
drivers/edac/edac_device.h +6 -0
+ * @msg: message to be printed
+ */
+extern void edac_device_handle_ce_count(struct edac_device_ctl_info *edac_dev,
+					u16 error_count, int inst_nr,
+					int block_nr, const char *msg);
+
Looks otherwise good to me.

Thanks!

Thanks,
Hanna

Quoted 11 lines

Thanks,
-Robert
Quoted 6 lines
drivers/edac/edac_device.h +0 -0
  /**
   * edac_device_alloc_index: Allocate a unique device index number
   *
Signature · 1 line
2.17.1

Borislav Petkov

maintainer

2019-09-08 08:16 UTC #3 lore raw

Quoted 5 lines · Hanna

Quoted 1 line
Better use WARN_ON_ONCE() to avoid flooding.
In case of two drivers using this function with wrong error count, only the first WARN_ON_ONCE will catch in this case, and other will miss other wrong usage of other edac device drivers.

The idea is to catch any driver using a 0 error count and fix it, not to flood dmesg. You want _ONCE.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Borislav Petkov

maintainer

2019-09-08 08:35 UTC #4 lore raw

Quoted 9 lines · Borislav Petkov

Quoted 5 lines · Hanna
Quoted 1 line
Better use WARN_ON_ONCE() to avoid flooding.
In case of two drivers using this function with wrong error count, only the first WARN_ON_ONCE will catch in this case, and other will miss other wrong usage of other edac device drivers.
The idea is to catch any driver using a 0 error count and fix it, not to flood dmesg. You want _ONCE.

... and you want to return early too, i.e.,

	if (WARN_ON_ONCE(!error_count))
		return;

Frankly, I'd even remove all the warning functionality and simply do

	if (!error_count)
		return;

but let's see how much it screams first.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Hawa, Hanna

author

2019-09-10 11:10 UTC #5 lore raw

Quoted 13 lines · Borislav Petkov

Quoted 9 lines · Borislav Petkov
Quoted 5 lines · Hanna
Quoted 1 line
Better use WARN_ON_ONCE() to avoid flooding.
In case of two drivers using this function with wrong error count, only the first WARN_ON_ONCE will catch in this case, and other will miss other wrong usage of other edac device drivers.
The idea is to catch any driver using a 0 error count and fix it, not to flood dmesg. You want _ONCE.
... and you want to return early too, i.e.,

Quoted 7 lines

	if (WARN_ON_ONCE(!error_count))
		return;
Frankly, I'd even remove all the warning functionality and simply do
	if (!error_count)
		return;

I'll keep it simple as you suggest and remove the warning functionality.

Quoted 3 lines

but let's see how much it screams first.

Thanks,
Hanna

Robert Richter

2019-09-10 12:42 UTC #6 lore raw

In reply to Hawa, Hanna

Quoted 16 lines · Hanna

Quoted 5 lines · Robert Richter
Quoted 5 lines
drivers/edac/edac_device.c +0 -0
index 65cf2b9355c4..bf6a4fd9831b 100644
@@ -555,12 +555,15 @@ static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info
Quoted 8 lines
drivers/edac/edac_device.c static inline int edac_device_get_panic_on_ue(struct edac_device_ctl_info · line 559
Quoted 6 lines
drivers/edac/edac_device.c +2 -2
  	return edac_dev->panic_on_ue;
  }
-void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
-			int inst_nr, int block_nr, const char *msg)
+static void __edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   u16 error_count, int inst_nr, int block_nr,
Just curious, why u16, some register mask size? Maybe just use unsigned int?

Quoted 2 lines

Wanted to be aligned with edac MC.
I can change it to be u32.

There is no specific reason for u32 either. This code is generic for many machines and compilers, so unsigned int seems to be the best choice here to get an optimum.

Quoted 5 lines

Quoted 2 lines
I think the variable can be shortened to 'count', the meaning should still be clear.
I think more clear to include 'error'.
maybe shorter name 'err_count'?

IMO 'count' is clear here, 'error' isn't. I prefer short names for local variables and arguments.

Quoted 12 lines

Quoted 11 lines
drivers/edac/edac_device.c void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev, · line 607
Quoted 6 lines
drivers/edac/edac_device.c +4 -0
+void edac_device_handle_ce(struct edac_device_ctl_info *edac_dev,
+			   int inst_nr, int block_nr, const char *msg)
+{
+	__edac_device_handle_ce(edac_dev, 1, inst_nr, block_nr, msg);
  }
  EXPORT_SYMBOL_GPL(edac_device_handle_ce);
We could just export the __*() version of those functions and make everything else inline in the header file? Though, better do this with two patches to avoid an ABI breakage in case someone wants to backport it. Let's see what others say here.

Quoted 1 line

Waiting for other reviewers.

If no one else complains I would prefer moving to __edac_device_handle_* as exported sympbol. But make this change in 2 patches to make backports easy, first add __edac_device_handle_*() (and introduce the *_count() inline functions), then remove edac_device_handle_*() entirely.

Thanks,

-Robert