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

[Sumanth Korikkar]

> > +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.

Hi David,

I prefer to preserve the ones we have.

> > -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?

Sure. Will add it.

> >   	}
> > -	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?

Noted.

> > +	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?

Sure.

> > +	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.

Right. We dont care about freeing sclp_mems here.

Thank you.