* [PATCH 0/2] drivers/base/memory: Two small cleanups
@ 2023-01-20 5:57 Gavin Shan
2023-01-20 5:57 ` [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() Gavin Shan
2023-01-20 5:57 ` [PATCH 2/2] drivers/base/memory: Use array to show memory block state Gavin Shan
0 siblings, 2 replies; 6+ messages in thread
From: Gavin Shan @ 2023-01-20 5:57 UTC (permalink / raw)
To: linux-mm; +Cc: linux-kernel, david, osalvador, gregkh, rafael, shan.gavin
PATCH[1] Fixes comments for phys_index_show() since it's showing
the memory block ID instead of the section ID.
PATCH[2] Use an array to show memory block state in state_show()
Gavin Shan (2):
drivers/base/memory: Fix comments for phys_index_show()
drivers/base/memory: Use array to show memory block state
drivers/base/memory.c | 34 ++++++++--------------------------
1 file changed, 8 insertions(+), 26 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() 2023-01-20 5:57 [PATCH 0/2] drivers/base/memory: Two small cleanups Gavin Shan @ 2023-01-20 5:57 ` Gavin Shan 2023-01-23 8:40 ` David Hildenbrand 2023-01-20 5:57 ` [PATCH 2/2] drivers/base/memory: Use array to show memory block state Gavin Shan 1 sibling, 1 reply; 6+ messages in thread From: Gavin Shan @ 2023-01-20 5:57 UTC (permalink / raw) To: linux-mm; +Cc: linux-kernel, david, osalvador, gregkh, rafael, shan.gavin According to 'admin-guide/mm/memory-hotplug.rst', the memory block ID, instead of the section index, is shown by '/sys/devices/system/memory/ memoryX/phys_index'. Fix the comments to match with 'admin-guide/mm/memory-hotplug.rst'. Besides, use the existing helper memory_block_id() to convert the section index to the memory block index. No functional change intended. Signed-off-by: Gavin Shan <gshan@redhat.com> --- drivers/base/memory.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/base/memory.c b/drivers/base/memory.c index fe98fb8d94e5..b456ac213610 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -115,18 +115,13 @@ unsigned long __weak memory_block_size_bytes(void) } EXPORT_SYMBOL_GPL(memory_block_size_bytes); -/* - * Show the first physical section index (number) of this memory block. - */ +/* Show the memory block ID, relative to the memory block size */ static ssize_t phys_index_show(struct device *dev, struct device_attribute *attr, char *buf) { struct memory_block *mem = to_memory_block(dev); - unsigned long phys_index; - - phys_index = mem->start_section_nr / sections_per_block; - return sysfs_emit(buf, "%08lx\n", phys_index); + return sysfs_emit(buf, "%08lx\n", memory_block_id(mem->start_section_nr)); } /* -- 2.23.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() 2023-01-20 5:57 ` [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() Gavin Shan @ 2023-01-23 8:40 ` David Hildenbrand 0 siblings, 0 replies; 6+ messages in thread From: David Hildenbrand @ 2023-01-23 8:40 UTC (permalink / raw) To: Gavin Shan, linux-mm; +Cc: linux-kernel, osalvador, gregkh, rafael, shan.gavin On 20.01.23 06:57, Gavin Shan wrote: > According to 'admin-guide/mm/memory-hotplug.rst', the memory block ID, > instead of the section index, is shown by '/sys/devices/system/memory/ > memoryX/phys_index'. > > Fix the comments to match with 'admin-guide/mm/memory-hotplug.rst'. > Besides, use the existing helper memory_block_id() to convert the section > index to the memory block index. > > No functional change intended. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com> -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drivers/base/memory: Use array to show memory block state 2023-01-20 5:57 [PATCH 0/2] drivers/base/memory: Two small cleanups Gavin Shan 2023-01-20 5:57 ` [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() Gavin Shan @ 2023-01-20 5:57 ` Gavin Shan 2023-01-20 13:14 ` Greg KH 1 sibling, 1 reply; 6+ messages in thread From: Gavin Shan @ 2023-01-20 5:57 UTC (permalink / raw) To: linux-mm; +Cc: linux-kernel, david, osalvador, gregkh, rafael, shan.gavin 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])) return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state); - } - return sysfs_emit(buf, "%s\n", output); + return sysfs_emit(buf, "%s\n", mem_state_str[mem->state]); } int memory_notify(unsigned long val, void *v) -- 2.23.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drivers/base/memory: Use array to show memory block state 2023-01-20 5:57 ` [PATCH 2/2] drivers/base/memory: Use array to show memory block state Gavin Shan @ 2023-01-20 13:14 ` Greg KH 2023-01-20 22:59 ` Gavin Shan 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2023-01-20 13:14 UTC (permalink / raw) To: Gavin Shan; +Cc: linux-mm, linux-kernel, david, osalvador, rafael, shan.gavin 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, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drivers/base/memory: Use array to show memory block state 2023-01-20 13:14 ` Greg KH @ 2023-01-20 22:59 ` Gavin Shan 0 siblings, 0 replies; 6+ messages in thread From: Gavin Shan @ 2023-01-20 22:59 UTC (permalink / raw) To: Greg KH; +Cc: linux-mm, linux-kernel, david, osalvador, rafael, shan.gavin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-23 8:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-20 5:57 [PATCH 0/2] drivers/base/memory: Two small cleanups Gavin Shan 2023-01-20 5:57 ` [PATCH 1/2] drivers/base/memory: Fix comments for phys_index_show() Gavin Shan 2023-01-23 8:40 ` David Hildenbrand 2023-01-20 5:57 ` [PATCH 2/2] drivers/base/memory: Use array to show memory block state Gavin Shan 2023-01-20 13:14 ` Greg KH 2023-01-20 22:59 ` Gavin Shan
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.