On 06.08.19 11:01, David Hildenbrand wrote:
"s/driver/drivers/" in subject.
I think long term, we should move the whole memory block size
configuration (set_memory_block_size_order() and
memory_block_size_bytes()) into drivers/base/memory.c.
> Let's validate the memory block size early, when initializing the
> memory device infrastructure. Fail hard in case the value is not
> suitable.
>
> As nobody checks the return value of memory_dev_init(), turn it into a
> void function and fail with a panic in all scenarios instead. Otherwise,
> we'll crash later during boot when core/drivers expect that the memory
> device infrastructure (including memory_block_size_bytes()) works as
> expected.
>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
> drivers/base/memory.c | 31 +++++++++----------------------
> include/linux/memory.h | 6 +++---
> 2 files changed, 12 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 790b3bcd63a6..6bea4f3f8040 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -100,21 +100,6 @@ unsigned long __weak memory_block_size_bytes(void)
> }
> EXPORT_SYMBOL_GPL(memory_block_size_bytes);
>
> -static unsigned long get_memory_block_size(void)
> -{
> - unsigned long block_sz;
> -
> - block_sz = memory_block_size_bytes();
> -
> - /* Validate blk_sz is a power of 2 and not less than section size */
> - if ((block_sz & (block_sz - 1)) || (block_sz < MIN_MEMORY_BLOCK_SIZE)) {
> - WARN_ON(1);
> - block_sz = MIN_MEMORY_BLOCK_SIZE;
> - }
> -
> - return block_sz;
> -}
> -
> /*
> * Show the first physical section index (number) of this memory block.
> */
> @@ -461,7 +446,7 @@ static DEVICE_ATTR_RO(removable);
> static ssize_t block_size_bytes_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - return sprintf(buf, "%lx\n", get_memory_block_size());
> + return sprintf(buf, "%lx\n", memory_block_size_bytes());
> }
>
> static DEVICE_ATTR_RO(block_size_bytes);
> @@ -811,19 +796,22 @@ static const struct attribute_group *memory_root_attr_groups[] = {
> /*
> * Initialize the sysfs support for memory devices...
> */
> -int __init memory_dev_init(void)
> +void __init memory_dev_init(void)
> {
> int ret;
> int err;
> unsigned long block_sz, nr;
>
> + /* Validate the configured memory block size */
> + block_sz = memory_block_size_bytes();
> + if (!is_power_of_2(block_sz) || block_sz < MIN_MEMORY_BLOCK_SIZE)
> + panic("Memory block size not suitable: 0x%lx\n", block_sz);
> + sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
> +
> ret = subsys_system_register(&memory_subsys, memory_root_attr_groups);
> if (ret)
> goto out;
>
> - block_sz = get_memory_block_size();
> - sections_per_block = block_sz / MIN_MEMORY_BLOCK_SIZE;
> -
> /*
> * Create entries for memory sections that were found
> * during boot and have been initialized
> @@ -839,8 +827,7 @@ int __init memory_dev_init(void)
>
> out:
> if (ret)
> - printk(KERN_ERR "%s() failed: %d\n", __func__, ret);
> - return ret;
> + panic("%s() failed: %d\n", __func__, ret);
> }
>
> /**
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 704215d7258a..0ebb105eb261 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -79,9 +79,9 @@ struct mem_section;
> #define IPC_CALLBACK_PRI 10
>
> #ifndef CONFIG_MEMORY_HOTPLUG_SPARSE
> -static inline int memory_dev_init(void)
> +static inline void memory_dev_init(void)
> {
> - return 0;
> + return;
> }
> static inline int register_memory_notifier(struct notifier_block *nb)
> {
> @@ -112,7 +112,7 @@ extern int register_memory_isolate_notifier(struct notifier_block *nb);
> extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
> int create_memory_block_devices(unsigned long start, unsigned long size);
> void remove_memory_block_devices(unsigned long start, unsigned long size);
> -extern int memory_dev_init(void);
> +extern void memory_dev_init(void);
> extern int memory_notify(unsigned long val, void *v);
> extern int memory_isolate_notify(unsigned long val, void *v);
> extern struct memory_block *find_memory_block(struct mem_section *);
>
--
Thanks,
David / dhildenb
