Re: [PATCH v3] memory-hotplug: fix store_mem_state() return value

[Reza Arbab]

On Thu, Sep 01, 2016 at 01:37:17PM -0700, Andrew Morton wrote:
> What the heck are the return value semantics of bus_type.online?
> Sometimes 0, sometimes 1 and apparently sometimes -Efoo values.  What
> are these things trying to tell the caller and why is "1" ever useful
> and why doesn't anyone document anything.  grr.

You might be getting tangled in the two codepaths the way I was.

If you do 'echo 1 > online':
	dev_attr_store
		online_store
			device_online
				memory_subsys_online
					memory_block_change_state

If you do 'echo online > state':
	dev_attr_store
		store_mem_state
			device_online
				memory_subsys_online
					memory_block_change_state

> static int memory_subsys_online(struct device *dev)
> {
> 	struct memory_block *mem = to_memory_block(dev);
> 	int ret;
>
> 	if (mem->state == MEM_ONLINE)
> 		return 0;
>
> Doesn't that "return 0" contradict the changelog?

The online-to-online check being used is higher in the call chain:

int device_online(struct device *dev)
{
	if (device_supports_offline(dev)) {
		if (dev->offline) {
			...
		} else {
			ret = 1;
		}
	}

> Also, is store_mem_state() the correct place to fix this?  Instead,
> should memory_block_change_state() detect an attempt to online
> already-online memory and itself return -EINVAL, and permit that to be
> propagated back?

Doing that would affect both codepaths, and as David made clear, would 
break backwards compatibility because their established behaviors are 
different.

'echo 1 > online' returns 0 if the device is already online
'echo online > state' returns -EINVAL if the device is already online

--
Reza Arbab