Re: [PATCH 2/2] drivers/base/memory: Use array to show memory block state

[Gavin Shan <gshan@redhat.com>]

On 1/21/23 12:14 AM, Greg KH wrote:
> On Fri, Jan 20, 2023 at 01:57:27PM +0800, Gavin Shan wrote:
>> Use an array to show memory block state from '/sys/devices/system/
>> memory/memoryX/state', to simplify the code.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   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..9474f25c452c 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 (WARN_ON(mem->state >= ARRAY_SIZE(mem_state_str) ||
>> +		    !mem_state_str[mem->state]))
> 
> Ick, the whole WARN_ON() should just be removed please.  We don't want
> to reboot any systems if this changed incorrectly.
> 
> Please fix this up to properly handle this and keep going on, don't mess
> with WARN_ON() anymore in code that can recover properly.
> 

Thanks for your review, Greg. Indeed, the WARN_ON() here is no sense because
the warning can be caught from the return value. "ERROR-UNKNOWN-%ld\n" is
returned for unknown or invalid state.

I will drop WARN_ON() in v2. PATCH[1/2] won't be reposted since it has been
merged to driver.core git tree.

Thanks,
Gavin