Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Thu, 14 Nov 2019 23:07:45 +0100 | Subject | Re: [PATCH v2] driver core: Allow device link operations inside sync_state() |
| |
On Thu, Nov 14, 2019 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Nov 14, 2019 at 1:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Nov 14, 2019 at 8:42 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > Some sync_state() implementations might need to call APIs that in turn > > > make calls to device link APIs. So, do the sync_state() callbacks > > > without holding the device link lock. > > > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > > > It would have been kind of nice to let me respond to your last reply > > before sending this. Oh well. > > > > > --- > > > drivers/base/core.c | 76 ++++++++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 68 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c > > > index e6d3e6d485da..2f14d4bf1472 100644 > > > --- a/drivers/base/core.c > > > +++ b/drivers/base/core.c > > > @@ -48,6 +48,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup); > > > static LIST_HEAD(wait_for_suppliers); > > > static DEFINE_MUTEX(wfs_lock); > > > static LIST_HEAD(deferred_sync); > > > +static LIST_HEAD(sync_list); > > > +static DEFINE_MUTEX(sync_lock); > > > > The two items above are not needed AFAICS. > > > > > static unsigned int defer_sync_state_count = 1; > > > > > > #ifdef CONFIG_SRCU > > > @@ -695,7 +697,23 @@ int device_links_check_suppliers(struct device *dev) > > > return ret; > > > } > > > > > > -static void __device_links_supplier_sync_state(struct device *dev) > > > +/** __device_links_queue_sync_state - Queue a device for sync_state() callback > > > > This should be > > > > /** > > * __device_links_queue_sync_state - Queue a device for sync_state() callback > > > > > + * @dev: Device to call sync_state() on > > > + * > > > + * Queues a device for a sync_state() callback when the device links write lock > > > + * isn't held. This allows the sync_state() execution flow to use device links > > > + * APIs. The caller must ensure this function is called with > > > + * device_links_write_lock() held. > > > + * > > > + * This function does a get_device() to make sure the device is not freed while > > > + * on this list. > > > + * > > > + * So the caller must also ensure that device_links_flush_sync_list() is called > > > + * as soon as the caller releases device_links_write_lock(). This is necessary > > > + * to make sure the sync_state() is called in a timely fashion and the > > > + * put_device() is called on this device. > > > + */ > > > +static void __device_links_queue_sync_state(struct device *dev) > > > > Pass a list head as a second arg. > > > > > { > > > struct device_link *link; > > > > > > @@ -709,12 +727,48 @@ static void __device_links_supplier_sync_state(struct device *dev) > > > return; > > > } > > > > > > - if (dev->bus->sync_state) > > > - dev->bus->sync_state(dev); > > > - else if (dev->driver && dev->driver->sync_state) > > > - dev->driver->sync_state(dev); > > > - > > > + /* > > > + * Set the flag here to avoid adding the same device to the sync_list > > > + * more than once. This can happen if new consumers get added to the > > > + * device before the sync_list is flushed. > > > + */ > > > dev->state_synced = true; > > > + > > > + mutex_lock(&sync_lock); > > > + > > > + if (list_empty(&dev->links.defer_sync)) { > > > + get_device(dev); > > > + list_add_tail(&dev->links.defer_sync, &sync_list); > > > > Add it to the list that you have passed as the second arg. No locking. > > > > > + } else { > > > + WARN_ON(1); > > > + } > > > + > > > + mutex_unlock(&sync_lock); > > > +} > > > + > > > +/** device_links_flush_sync_list - Call sync_state() on devices queued for it > > > + * > > > + * Calls sync_state() on all the devices that have been queued for it. This > > > + * function is used in conjunction with __device_links_queue_sync_state(). > > > + */ > > > +static void device_links_flush_sync_list(void) > > > > Make it take a list to flush as an arg. > > > > > +{ > > > + struct device *dev, *tmp; > > > + > > > + mutex_lock(&sync_lock); > > > > This isn't necessary. > > > > > + > > > + list_for_each_entry_safe(dev, tmp, &sync_list, links.defer_sync) { > > > + list_del_init(&dev->links.defer_sync); > > > + device_lock(dev); > > > > Empty lines around this? > > > > > + if (dev->bus->sync_state) > > > + dev->bus->sync_state(dev); > > > + else if (dev->driver && dev->driver->sync_state) > > > + dev->driver->sync_state(dev); > > > + device_unlock(dev); > > > > And this? > > > > > + put_device(dev); > > > + } > > > + > > > + mutex_unlock(&sync_lock); > > > > This isn't necessary. > > > > > } > > > > > > void device_links_supplier_sync_state_pause(void) > > > @@ -738,11 +792,16 @@ void device_links_supplier_sync_state_resume(void) > > > goto out; > > > > > > list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) { > > > - __device_links_supplier_sync_state(dev); > > > + /* > > > + * Delete from deferred_sync list before queuing it to > > > + * sync_list because defer_sync is used for both lists. > > > + */ > > > list_del_init(&dev->links.defer_sync); > > > + __device_links_queue_sync_state(dev); > > > > Use a local list (initially empty) in this function and pass it to the above. > > > > > } > > > out: > > > device_links_write_unlock(); > > > + device_links_flush_sync_list(); > > > > Pass the local list here too. > > > > > } > > > > > > static int sync_state_resume_initcall(void) > > > @@ -815,12 +874,13 @@ void device_links_driver_bound(struct device *dev) > > > if (defer_sync_state_count) > > > __device_links_supplier_defer_sync(link->supplier); > > > else > > > - __device_links_supplier_sync_state(link->supplier); > > > + __device_links_queue_sync_state(link->supplier); > > > > Like in the previous case, use a local list (initially empty) in this > > function and pass it to the above. > > > > > } > > > > > > dev->links.status = DL_DEV_DRIVER_BOUND; > > > > > > device_links_write_unlock(); > > > + device_links_flush_sync_list(); > > > > And pass that list here too. > > > > > } > > > > > > static void device_link_drop_managed(struct device_link *link) > > > -- > > Ok, I'll move it to a local list, add the blank lines and send out a patch?
Works for me. :-)
| |