Re: [PATCH v2 2/4] s390/sclp: Add support for dynamic (de)configuration of memory

David Hildenbrand
2025-10-09T19:13:13Z

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