Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Reply-To: Gavin Shan <gshan@redhat.com>
Subject: Re: [PATCH 2/2] drivers/base/memory: Use array to show memory block
 state
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org, david@redhat.com,
        osalvador@suse.de, rafael@kernel.org, shan.gavin@gmail.com
References: <20230120055727.355483-1-gshan@redhat.com>
 <20230120055727.355483-3-gshan@redhat.com> <Y8qTyEgc8ih6M+DW@kroah.com>
From:   Gavin Shan <gshan@redhat.com>
Message-ID: <bf2a6311-4610-6cdd-1810-2419ff330871@redhat.com>
Date:   Sat, 21 Jan 2023 09:59:20 +1100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <Y8qTyEgc8ih6M+DW@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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