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

[David Hildenbrand <david@redhat.com>]

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