Re: [PATCH v3 2/2] dax: add a sysfs knob to control memmap_on_memory behavior

[Verma, Vishal L]

On Tue, 2023-12-12 at 08:56 +0800, Huang, Ying wrote:
> "Verma, Vishal L" <vishal.l.verma@xxxxxxxxx> writes:
> 
> > On Tue, 2023-12-12 at 08:30 +0800, Huang, Ying wrote:
> > > 
> > > > +
> > > > +static ssize_t memmap_on_memory_store(struct device *dev,
> > > > +                                     struct device_attribute *attr,
> > > > +                                     const char *buf, size_t len)
> > > > +{
> > > > +       struct device_driver *drv = dev->driver;
> > > > +       struct dev_dax *dev_dax = to_dev_dax(dev);
> > > > +       struct dax_region *dax_region = dev_dax->region;
> > > > +       struct dax_device_driver *dax_drv = to_dax_drv(drv);
> > > > +       ssize_t rc;
> > > > +       bool val;
> > > > +
> > > > +       rc = kstrtobool(buf, &val);
> > > > +       if (rc)
> > > > +               return rc;
> > > > +
> > > > +       if (dev_dax->memmap_on_memory == val)
> > > > +               return len;
> > > > +
> > > > +       device_lock(dax_region->dev);
> > > > +       if (!dax_region->dev->driver) {
> > > > +               device_unlock(dax_region->dev);
> > > > +               return -ENXIO;
> > > > +       }
> > > 
> > > I think that it should be OK to write to "memmap_on_memory" if no driver
> > > is bound to the device.  We just need to avoid to write to it when kmem
> > > driver is bound.
> > 
> > Oh this is just a check on the region driver, not for a dax driver
> > being bound to the device. It's the same as what things like
> > align_store(), size_store() etc. do for dax device reconfiguration.
> 
> Sorry, I misunderstood it.
> 
> > That said, it might be okay to remove this check, as this operation
> > doesn't change any attributes of the dax region (the other interfaces I
> > mentioned above can affect regions, so we want to lock the region
> > device). If removing the check, we'd drop the region lock acquisition
> > as well.
> 
> This sounds good to me.
> 
> And is it necessary to check driver type with device_lock()?  Can driver
> be changed between checking and lock?
> 
Oh, good point, the type check should happen with the device lock held.
I'll make that change.