Re: [PATCH 2/6] cxl: add sysram_region memory controller

[Gregory Price <gourry@gourry.net>]

On Mon, Jan 12, 2026 at 03:10:41PM -0600, Cheatham, Benjamin wrote:
> On 1/12/2026 10:35 AM, Gregory Price wrote:
> > Add a sysram memctrl that directly hotplugs memory without needing to
> > route through DAX.  This simplifies the sysram usecase considerably.
> > 
> > The sysram memctl adds new sysfs controls when registered:
> > 	region/memctrl/[hotplug, hotunplug, state]
> > 
> > hotplug:   controller attempts to hotplug the memory region
> > hotunplug: controller attempts to offline and hotunplug the memory region
> 
> Nit: Would it be better to use hotadd/hotremove here instead of hotplug/hotunplug? The terms
> are basically synonymous, but I think hotadd and hotremove are more descriptive.

I will defer to David on this.  I think keeping the terminology
consistent is better, but also hotplug is overloaded between physical
and logical.  It ultimately means the same thing to be honest.

> > state:     [online,online_normal,offline]
> >    online       : controller onlines blocks in ZONE_MOVABLE
> >    online_normal: controller onlines blocks in ZONE_NORMAL
> 
> The naming for online states could be improved imo. I understand and agree with the motivation
> behind the names, but I could see the use of the word "normal" being confusing to less savvy users.
> You could change it to include the zone for both (online_movable/online_normal), but I think it may
> be easier to mark which one has drawbacks, i.e. change "online_normal" to something like "online_nonremovable".
> That way, anyone who doesn't want to go find the documentation for these can understand the user-visible
> impact.
> 
> In any case, all of these attributes need ABI documentation as well.
> 

This is what i was getting at originally, I will consider the other
feedback and spin a v2 with this simplified a bit.

I'm leaning towards agreeing with Dan and David that probably we just
keep online/online_movable since it's consistent with base/memory.c, but
we can continue to have this argument.

I don't think we can reasonable get away from users of this interface
understanding the implications of ZONEs, since whatever they choose to
do dictates what zone the memory gets added to.

> > +static DEFINE_MUTEX(cxl_memory_type_lock);
> > +static LIST_HEAD(cxl_memory_types);
> > +
> > +static struct cxl_region *to_cxl_region(struct device *dev)
> > +{
> > +	if (dev->type != &cxl_region_type)
> > +		return NULL;
> > +	return container_of(dev, struct cxl_region, dev);
> > +}
> 
> What's the reasoning behind redefining this in this file? It's still defined in cxl/core/region.c,
> so I would probably just drop the static there and include it through core.h.
> 

Just cruft from rapidly moving stuff around.  Will fixup.

> > +	rc = cxl_sysram_range(cxlr, &range);
> > +	if (rc) {
> > +		dev_info(dev, "range %#llx-%#llx too small after alignment\n",
> > +			 range.start, range.end);
> 
> This should probably be a warning instead. You do it for the next check which is essentially the same
> case, so may as well do it here.

ack.

> > +	if (!total_len) {
> > +		dev_warn(dev, "rejecting CXL region without any memory after alignment\n");
> > +		return -EINVAL;
> > +	}
> 
> I don't think this check is needed. cxl_sysram_range() checks if the range->start == range->end (i.e. size == 0)
> and errors out. That should cause the above check to error out before this.

ack

> > +	/*
> > +	 * Setup flags for System RAM. Leave _BUSY clear so add_memory() can add
> > +	 * a child resource. Do not inherit flags from parent since it may set
> > +	 * flags unknown to us that will the break add_memory() below.
> > +	 */
> > +	res->flags = IORESOURCE_SYSTEM_RAM;
> > +	mhp_flags = MHP_NID_IS_MGID;
> > +	rc = add_memory_driver_managed(data->mgid, range.start,
> > +				       range_len(&range), sysram_name, mhp_flags);
> 
> Look like mhp_flags is only used once, I'd get rid of it and just use MHP_NID_IS_MGID instead.
> 

ack - yeah this was cribbed from dax.c

Thank you!
~Gregory