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

[Gavin Shan]

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@xxxxxxxxxx>
> > ---
> >   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