On Thu, Jul 09, 2026 at 03:36:23PM -0700, Dan Williams (nvidia) wrote:
> Gregory Price wrote:
> > + ranges = kmalloc_array(dev_dax->nr_range, sizeof(*ranges), GFP_KERNEL);
>
> The new hotness is:
>
> ranges = kmalloc_objs(*ranges, dev_dax->nr_range);
>
ack.
> > +static ssize_t state_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct dax_kmem_data *data = dev_get_drvdata(dev);
> > + const char *state_str;
> > +
> > + if (!data)
> > + return -ENXIO;
>
> Cannot happen with a dev_group attribute. If probe succeeds then drvdata
> is present. If probe fails, attribute never appears.
>
> When unplugging, dev_groups are unregistered before drvdata is cleared.
>
Ah good to know. I am always paranoid, but i'll drop.
> > +
> > + if (data->state == DAX_KMEM_UNPLUGGED)
> > + state_str = "unplugged";
> > + else
> > + state_str = mhp_online_type_to_str(data->state);
> > +
> > + return sysfs_emit(buf, "%s\n", state_str ?: "unknown");
>
> So the "unknown" case does not need to be here.
>
mhp_online_type_to_str can technically return NULL, seems better to not
just let a NULL dereference sit latent even if we can visually tell it
can't happen today?
> > + /* Always create blocks for backward compatibility, even if offline */
>
> Unless maybe a driver knows better and wants to preclude the possibility
> of legacy per-block hotplug policy firing? I.e. driver asks for dax_kmem
> to start in the unplugged mode per my "should DAX_KMEM_UNPLUGGED be a
> online_type with a different sentinel".
>
I suppose I can put DAX_KMEM_UNPLUGGED in the header and allow this.
That seems reasonable, and makes sense why to differentiate
DEFAULT/UNPLUGGED.
ack.
> > + rc = device_create_file(dev, &dev_attr_state);
> > + if (rc)
> > + dev_warn(dev, "failed to create state sysfs entry\n");
>
> Always prefer statically declared attributes. In this case an attribute
> that only appears while the driver is attached would be something like:
>
ack.
> > - success = dax_kmem_do_hotremove(dev_dax, data);
> > - if (success < dev_dax->nr_range) {
> > - dev_err(dev, "Hotplug regions stuck online until reboot\n");
> > + if (dax_kmem_state_is_online(data->state)) {
> > + dev_warn(dev, "Hotplug regions stuck online until reboot\n");
>
> I like that the BUG() is avoided, but I think these should stay
> dev_err() given the severity.
>
I had to go back to calling remove_memory() by default given different
feedback, but I think if anything I will just modify the BUG() to a
WARN() and call it a day.
ack on dev_err().
> > + struct device *dev = &dev_dax->dev;
> > +
> > + device_remove_file(dev, &dev_attr_state);
> > +
>
> One less cleanup to do if the attribute is registered statically.
> Attributes are shutdown prior to this point.
>
ack.
~Gregory
