Re: [PATCH 01/13] driver core, libnvdimm: disable manual unbind of dimms while region active

[Greg Kroah-Hartman]

On Sat, Jun 04, 2016 at 02:39:02PM -0700, Dan Williams wrote:
> On Sat, Jun 4, 2016 at 2:10 PM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Sat, Jun 04, 2016 at 01:52:38PM -0700, Dan Williams wrote:
> >> There are scenarios where we need a middle ground between disabling all
> >> manual bind/unbind attempts (via driver->suppress_bind_attrs) and
> >> allowing unbind at any userspace-determined time.  Pinning modules takes
> >> away one vector for unwanted out-of-sequence device_release_driver()
> >> invocations, this new mechanism (via device->suppress_unbind_attr) takes
> >> away another.
> >>
> >> The first user of this mechanism is the libnvdimm sub-system where
> >> manual dimm disabling should be prevented while the dimm is active in
> >> any region.  Note that there is a 1:N dimm-to-region relationship which
> >> is why this is implemented as a disable count rather than a flag.  This
> >> forces userspace to disable regions before dimms when manually shutting
> >> down a bus topology.
> >>
> >> This does not affect any of the kernel-internal initiated invocations of
> >> device_release_driver().
> >>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> >> ---
> >>  drivers/base/base.h             |    1 +
> >>  drivers/base/bus.c              |   12 ++++++++++--
> >>  drivers/base/core.c             |    1 +
> >>  drivers/base/dd.c               |    2 +-
> >>  drivers/nvdimm/namespace_devs.c |    1 +
> >>  drivers/nvdimm/region_devs.c    |    4 +++-
> >>  include/linux/device.h          |   20 ++++++++++++++++++++
> >>  7 files changed, 37 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/base/base.h b/drivers/base/base.h
> >> index e05db388bd1c..129814b17ca6 100644
> >> --- a/drivers/base/base.h
> >> +++ b/drivers/base/base.h
> >> @@ -109,6 +109,7 @@ extern int bus_add_driver(struct device_driver *drv);
> >>  extern void bus_remove_driver(struct device_driver *drv);
> >>
> >>  extern void driver_detach(struct device_driver *drv);
> >> +extern void __device_release_driver(struct device *dev);
> >>  extern int driver_probe_device(struct device_driver *drv, struct device *dev);
> >>  extern void driver_deferred_probe_del(struct device *dev);
> >>  static inline int driver_match_device(struct device_driver *drv,
> >> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> >> index 6470eb8088f4..b48a903f2d28 100644
> >> --- a/drivers/base/bus.c
> >> +++ b/drivers/base/bus.c
> >> @@ -188,10 +188,18 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
> >>       if (dev && dev->driver == drv) {
> >>               if (dev->parent)        /* Needed for USB */
> >>                       device_lock(dev->parent);
> >> -             device_release_driver(dev);
> >> +
> >> +             device_lock(dev);
> >> +             if (atomic_read(&dev->suppress_unbind_attr) > 0)
> >> +                     err = -EBUSY;
> >> +             else {
> >> +                     __device_release_driver(dev);
> >> +                     err = count;
> >> +             }
> >> +             device_unlock(dev);
> >> +
> >>               if (dev->parent)
> >>                       device_unlock(dev->parent);
> >> -             err = count;
> >>       }
> >>       put_device(dev);
> >>       bus_put(bus);
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index 0a8bdade53f2..0ea0e8560e1d 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -708,6 +708,7 @@ void device_initialize(struct device *dev)
> >>       INIT_LIST_HEAD(&dev->devres_head);
> >>       device_pm_init(dev);
> >>       set_dev_node(dev, -1);
> >> +     atomic_set(&dev->suppress_unbind_attr, 0);
> >>  #ifdef CONFIG_GENERIC_MSI_IRQ
> >>       INIT_LIST_HEAD(&dev->msi_list);
> >>  #endif
> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> >> index 16688f50729c..9e21817ca2d6 100644
> >> --- a/drivers/base/dd.c
> >> +++ b/drivers/base/dd.c
> >> @@ -756,7 +756,7 @@ EXPORT_SYMBOL_GPL(driver_attach);
> >>   * __device_release_driver() must be called with @dev lock held.
> >>   * When called for a USB interface, @dev->parent lock must be held as well.
> >>   */
> >> -static void __device_release_driver(struct device *dev)
> >> +void __device_release_driver(struct device *dev)
> >>  {
> >>       struct device_driver *drv;
> >>
> >> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> >> index c5e3196c45b0..e65572b6092c 100644
> >> --- a/drivers/nvdimm/namespace_devs.c
> >> +++ b/drivers/nvdimm/namespace_devs.c
> >> @@ -1950,6 +1950,7 @@ static int init_active_labels(struct nd_region *nd_region)
> >>               }
> >>               nd_mapping->ndd = ndd;
> >>               atomic_inc(&nvdimm->busy);
> >> +             device_disable_unbind_attr(&nvdimm->dev);
> >>               get_ndd(ndd);
> >>
> >>               count = nd_label_active_count(ndd);
> >> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> >> index 40fcfea26fbb..320f0f3ea367 100644
> >> --- a/drivers/nvdimm/region_devs.c
> >> +++ b/drivers/nvdimm/region_devs.c
> >> @@ -427,8 +427,10 @@ static void nd_region_notify_driver_action(struct nvdimm_bus *nvdimm_bus,
> >>                       nd_mapping->labels = NULL;
> >>                       put_ndd(ndd);
> >>                       nd_mapping->ndd = NULL;
> >> -                     if (ndd)
> >> +                     if (ndd) {
> >>                               atomic_dec(&nvdimm->busy);
> >> +                             device_enable_unbind_attr(&nvdimm->dev);
> >> +                     }
> >>               }
> >>
> >>               if (is_nd_pmem(dev))
> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index 38f02814d53a..d9eaa85bb9cf 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -849,6 +849,7 @@ struct device {
> >>
> >>       void    (*release)(struct device *dev);
> >>       struct iommu_group      *iommu_group;
> >> +     atomic_t                suppress_unbind_attr; /* disable manual unbind */
> >>
> >>       bool                    offline_disabled:1;
> >>       bool                    offline:1;
> >> @@ -988,6 +989,25 @@ static inline void device_lock_assert(struct device *dev)
> >>       lockdep_assert_held(&dev->mutex);
> >>  }
> >>
> >> +static inline bool device_disable_unbind_attr(struct device *dev)
> >> +{
> >> +     bool suppressed = false;
> >> +
> >> +     device_lock(dev);
> >> +     if (dev->driver) {
> >> +             atomic_inc(&dev->suppress_unbind_attr);
> >> +             suppressed = true;
> >> +     }
> >> +     device_unlock(dev);
> >> +
> >> +     return suppressed;
> >> +}
> >> +
> >> +static inline bool device_enable_unbind_attr(struct device *dev)
> >> +{
> >> +     return atomic_dec_and_test(&dev->suppress_unbind_attr);
> >> +}
> >> +
> >
> > Ick, that's a mess.
> >
> > Why not just prevent the unbinding from happening in your bus when you
> > need it?
> 
> Because historically unbind never fails...
> 
>     void device_release_driver(struct device *dev);

Ah, yes, forgot about that.

> > And as you are grabbing a lock, why is this an atomic variable?
> >
> > This is just making things _really_ complex for very limited gain for
> > what I can tell.
> 
> I thought it was cleaner to have the disable action synchronize with
> unbind, but the lock isn't needed to re-enable unbind.  I can move the
> complexity to the caller to bounce the device_lock() and re-validate
> that dev->driver is still set, but that does not seem cleaner.

No, if the user wants to unbind, then they can unbind, don't try to
muddy up the situation by letting it work sometimes and others not at
all.

bind/unbind is a "I really really know what I am doing here" action,
it's rare, and the user gets to keep both pieces if something fails.  I
know some busses don't like it, so we allow them to opt-out.  But to
make it "sometimes yes, sometimes no" is a mess, I don't want to support
that.

I suggest you just don't allow this if you don't want it.  It's not
anything you should ever be relying on for configuration so it shouldn't
be a big deal.

thanks,

greg k-h