On Wed, 2018-11-28 at 17:57 -0800, Dan Williams wrote: > On Wed, Nov 28, 2018 at 4:32 PM Alexander Duyck > <[email protected]> wrote: > > > > Add an additional bit flag to the device struct named async_probe. This > > additional flag allows us to guarantee ordering between probe and remove > > operations. > > > > This allows us to guarantee that if we execute a remove operation on a > > You missed the review comment on the usage of "us". I've long been an > abuser of this as well saying "we" and "us" to casually refer to > whatever part of the kernel I'm currently modifying. The problem is > that it is ambiguous and assumes the reader happens translates the > "us" / "we" to the same specific subject you had in mind. It leaves > room for confusion that can be eliminated by explicitly referencing > the expected agent, subject, object in mind. > > I long blew off suggestions to correct usages like this, but it > finally sunk in for me after reading Thomas' rewrite of a "we" and > "this" laden changelog, and why he and other tip-maintainers want to > push back on the usage in the tip tree, see the "Changelog" section of > the guidance in "[patch 2/2] Documentation/process: Add tip tree > handbook": https://lkml.org/lkml/2018/11/7/932. > > Patch review is quicker without the speed bumps of translating > occurrences of the "we" and "us"
It wasn't my intention to blow it off. I have gone through and updated it in my repo and I can see how it can be confusing as in one spot I wasn't sure if the "we"/"us" was the probe or the remove routine. > > given interface it will not attempt to update the driver member > > asynchronously following the earlier operation. Previously this guarantee > > was not present and could result in us attempting to remove a driver from > > an interface only to have it attempt to attach the driver later when we > > finally complete the deferred asynchronous probe call. > > > > Reviewed-by: Bart Van Assche <[email protected]> > > Signed-off-by: Alexander Duyck <[email protected]> > > --- > > drivers/base/dd.c | 16 ++++++++++++++++ > > include/linux/device.h | 3 +++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > index 88713f182086..ef3f70a7cb5a 100644 > > --- a/drivers/base/dd.c > > +++ b/drivers/base/dd.c > > @@ -774,6 +774,10 @@ static void __device_attach_async_helper(void *_dev, > > async_cookie_t cookie) > > > > device_lock(dev); > > > > + /* nothing to do if async_probe has been cleared */ > > + if (!dev->async_probe) > > + goto out_unlock; > > + > > if (dev->parent) > > pm_runtime_get_sync(dev->parent); > > > > @@ -785,6 +789,9 @@ static void __device_attach_async_helper(void *_dev, > > async_cookie_t cookie) > > if (dev->parent) > > pm_runtime_put(dev->parent); > > > > + /* We made our attempt at an async_probe, clear the flag */ > > + dev->async_probe = false; > > +out_unlock: > > device_unlock(dev); > > > > put_device(dev); > > @@ -829,6 +836,7 @@ static int __device_attach(struct device *dev, bool > > allow_async) > > */ > > dev_dbg(dev, "scheduling asynchronous probe\n"); > > get_device(dev); > > + dev->async_probe = true; > > async_schedule(__device_attach_async_helper, dev); > > } else { > > pm_request_idle(dev); > > @@ -929,6 +937,14 @@ static void __device_release_driver(struct device > > *dev, struct device *parent) > > { > > struct device_driver *drv; > > > > + /* > > + * In the event that we are asked to release the driver on an > > + * interface that is still waiting on a probe we can just terminate > > + * the probe by setting async_probe to false. When the async call > > + * is finally completed it will see this state and just exit. > > + */ > > + dev->async_probe = false; > > + > > drv = dev->driver; > > if (drv) { > > while (device_links_busy(dev)) { > > diff --git a/include/linux/device.h b/include/linux/device.h > > index 1b25c7a43f4c..4d2eb2c74149 100644 > > --- a/include/linux/device.h > > +++ b/include/linux/device.h > > @@ -957,6 +957,8 @@ struct dev_links_info { > > * device. > > * @dma_coherent: this particular device is dma coherent, even if the > > * architecture supports non-coherent devices. > > + * @async_probe: This device has an asynchronous probe event pending. > > Should > > + * only be updated while holding device lock. > > * > > * At the lowest level, every device in a Linux system is represented by an > > * instance of struct device. The device structure contains the information > > @@ -1051,6 +1053,7 @@ struct device { > > defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL) > > bool dma_coherent:1; > > #endif > > + bool async_probe:1; > > I think this flag is misnamed, the wrong polarity and should be set in > the device removal path, not the driver detach path. The wider problem > is the removal of a device while actions initiated by its arrival may > still be in flight, or have yet to start. It's not just the probe path > in the driver-core that might be interested in this state, but also > bus implementations that kick off their own async operations. Okay, so increase the scope so that the information is usable outside of driver core. > I think the flag should be named "cancel" and set it in the > device_del() path. Otherwise this is encoding code flow state in the > struct rather than device-state that the code needs to comprehend. Instead of "cancel" what would you think of "dead"? In my mind once we call device_del we are essentially working with a dead device object so that might make more sense in terms of a state rather than "cancel" which doesn't really tell us what should be canceled. Looking over the code I could probably set it before we start calling the notifiers for BUS_NOTIFY_DEL_DEVICE. The only thing I am not sure about is if we would need to add any sort of synchronization primitives around it. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
