diff mbox series

[v2] drivers/base/memory: Use array to show memory block state

Message ID 20230120233814.368803-1-gshan@redhat.com
State New
Headers
Series [v2] drivers/base/memory: Use array to show memory block state |

Commit Message

Gavin Shan Jan. 20, 2023, 11:38 p.m. UTC 
  Use an array to show memory block state from '/sys/devices/system/
memory/memoryX/state', to simplify the code. Besides, WARN_ON()
is removed since the warning can be caught by the return value,
which is "ERROR-UNKNOWN-%ld\n". A system reboot caused by WARN_ON()
is definitely unexpected as Greg mentioned.

No functional change intended.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
v2: Drop WARN_ON()					(Greg)
---
 drivers/base/memory.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

Comments

David Hildenbrand Jan. 23, 2023, 10:33 a.m. UTC | #1 
On 21.01.23 00:38, Gavin Shan wrote:
> Use an array to show memory block state from '/sys/devices/system/
> memory/memoryX/state', to simplify the code. Besides, WARN_ON()
> is removed since the warning can be caught by the return value,
> which is "ERROR-UNKNOWN-%ld\n". A system reboot caused by WARN_ON()
> is definitely unexpected as Greg mentioned.
> 
> No functional change intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>
Greg KH Jan. 31, 2023, 2:54 p.m. UTC | #2 
On Sat, Jan 21, 2023 at 07:38:14AM +0800, Gavin Shan wrote:
> Use an array to show memory block state from '/sys/devices/system/
> memory/memoryX/state', to simplify the code.

But does it really?

Now you have an implicit binding between the order of this specific
string array and an enumerated type that is defined in some other
location.

This makes any future changes really really hard to determine that you
got this correct.

Besides, WARN_ON()
> is removed since the warning can be caught by the return value,
> which is "ERROR-UNKNOWN-%ld\n". A system reboot caused by WARN_ON()
> is definitely unexpected as Greg mentioned.
> 
> No functional change intended.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> v2: Drop WARN_ON()					(Greg)
> ---
>  drivers/base/memory.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index b456ac213610..0fdacdc79806 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -141,28 +141,15 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>  			  char *buf)
>  {
>  	struct memory_block *mem = to_memory_block(dev);
> -	const char *output;
> +	static const char *const mem_state_str[] = {
> +		NULL, "online", "going-offline", NULL, "offline",
> +	};
>  
> -	/*
> -	 * We can probably put these states in a nice little array
> -	 * so that they're not open-coded
> -	 */
> -	switch (mem->state) {
> -	case MEM_ONLINE:
> -		output = "online";
> -		break;
> -	case MEM_OFFLINE:
> -		output = "offline";
> -		break;
> -	case MEM_GOING_OFFLINE:
> -		output = "going-offline";
> -		break;
> -	default:
> -		WARN_ON(1);
> +	if (mem->state >= ARRAY_SIZE(mem_state_str) ||
> +	    !mem_state_str[mem->state])
>  		return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state);
> -	}
>  
> -	return sysfs_emit(buf, "%s\n", output);
> +	return sysfs_emit(buf, "%s\n", mem_state_str[mem->state]);

Overall, the current code is simpler and easier to maintain and
understand over time.  You don't have to mess with an array length, or
anything else like that.

I'm all for removing the WARN_ON() if you want to do that, but I think
this is a regression in the ability to maintain this code for the next
40+ years, sorry.

greg k-h
diff mbox series

Patch

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index b456ac213610..0fdacdc79806 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -141,28 +141,15 @@  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
 	struct memory_block *mem = to_memory_block(dev);
-	const char *output;
+	static const char *const mem_state_str[] = {
+		NULL, "online", "going-offline", NULL, "offline",
+	};
 
-	/*
-	 * We can probably put these states in a nice little array
-	 * so that they're not open-coded
-	 */
-	switch (mem->state) {
-	case MEM_ONLINE:
-		output = "online";
-		break;
-	case MEM_OFFLINE:
-		output = "offline";
-		break;
-	case MEM_GOING_OFFLINE:
-		output = "going-offline";
-		break;
-	default:
-		WARN_ON(1);
+	if (mem->state >= ARRAY_SIZE(mem_state_str) ||
+	    !mem_state_str[mem->state])
 		return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state);
-	}
 
-	return sysfs_emit(buf, "%s\n", output);
+	return sysfs_emit(buf, "%s\n", mem_state_str[mem->state]);
 }
 
 int memory_notify(unsigned long val, void *v)