Re: [PATCH 2/9] mm/memory_hotplug: add __add_memory_driver_managed() with online_type arg

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 2 Feb 2026 13:02:10 -0500
Gregory Price <gourry@gourry.net> wrote:

> On Mon, Feb 02, 2026 at 05:25:24PM +0000, Jonathan Cameron wrote:
> > On Thu, 29 Jan 2026 16:04:35 -0500
> > Gregory Price <gourry@gourry.net> wrote:
> >   
> > > Enable dax kmem driver to select how to online the memory rather than
> > > implicitly depending on the system default.  This will allow users of
> > > dax to plumb through a preferred auto-online policy for their region.
> > > 
> > > Refactor and new interface:
> > > Add __add_memory_driver_managed() which accepts an explicit online_type
> > > and export mhp_get_default_online_type() so callers can pass it when
> > > they want the default behavior.  
> > 
> > Hi Gregory,
> > 
> > I think maybe I'd have left the export for the first user outside of
> > memory_hotplug.c. Not particularly important however.
> > 
> > Maybe talk about why a caller of __add_memory_driver_managed() might want
> > the default?  Feels like that's for the people who don't...
> >  
> 
> Less about why they want the default, more about maintaining backward
> compatibility.
> 
> In the cxl driver, Ben pointed out something that made me realize we can
> change `region/bind()` to actually use the new `sysram/bind` path by
> just adding a one line `sysram_regionN->online_type = default()`
> 
> I can add this detail to the changelog.
> 
> > 
> > Other comments are mostly about using a named enum. I'm not sure
> > if there is some existing reason why that doesn't work?  -Errno pushed through
> > this variable or anything like that?
> >   
> 
> I can add a cleanup-patch prior to use the enum, but i don't think this
> actually enables the compiler to do anything new at the moment?

Good point. More coffee needed (or sleep)

It lets sparse do some checking, but sadly only for wrong enum assignment.
(Gcc has -Wenum-conversion as well which I think is effectively the same)
I.e. you can't assign a value from a different enum without casting.

It can't do anything if people just pass in an out of range int.

> 
> An enum just resolves to an int, and setting `enum thing val = -1` when
> the enum definition doesn't include -1 doesn't actually fire any errors
> (at least IIRC - maybe i'm just wrong). Same with
> 
>    function(enum) -> function(-1) wouldn't fire a compilation error
> 
> It might actually be worth adding `MMOP_NOT_CONFIGURED = -1` so that the
> cxl-sysram driver can set this explicitly rather than just setting -1
> as an implicit version of this - but then why would memory_hotplug.c
> ever want to expose a NOT_CONFIGURED option lol.
> 
> So, yeah, the enum looks nicer, but not sure how much it buys us beyond
> that.
> 
> > It's a little odd to add nice kernel-doc formatted documentation
> > when the non __ variant has free form docs.  Maybe tidy that up first
> > if we want to go kernel-doc in this file?  (I'm in favor, but no idea
> > on general feelings...)
> >  
> 
> ack.  Can add some more cleanups early in the series.
> 
> > > +	if (online_type < 0 || online_type > MMOP_ONLINE_MOVABLE)  
> > 
> > This is where using an enum would help compiler know what is going on
> > and maybe warn if anyone writes something that isn't defined.
> >  
> 
> I think you still have to sanity check this, but maybe the code looks
> cleaner, so will do. 

I'm in two minds about this. If it's an enum and someone writes an int
I take take the view it's not our problem that they shot themselves in
the foot.  Maybe we should be paranoid...

J


> 
> ~Gregory
>