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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf2a6311-4610-6cdd-1810-2419ff330871@redhat.com>
Date:   Sat, 21 Jan 2023 09:59:20 +1100
From:   Gavin Shan <gshan@...hat.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org, david@...hat.com,
        osalvador@...e.de, rafael@...nel.org, shan.gavin@...il.com
Subject: Re: [PATCH 2/2] drivers/base/memory: Use array to show memory block
 state

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@...hat.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