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

Sumanth Korikkar
2025-10-10T08:09:44Z

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