* [PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-05-05 19:07 Reza Arbab 2016-05-26 14:43 ` Reza Arbab 0 siblings, 1 reply; 24+ messages in thread From: Reza Arbab @ 2016-05-05 19:07 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-kernel Attempting to online memory which is already online will cause this: 1. store_mem_state() called with buf="online" 2. device_online() returns 1 because device is already online 3. store_mem_state() returns 1 4. calling code interprets this as 1-byte buffer read 5. store_mem_state() called again with buf="nline" 6. store_mem_state() returns -EINVAL Example: $ cat /sys/devices/system/memory/memory0/state online $ echo online > /sys/devices/system/memory/memory0/state -bash: echo: write error: Invalid argument Fix the return value of store_mem_state() so this doesn't happen. Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com> --- drivers/base/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 961e2cf..eebd9a8 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -359,7 +359,7 @@ store_mem_state(struct device *dev, err: unlock_device_hotplug(); - if (ret) + if (ret < 0) return ret; return count; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-05-05 19:07 [PATCH v2] memory-hotplug: fix store_mem_state() return value Reza Arbab @ 2016-05-26 14:43 ` Reza Arbab 2016-05-26 15:34 ` Greg Kroah-Hartman 2016-08-31 12:21 ` Greg Kroah-Hartman 0 siblings, 2 replies; 24+ messages in thread From: Reza Arbab @ 2016-05-26 14:43 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel Hi Greg, On Thu, May 05, 2016 at 02:07:24PM -0500, Reza Arbab wrote: >$ echo online > /sys/devices/system/memory/memory0/state >-bash: echo: write error: Invalid argument > >Fix the return value of store_mem_state() so this doesn't happen. I can't find where this might have been committed. Am I being impatient, or did it slip through the cracks? -- Reza Arbab ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-05-26 14:43 ` Reza Arbab @ 2016-05-26 15:34 ` Greg Kroah-Hartman 2016-05-26 15:35 ` Greg Kroah-Hartman 2016-08-31 12:21 ` Greg Kroah-Hartman 1 sibling, 1 reply; 24+ messages in thread From: Greg Kroah-Hartman @ 2016-05-26 15:34 UTC (permalink / raw) To: Reza Arbab; +Cc: linux-kernel On Thu, May 26, 2016 at 09:43:04AM -0500, Reza Arbab wrote: > Hi Greg, > > On Thu, May 05, 2016 at 02:07:24PM -0500, Reza Arbab wrote: > > $ echo online > /sys/devices/system/memory/memory0/state > > -bash: echo: write error: Invalid argument > > > > Fix the return value of store_mem_state() so this doesn't happen. > > I can't find where this might have been committed. Am I being impatient, or > did it slip through the cracks? You ignored my patch review that asked for this patch to be changed for some reason, which is why it was never applied. greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-05-26 15:34 ` Greg Kroah-Hartman @ 2016-05-26 15:35 ` Greg Kroah-Hartman 0 siblings, 0 replies; 24+ messages in thread From: Greg Kroah-Hartman @ 2016-05-26 15:35 UTC (permalink / raw) To: Reza Arbab; +Cc: linux-kernel On Thu, May 26, 2016 at 08:34:12AM -0700, Greg Kroah-Hartman wrote: > On Thu, May 26, 2016 at 09:43:04AM -0500, Reza Arbab wrote: > > Hi Greg, > > > > On Thu, May 05, 2016 at 02:07:24PM -0500, Reza Arbab wrote: > > > $ echo online > /sys/devices/system/memory/memory0/state > > > -bash: echo: write error: Invalid argument > > > > > > Fix the return value of store_mem_state() so this doesn't happen. > > > > I can't find where this might have been committed. Am I being impatient, or > > did it slip through the cracks? > > You ignored my patch review that asked for this patch to be changed for > some reason, which is why it was never applied. Ah nevermind, this was v2, yes, it's in my queue, looking in my wrong mbox. Please be patient. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-05-26 14:43 ` Reza Arbab 2016-05-26 15:34 ` Greg Kroah-Hartman @ 2016-08-31 12:21 ` Greg Kroah-Hartman 2016-08-31 14:37 ` Reza Arbab 2016-08-31 14:38 ` Reza Arbab 1 sibling, 2 replies; 24+ messages in thread From: Greg Kroah-Hartman @ 2016-08-31 12:21 UTC (permalink / raw) To: Reza Arbab; +Cc: linux-kernel On Thu, May 26, 2016 at 09:43:04AM -0500, Reza Arbab wrote: > Hi Greg, > > On Thu, May 05, 2016 at 02:07:24PM -0500, Reza Arbab wrote: > > $ echo online > /sys/devices/system/memory/memory0/state > > -bash: echo: write error: Invalid argument > > > > Fix the return value of store_mem_state() so this doesn't happen. > > I can't find where this might have been committed. Am I being impatient, or > did it slip through the cracks? I think it fell through, can you resend it and cc: all of the people involved in this subsystem, like you did for your other patch series? thanks, greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 12:21 ` Greg Kroah-Hartman @ 2016-08-31 14:37 ` Reza Arbab 2016-08-31 15:01 ` Greg Kroah-Hartman 2016-08-31 14:38 ` Reza Arbab 1 sibling, 1 reply; 24+ messages in thread From: Reza Arbab @ 2016-08-31 14:37 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel On Wed, Aug 31, 2016 at 02:21:28PM +0200, Greg Kroah-Hartman wrote: >> I can't find where this might have been committed. Am I being >> impatient, or did it slip through the cracks? > >I think it fell through, can you resend it and cc: all of the people >involved in this subsystem, like you did for your other patch series? I can resend, but I'm not sure what you mean by the rest--this was a singleton patch, and you're the only one listed by get_maintainer.pl: $ scripts/get_maintainer.pl drivers/base/memory.c Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS) linux-kernel@vger.kernel.org (open list) -- Reza Arbab ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 14:37 ` Reza Arbab @ 2016-08-31 15:01 ` Greg Kroah-Hartman 2016-08-31 15:44 ` Reza Arbab 0 siblings, 1 reply; 24+ messages in thread From: Greg Kroah-Hartman @ 2016-08-31 15:01 UTC (permalink / raw) To: Reza Arbab; +Cc: linux-kernel On Wed, Aug 31, 2016 at 09:37:55AM -0500, Reza Arbab wrote: > On Wed, Aug 31, 2016 at 02:21:28PM +0200, Greg Kroah-Hartman wrote: > > > I can't find where this might have been committed. Am I being > > > impatient, or did it slip through the cracks? > > > > I think it fell through, can you resend it and cc: all of the people > > involved in this subsystem, like you did for your other patch series? > > I can resend, but I'm not sure what you mean by the rest--this was a > singleton patch, and you're the only one listed by get_maintainer.pl: > > $ scripts/get_maintainer.pl drivers/base/memory.c > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS) > linux-kernel@vger.kernel.org (open list) You sent in other memory-hotplug patches that got merged through the -mm tree. I suggest doing that here as well, as those developers know this code much better than I do. thanks, greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 15:01 ` Greg Kroah-Hartman @ 2016-08-31 15:44 ` Reza Arbab 0 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-08-31 15:44 UTC (permalink / raw) To: Greg Kroah-Hartman, Andrew Morton, Vlastimil Babka, Vitaly Kuznetsov, David Rientjes, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel Attempting to online memory which is already online will cause this: 1. store_mem_state() called with buf="online" 2. device_online() returns 1 because device is already online 3. store_mem_state() returns 1 4. calling code interprets this as 1-byte buffer read 5. store_mem_state() called again with buf="nline" 6. store_mem_state() returns -EINVAL Example: $ cat /sys/devices/system/memory/memory0/state online $ echo online > /sys/devices/system/memory/memory0/state -bash: echo: write error: Invalid argument Fix the return value of store_mem_state() so this doesn't happen. Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com> --- Andrew et al, Greg asked that this come in through the -mm tree, as you know this code better than him. drivers/base/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 1cea0ba..8e385ea 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -359,7 +359,7 @@ store_mem_state(struct device *dev, err: unlock_device_hotplug(); - if (ret) + if (ret < 0) return ret; return count; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-08-31 15:44 ` Reza Arbab 0 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-08-31 15:44 UTC (permalink / raw) To: Greg Kroah-Hartman, Andrew Morton, Vlastimil Babka, Vitaly Kuznetsov, David Rientjes, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel Attempting to online memory which is already online will cause this: 1. store_mem_state() called with buf="online" 2. device_online() returns 1 because device is already online 3. store_mem_state() returns 1 4. calling code interprets this as 1-byte buffer read 5. store_mem_state() called again with buf="nline" 6. store_mem_state() returns -EINVAL Example: $ cat /sys/devices/system/memory/memory0/state online $ echo online > /sys/devices/system/memory/memory0/state -bash: echo: write error: Invalid argument Fix the return value of store_mem_state() so this doesn't happen. Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com> --- Andrew et al, Greg asked that this come in through the -mm tree, as you know this code better than him. drivers/base/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 1cea0ba..8e385ea 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -359,7 +359,7 @@ store_mem_state(struct device *dev, err: unlock_device_hotplug(); - if (ret) + if (ret < 0) return ret; return count; } -- 1.8.3.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 15:44 ` Reza Arbab @ 2016-08-31 20:25 ` Andrew Morton -1 siblings, 0 replies; 24+ messages in thread From: Andrew Morton @ 2016-08-31 20:25 UTC (permalink / raw) To: Reza Arbab Cc: Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, David Rientjes, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, 31 Aug 2016 10:44:01 -0500 Reza Arbab <arbab@linux.vnet.ibm.com> wrote: > Attempting to online memory which is already online will cause this: > > 1. store_mem_state() called with buf="online" > 2. device_online() returns 1 because device is already online > 3. store_mem_state() returns 1 > 4. calling code interprets this as 1-byte buffer read > 5. store_mem_state() called again with buf="nline" > 6. store_mem_state() returns -EINVAL > > Example: > > $ cat /sys/devices/system/memory/memory0/state > online > $ echo online > /sys/devices/system/memory/memory0/state > -bash: echo: write error: Invalid argument > > Fix the return value of store_mem_state() so this doesn't happen. So.. what *does* happen after the patch? Is some sort of failure still reported? Or am I correct in believing that the operation will appear to have succeeded? If so, is that desirable? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-08-31 20:25 ` Andrew Morton 0 siblings, 0 replies; 24+ messages in thread From: Andrew Morton @ 2016-08-31 20:25 UTC (permalink / raw) To: Reza Arbab Cc: Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, David Rientjes, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, 31 Aug 2016 10:44:01 -0500 Reza Arbab <arbab@linux.vnet.ibm.com> wrote: > Attempting to online memory which is already online will cause this: > > 1. store_mem_state() called with buf="online" > 2. device_online() returns 1 because device is already online > 3. store_mem_state() returns 1 > 4. calling code interprets this as 1-byte buffer read > 5. store_mem_state() called again with buf="nline" > 6. store_mem_state() returns -EINVAL > > Example: > > $ cat /sys/devices/system/memory/memory0/state > online > $ echo online > /sys/devices/system/memory/memory0/state > -bash: echo: write error: Invalid argument > > Fix the return value of store_mem_state() so this doesn't happen. So.. what *does* happen after the patch? Is some sort of failure still reported? Or am I correct in believing that the operation will appear to have succeeded? If so, is that desirable? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 20:25 ` Andrew Morton @ 2016-08-31 21:06 ` David Rientjes -1 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-08-31 21:06 UTC (permalink / raw) To: Andrew Morton Cc: Reza Arbab, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, 31 Aug 2016, Andrew Morton wrote: > > Attempting to online memory which is already online will cause this: > > > > 1. store_mem_state() called with buf="online" > > 2. device_online() returns 1 because device is already online > > 3. store_mem_state() returns 1 > > 4. calling code interprets this as 1-byte buffer read > > 5. store_mem_state() called again with buf="nline" > > 6. store_mem_state() returns -EINVAL > > > > Example: > > > > $ cat /sys/devices/system/memory/memory0/state > > online > > $ echo online > /sys/devices/system/memory/memory0/state > > -bash: echo: write error: Invalid argument > > > > Fix the return value of store_mem_state() so this doesn't happen. > > So.. what *does* happen after the patch? Is some sort of failure still > reported? Or am I correct in believing that the operation will appear > to have succeeded? If so, is that desirable? > It's not desirable, before commit 4f3549d72 this would have returned EINVAL since __memory_block_change_state() does not see the state as MEM_OFFLINE when the write is done. The correct fix is for store_mem_state() to return -EINVAL when device_online() returns non-zero. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-08-31 21:06 ` David Rientjes 0 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-08-31 21:06 UTC (permalink / raw) To: Andrew Morton Cc: Reza Arbab, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, 31 Aug 2016, Andrew Morton wrote: > > Attempting to online memory which is already online will cause this: > > > > 1. store_mem_state() called with buf="online" > > 2. device_online() returns 1 because device is already online > > 3. store_mem_state() returns 1 > > 4. calling code interprets this as 1-byte buffer read > > 5. store_mem_state() called again with buf="nline" > > 6. store_mem_state() returns -EINVAL > > > > Example: > > > > $ cat /sys/devices/system/memory/memory0/state > > online > > $ echo online > /sys/devices/system/memory/memory0/state > > -bash: echo: write error: Invalid argument > > > > Fix the return value of store_mem_state() so this doesn't happen. > > So.. what *does* happen after the patch? Is some sort of failure still > reported? Or am I correct in believing that the operation will appear > to have succeeded? If so, is that desirable? > It's not desirable, before commit 4f3549d72 this would have returned EINVAL since __memory_block_change_state() does not see the state as MEM_OFFLINE when the write is done. The correct fix is for store_mem_state() to return -EINVAL when device_online() returns non-zero. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 21:06 ` David Rientjes @ 2016-08-31 23:38 ` Reza Arbab -1 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-08-31 23:38 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 02:06:14PM -0700, David Rientjes wrote: >The correct fix is for store_mem_state() to return -EINVAL when >device_online() returns non-zero. Let me put it to you this way--which one of these sysfs operations is behaving correctly? # cd /sys/devices/system/memory/memory0 # cat online 1 # echo 1 > online; echo $? 0 or # cd /sys/devices/system/memory/memory0 # cat state online # echo online > state; echo $? -bash: echo: write error: Invalid argument 1 One of them should change to match the other. -- Reza Arbab ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-08-31 23:38 ` Reza Arbab 0 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-08-31 23:38 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 02:06:14PM -0700, David Rientjes wrote: >The correct fix is for store_mem_state() to return -EINVAL when >device_online() returns non-zero. Let me put it to you this way--which one of these sysfs operations is behaving correctly? # cd /sys/devices/system/memory/memory0 # cat online 1 # echo 1 > online; echo $? 0 or # cd /sys/devices/system/memory/memory0 # cat state online # echo online > state; echo $? -bash: echo: write error: Invalid argument 1 One of them should change to match the other. -- Reza Arbab -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 23:38 ` Reza Arbab @ 2016-09-01 0:03 ` David Rientjes -1 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-09-01 0:03 UTC (permalink / raw) To: Reza Arbab Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, 31 Aug 2016, Reza Arbab wrote: > > The correct fix is for store_mem_state() to return -EINVAL when > > device_online() returns non-zero. > > Let me put it to you this way--which one of these sysfs operations is behaving > correctly? > > # cd /sys/devices/system/memory/memory0 > # cat online > 1 > # echo 1 > online; echo $? > 0 > > or > > # cd /sys/devices/system/memory/memory0 > # cat state > online > # echo online > state; echo $? > -bash: echo: write error: Invalid argument > 1 > > One of them should change to match the other. > Nope, the return value of changing state from online to online was established almost 11 years ago in commit 3947be1969a9. This was broken by commit fa2be40fe7c0 ("drivers: base: use standard device online/offline for state change") which was not intended to introduce a functional change, but it did (memory_block_change_state() would have returned EINVAL, device_online() does not). ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-09-01 0:03 ` David Rientjes 0 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-09-01 0:03 UTC (permalink / raw) To: Reza Arbab Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, 31 Aug 2016, Reza Arbab wrote: > > The correct fix is for store_mem_state() to return -EINVAL when > > device_online() returns non-zero. > > Let me put it to you this way--which one of these sysfs operations is behaving > correctly? > > # cd /sys/devices/system/memory/memory0 > # cat online > 1 > # echo 1 > online; echo $? > 0 > > or > > # cd /sys/devices/system/memory/memory0 > # cat state > online > # echo online > state; echo $? > -bash: echo: write error: Invalid argument > 1 > > One of them should change to match the other. > Nope, the return value of changing state from online to online was established almost 11 years ago in commit 3947be1969a9. This was broken by commit fa2be40fe7c0 ("drivers: base: use standard device online/offline for state change") which was not intended to introduce a functional change, but it did (memory_block_change_state() would have returned EINVAL, device_online() does not). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-09-01 0:03 ` David Rientjes @ 2016-09-01 0:17 ` Reza Arbab -1 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-09-01 0:17 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 05:03:25PM -0700, David Rientjes wrote: >Nope, the return value of changing state from online to online was >established almost 11 years ago in commit 3947be1969a9. Fair enough. So if online-to-online is -EINVAL, 1. Shouldn't 'echo 1 > online' then also return -EINVAL? 2. store_mem_state() still needs a tweak, right? It was only returning -EINVAL by accident, due to the convoluted sequence I listed in the patch. -- Reza Arbab ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-09-01 0:17 ` Reza Arbab 0 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-09-01 0:17 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 05:03:25PM -0700, David Rientjes wrote: >Nope, the return value of changing state from online to online was >established almost 11 years ago in commit 3947be1969a9. Fair enough. So if online-to-online is -EINVAL, 1. Shouldn't 'echo 1 > online' then also return -EINVAL? 2. store_mem_state() still needs a tweak, right? It was only returning -EINVAL by accident, due to the convoluted sequence I listed in the patch. -- Reza Arbab -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-09-01 0:17 ` Reza Arbab @ 2016-09-01 0:28 ` David Rientjes -1 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-09-01 0:28 UTC (permalink / raw) To: Reza Arbab Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, 31 Aug 2016, Reza Arbab wrote: > > Nope, the return value of changing state from online to online was > > established almost 11 years ago in commit 3947be1969a9. > > Fair enough. So if online-to-online is -EINVAL, online-to-online for state is -EINVAL, it has been since 2005. > 1. Shouldn't 'echo 1 > online' then also return -EINVAL? > No, it's a different tunable. There's no requirement that two different tunables that do a similar thing have the same return values: the former existed long before device_online() and still exists for backwards compatibility. > 2. store_mem_state() still needs a tweak, right? It was only returning -EINVAL > by accident, due to the convoluted sequence I listed in the patch. > Yes, absolutely. It returning -EINVAL for "nline" is what is accidently preserving it's backwards compatibility :) Note that device_online() returns 1 if already online and memory_subsys_online() returns 0 if online in this case. So we want store_mem_state() to return -EINVAL if device_online() returns non-zero (this was in my first email). ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-09-01 0:28 ` David Rientjes 0 siblings, 0 replies; 24+ messages in thread From: David Rientjes @ 2016-09-01 0:28 UTC (permalink / raw) To: Reza Arbab Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, 31 Aug 2016, Reza Arbab wrote: > > Nope, the return value of changing state from online to online was > > established almost 11 years ago in commit 3947be1969a9. > > Fair enough. So if online-to-online is -EINVAL, online-to-online for state is -EINVAL, it has been since 2005. > 1. Shouldn't 'echo 1 > online' then also return -EINVAL? > No, it's a different tunable. There's no requirement that two different tunables that do a similar thing have the same return values: the former existed long before device_online() and still exists for backwards compatibility. > 2. store_mem_state() still needs a tweak, right? It was only returning -EINVAL > by accident, due to the convoluted sequence I listed in the patch. > Yes, absolutely. It returning -EINVAL for "nline" is what is accidently preserving it's backwards compatibility :) Note that device_online() returns 1 if already online and memory_subsys_online() returns 0 if online in this case. So we want store_mem_state() to return -EINVAL if device_online() returns non-zero (this was in my first email). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-09-01 0:28 ` David Rientjes @ 2016-09-01 1:57 ` Reza Arbab -1 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-09-01 1:57 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 05:28:26PM -0700, David Rientjes wrote: >> 2. store_mem_state() still needs a tweak, right? It was only >> returning -EINVAL by accident, due to the convoluted sequence I >> listed in the patch. > >Yes, absolutely. It returning -EINVAL for "nline" is what is accidently >preserving it's backwards compatibility :) Note that device_online() >returns 1 if already online and memory_subsys_online() returns 0 if online >in this case. So we want store_mem_state() to return -EINVAL if >device_online() returns non-zero (this was in my first email). I'll spin a v3 patch to do this. Thank you for your review! -- Reza Arbab ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value @ 2016-09-01 1:57 ` Reza Arbab 0 siblings, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-09-01 1:57 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Greg Kroah-Hartman, Vlastimil Babka, Vitaly Kuznetsov, Yaowei Bai, Joonsoo Kim, Dan Williams, Xishi Qiu, David Vrabel, Chen Yucong, Andrew Banman, Seth Jennings, linux-mm, linux-kernel On Wed, Aug 31, 2016 at 05:28:26PM -0700, David Rientjes wrote: >> 2. store_mem_state() still needs a tweak, right? It was only >> returning -EINVAL by accident, due to the convoluted sequence I >> listed in the patch. > >Yes, absolutely. It returning -EINVAL for "nline" is what is accidently >preserving it's backwards compatibility :) Note that device_online() >returns 1 if already online and memory_subsys_online() returns 0 if online >in this case. So we want store_mem_state() to return -EINVAL if >device_online() returns non-zero (this was in my first email). I'll spin a v3 patch to do this. Thank you for your review! -- Reza Arbab -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v2] memory-hotplug: fix store_mem_state() return value 2016-08-31 12:21 ` Greg Kroah-Hartman 2016-08-31 14:37 ` Reza Arbab @ 2016-08-31 14:38 ` Reza Arbab 1 sibling, 0 replies; 24+ messages in thread From: Reza Arbab @ 2016-08-31 14:38 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-kernel Attempting to online memory which is already online will cause this: 1. store_mem_state() called with buf="online" 2. device_online() returns 1 because device is already online 3. store_mem_state() returns 1 4. calling code interprets this as 1-byte buffer read 5. store_mem_state() called again with buf="nline" 6. store_mem_state() returns -EINVAL Example: $ cat /sys/devices/system/memory/memory0/state online $ echo online > /sys/devices/system/memory/memory0/state -bash: echo: write error: Invalid argument Fix the return value of store_mem_state() so this doesn't happen. Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com> --- drivers/base/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 1cea0ba..8e385ea 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -359,7 +359,7 @@ store_mem_state(struct device *dev, err: unlock_device_hotplug(); - if (ret) + if (ret < 0) return ret; return count; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2016-09-01 1:57 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-05 19:07 [PATCH v2] memory-hotplug: fix store_mem_state() return value Reza Arbab 2016-05-26 14:43 ` Reza Arbab 2016-05-26 15:34 ` Greg Kroah-Hartman 2016-05-26 15:35 ` Greg Kroah-Hartman 2016-08-31 12:21 ` Greg Kroah-Hartman 2016-08-31 14:37 ` Reza Arbab 2016-08-31 15:01 ` Greg Kroah-Hartman 2016-08-31 15:44 ` [RESEND PATCH " Reza Arbab 2016-08-31 15:44 ` Reza Arbab 2016-08-31 20:25 ` Andrew Morton 2016-08-31 20:25 ` Andrew Morton 2016-08-31 21:06 ` David Rientjes 2016-08-31 21:06 ` David Rientjes 2016-08-31 23:38 ` Reza Arbab 2016-08-31 23:38 ` Reza Arbab 2016-09-01 0:03 ` David Rientjes 2016-09-01 0:03 ` David Rientjes 2016-09-01 0:17 ` Reza Arbab 2016-09-01 0:17 ` Reza Arbab 2016-09-01 0:28 ` David Rientjes 2016-09-01 0:28 ` David Rientjes 2016-09-01 1:57 ` Reza Arbab 2016-09-01 1:57 ` Reza Arbab 2016-08-31 14:38 ` Reza Arbab
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.