[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ed7db8b9-e828-420d-a8b2-3e1b8aa8c95c@redhat.com>
Date: Thu, 9 Oct 2025 21:13:06 +0200
From: David Hildenbrand <david@...hat.com>
To: Sumanth Korikkar <sumanthk@...ux.ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>, linux-mm <linux-mm@...ck.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
linux-s390 <linux-s390@...r.kernel.org>,
Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>
Subject: Re: [PATCH v2 2/4] s390/sclp: Add support for dynamic
(de)configuration of memory
Just a couple of nits
> ---
> drivers/s390/char/sclp_mem.c | 290 +++++++++++++++++++++++++----------
> 1 file changed, 207 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/s390/char/sclp_mem.c b/drivers/s390/char/sclp_mem.c
> index 27f49f5fd358..e1302b1c98ac 100644
> --- a/drivers/s390/char/sclp_mem.c
> +++ b/drivers/s390/char/sclp_mem.c
> @@ -9,9 +9,12 @@
> #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
>
> #include <linux/cpufeature.h>
> +#include <linux/container_of.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/kstrtox.h>
> #include <linux/memory.h>
> #include <linux/memory_hotplug.h>
> #include <linux/mm.h>
> @@ -27,7 +30,6 @@
> #define SCLP_CMDW_ASSIGN_STORAGE 0x000d0001
> #define SCLP_CMDW_UNASSIGN_STORAGE 0x000c0001
>
> -static DEFINE_MUTEX(sclp_mem_mutex);
> static LIST_HEAD(sclp_mem_list);
> static u8 sclp_max_storage_id;
> static DECLARE_BITMAP(sclp_storage_ids, 256);
> @@ -38,6 +40,18 @@ struct memory_increment {
> int standby;
> };
>
> +struct sclp_mem {
> + struct kobject kobj;
> + unsigned int id;
> + unsigned int memmap_on_memory;
> + unsigned int config;
> +};
> +
> +struct sclp_mem_arg {
> + struct sclp_mem *sclp_mems;
> + struct kset *kset;
> +};
Just one thought: if you keep either as global variable you wouldn't
need this. (I would just keep both as globals, but whatever you prefer)
Whatever you prefer.
[...]
>
> -static void __init sclp_add_standby_memory(void)
> +static int __init create_standby_sclp_mems(struct sclp_mem *sclp_mems, struct kset *kset)
> {
> struct memory_increment *incr;
> + int rc = 0;
>
> list_for_each_entry(incr, &sclp_mem_list, list) {
> if (incr->standby)
> - add_memory_merged(incr->rn);
> + rc = create_standby_sclp_mems_merged(sclp_mems, kset, incr->rn);
> + if (rc)
> + goto out;
Why not "return rc;" to avoid the goto label?
> }
> - add_memory_merged(0);
> + rc = create_standby_sclp_mems_merged(sclp_mems, kset, 0);
> +out:
> + return rc;
> +}
> +
> +static int __init init_sclp_mem(void)
> +{
> + const u64 block_size = memory_block_size_bytes();
Instead of "u64" maybe "unsigned long" like memory_block_size_bytes()
returns?
> + const u64 max_sclp_mems = roundup(sclp.rnmax * sclp.rzm, block_size) / block_size;
Instead of u64 maybe "unsigned int" like the ids you store per sclp_mem?
> + struct sclp_mem *sclp_mems;
> + struct sclp_mem_arg arg;
> + struct kset *kset;
> + int rc;
> +
> + /* Allocate memory for all blocks ahead of time. */
> + sclp_mems = kcalloc(max_sclp_mems, sizeof(struct sclp_mem), GFP_KERNEL);
> + if (!sclp_mems)
> + return -ENOMEM;
> +
> + kset = kset_create_and_add("memory", NULL, firmware_kobj);
> + if (!kset)
> + return -ENOMEM;
I guess we don't care about freeing sclp_mems in that case? Likely it
should never ever happen either way.
--
Cheers
David / dhildenb
Powered by blists - more mailing lists