On Sat, Jun 4, 2016 at 2:10 PM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> 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 <gre...@linuxfoundation.org>
>> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
>> ---
>> 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);
> 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.
If making device_release_driver() fail is an option I can take a look,
but that seems like a major change in policy.
