Date:   Tue, 16 Jul 2019 11:29:17 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Saravana Kannan <saravanak@...gle.com>,
        Lukas Wunner <lukas@...ner.de>,
        Jon Hunter <jonathanh@...dia.com>,
        Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH] driver core: Remove device link creation limitation

Hi Rafael,

On 2019-07-10 12:19, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> If device_link_add() is called for a consumer/supplier pair with an
> existing device link between them and the existing link's type is
> not in agreement with the flags passed to that function by its
> caller, NULL will be returned.  That is seriously inconvenient,
> because it forces the callers of device_link_add() to worry about
> what others may or may not do even if that is not relevant to them
> for any other reasons.
>
> It turns out, however, that this limitation can be made go away
> relatively easily.
>
> The underlying observation is that if DL_FLAG_STATELESS has been
> passed to device_link_add() in flags for the given consumer/supplier
> pair at least once, calling either device_link_del() or
> device_link_remove() to release the link returned by it should work,
> but there are no other requirements associated with that flag.  In
> turn, if at least one of the callers of device_link_add() for the
> given consumer/supplier pair has not passed DL_FLAG_STATELESS to it
> in flags, the driver core should track the status of the link and act
> on it as appropriate (ie. the link should be treated as "managed").
> This means that DL_FLAG_STATELESS needs to be set for managed device
> links and it should be valid to call device_link_del() or
> device_link_remove() to drop references to them in certain
> sutiations.
>
> To allow that to happen, introduce a new (internal) device link flag
> called DL_FLAG_MANAGED and make device_link_add() set it automatically
> whenever DL_FLAG_STATELESS is not passed to it.  Also make it take
> additional references to existing device links that were previously
> stateless (that is, with DL_FLAG_STATELESS set and DL_FLAG_MANAGED
> unset) and will need to be managed going forward and initialize
> their status (which has been DL_STATE_NONE so far).
>
> Accordingly, when a managed device link is dropped automatically
> by the driver core, make it clear DL_FLAG_MANAGED, reset the link's
> status back to DL_STATE_NONE and drop the reference to it associated
> with DL_FLAG_MANAGED instead of just deleting it right away (to
> allow it to stay around in case it still needs to be released
> explicitly by someone).
>
> With that, since setting DL_FLAG_STATELESS doesn't mean that the
> device link in question is not managed any more, replace all of the
> status-tracking checks against DL_FLAG_STATELESS with analogous
> checks against DL_FLAG_MANAGED and update the documentation to
> reflect these changes.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>

Exynos IOMMU works fine with those changes.

> ---
>   Documentation/driver-api/device_link.rst |    4
>   drivers/base/core.c                      |  169 +++++++++++++++++--------------
>   drivers/base/power/runtime.c             |    4
>   include/linux/device.h                   |    4
>   4 files changed, 102 insertions(+), 79 deletions(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -124,6 +124,50 @@ static int device_is_dependent(struct de
>   	return ret;
>   }
>   
> +static void device_link_init_status(struct device_link *link,
> +				    struct device *consumer,
> +				    struct device *supplier)
> +{
> +	switch (supplier->links.status) {
> +	case DL_DEV_PROBING:
> +		switch (consumer->links.status) {
> +		case DL_DEV_PROBING:
> +			/*
> +			 * A consumer driver can create a link to a supplier
> +			 * that has not completed its probing yet as long as it
> +			 * knows that the supplier is already functional (for
> +			 * example, it has just acquired some resources from the
> +			 * supplier).
> +			 */
> +			link->status = DL_STATE_CONSUMER_PROBE;
> +			break;
> +		default:
> +			link->status = DL_STATE_DORMANT;
> +			break;
> +		}
> +		break;
> +	case DL_DEV_DRIVER_BOUND:
> +		switch (consumer->links.status) {
> +		case DL_DEV_PROBING:
> +			link->status = DL_STATE_CONSUMER_PROBE;
> +			break;
> +		case DL_DEV_DRIVER_BOUND:
> +			link->status = DL_STATE_ACTIVE;
> +			break;
> +		default:
> +			link->status = DL_STATE_AVAILABLE;
> +			break;
> +		}
> +		break;
> +	case DL_DEV_UNBINDING:
> +		link->status = DL_STATE_SUPPLIER_UNBIND;
> +		break;
> +	default:
> +		link->status = DL_STATE_DORMANT;
> +		break;
> +	}
> +}
> +
>   static int device_reorder_to_tail(struct device *dev, void *not_used)
>   {
>   	struct device_link *link;
> @@ -179,9 +223,9 @@ void device_pm_move_to_tail(struct devic
>    * of the link.  If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
>    * ignored.
>    *
> - * If DL_FLAG_STATELESS is set in @flags, the link is not going to be managed by
> - * the driver core and, in particular, the caller of this function is expected
> - * to drop the reference to the link acquired by it directly.
> + * If DL_FLAG_STATELESS is set in @flags, the caller of this function is
> + * expected to release the link returned by it directly with the help of either
> + * device_link_del() or device_link_remove().
>    *
>    * If that flag is not set, however, the caller of this function is handing the
>    * management of the link over to the driver core entirely and its return value
> @@ -201,9 +245,16 @@ void device_pm_move_to_tail(struct devic
>    * be used to request the driver core to automaticall probe for a consmer
>    * driver after successfully binding a driver to the supplier device.
>    *
> - * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
> - * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
> - * will cause NULL to be returned upfront.
> + * The combination of DL_FLAG_STATELESS and one of DL_FLAG_AUTOREMOVE_CONSUMER,
> + * DL_FLAG_AUTOREMOVE_SUPPLIER, or DL_FLAG_AUTOPROBE_CONSUMER set in @flags at
> + * the same time is invalid and will cause NULL to be returned upfront.
> + * However, if a device link between the given @consumer and @supplier pair
> + * exists already when this function is called for them, the existing link will
> + * be returned regardless of its current type and status (the link's flags may
> + * be modified then).  The caller of this function is then expected to treat
> + * the link as though it has just been created, so (in particular) if
> + * DL_FLAG_STATELESS was passed in @flags, the link needs to be released
> + * explicitly when not needed any more (as stated above).
>    *
>    * A side effect of the link creation is re-ordering of dpm_list and the
>    * devices_kset list by moving the consumer device and all devices depending
> @@ -223,7 +274,8 @@ struct device_link *device_link_add(stru
>   	    (flags & DL_FLAG_STATELESS &&
>   	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
>   		      DL_FLAG_AUTOREMOVE_SUPPLIER |
> -		      DL_FLAG_AUTOPROBE_CONSUMER)) ||
> +		      DL_FLAG_AUTOPROBE_CONSUMER |
> +		      DL_FLAG_MANAGED)) ||
>   	    (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
>   	     flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
>   		      DL_FLAG_AUTOREMOVE_SUPPLIER)))
> @@ -236,6 +288,9 @@ struct device_link *device_link_add(stru
>   		}
>   	}
>   
> +	if (!(flags & DL_FLAG_STATELESS))
> +		flags |= DL_FLAG_MANAGED;
> +
>   	device_links_write_lock();
>   	device_pm_lock();
>   
> @@ -262,15 +317,6 @@ struct device_link *device_link_add(stru
>   		if (link->consumer != consumer)
>   			continue;
>   
> -		/*
> -		 * Don't return a stateless link if the caller wants a stateful
> -		 * one and vice versa.
> -		 */
> -		if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
> -			link = NULL;
> -			goto out;
> -		}
> -
>   		if (flags & DL_FLAG_PM_RUNTIME) {
>   			if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
>   				pm_runtime_new_link(consumer);
> @@ -281,6 +327,7 @@ struct device_link *device_link_add(stru
>   		}
>   
>   		if (flags & DL_FLAG_STATELESS) {
> +			link->flags |= DL_FLAG_STATELESS;
>   			kref_get(&link->kref);
>   			goto out;
>   		}
> @@ -299,6 +346,11 @@ struct device_link *device_link_add(stru
>   			link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
>   					 DL_FLAG_AUTOREMOVE_SUPPLIER);
>   		}
> +		if (!(link->flags & DL_FLAG_MANAGED)) {
> +			kref_get(&link->kref);
> +			link->flags |= DL_FLAG_MANAGED;
> +			device_link_init_status(link, consumer, supplier);
> +		}
>   		goto out;
>   	}
>   
> @@ -325,48 +377,10 @@ struct device_link *device_link_add(stru
>   	kref_init(&link->kref);
>   
>   	/* Determine the initial link state. */
> -	if (flags & DL_FLAG_STATELESS) {
> +	if (flags & DL_FLAG_STATELESS)
>   		link->status = DL_STATE_NONE;
> -	} else {
> -		switch (supplier->links.status) {
> -		case DL_DEV_PROBING:
> -			switch (consumer->links.status) {
> -			case DL_DEV_PROBING:
> -				/*
> -				 * A consumer driver can create a link to a
> -				 * supplier that has not completed its probing
> -				 * yet as long as it knows that the supplier is
> -				 * already functional (for example, it has just
> -				 * acquired some resources from the supplier).
> -				 */
> -				link->status = DL_STATE_CONSUMER_PROBE;
> -				break;
> -			default:
> -				link->status = DL_STATE_DORMANT;
> -				break;
> -			}
> -			break;
> -		case DL_DEV_DRIVER_BOUND:
> -			switch (consumer->links.status) {
> -			case DL_DEV_PROBING:
> -				link->status = DL_STATE_CONSUMER_PROBE;
> -				break;
> -			case DL_DEV_DRIVER_BOUND:
> -				link->status = DL_STATE_ACTIVE;
> -				break;
> -			default:
> -				link->status = DL_STATE_AVAILABLE;
> -				break;
> -			}
> -			break;
> -		case DL_DEV_UNBINDING:
> -			link->status = DL_STATE_SUPPLIER_UNBIND;
> -			break;
> -		default:
> -			link->status = DL_STATE_DORMANT;
> -			break;
> -		}
> -	}
> +	else
> +		device_link_init_status(link, consumer, supplier);
>   
>   	/*
>   	 * Some callers expect the link creation during consumer driver probe to
> @@ -528,7 +542,7 @@ static void device_links_missing_supplie
>    * mark the link as "consumer probe in progress" to make the supplier removal
>    * wait for us to complete (or bad things may happen).
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   int device_links_check_suppliers(struct device *dev)
>   {
> @@ -538,7 +552,7 @@ int device_links_check_suppliers(struct
>   	device_links_write_lock();
>   
>   	list_for_each_entry(link, &dev->links.suppliers, c_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		if (link->status != DL_STATE_AVAILABLE) {
> @@ -563,7 +577,7 @@ int device_links_check_suppliers(struct
>    *
>    * Also change the status of @dev's links to suppliers to "active".
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   void device_links_driver_bound(struct device *dev)
>   {
> @@ -572,7 +586,7 @@ void device_links_driver_bound(struct de
>   	device_links_write_lock();
>   
>   	list_for_each_entry(link, &dev->links.consumers, s_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		/*
> @@ -593,7 +607,7 @@ void device_links_driver_bound(struct de
>   	}
>   
>   	list_for_each_entry(link, &dev->links.suppliers, c_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> @@ -605,6 +619,13 @@ void device_links_driver_bound(struct de
>   	device_links_write_unlock();
>   }
>   
> +static void device_link_drop_managed(struct device_link *link)
> +{
> +	link->flags &= ~DL_FLAG_MANAGED;
> +	WRITE_ONCE(link->status, DL_STATE_NONE);
> +	kref_put(&link->kref, __device_link_del);
> +}
> +
>   /**
>    * __device_links_no_driver - Update links of a device without a driver.
>    * @dev: Device without a drvier.
> @@ -615,18 +636,18 @@ void device_links_driver_bound(struct de
>    * unless they already are in the "supplier unbind in progress" state in which
>    * case they need not be updated.
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   static void __device_links_no_driver(struct device *dev)
>   {
>   	struct device_link *link, *ln;
>   
>   	list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
> -			__device_link_del(&link->kref);
> +			device_link_drop_managed(link);
>   		else if (link->status == DL_STATE_CONSUMER_PROBE ||
>   			 link->status == DL_STATE_ACTIVE)
>   			WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> @@ -643,7 +664,7 @@ static void __device_links_no_driver(str
>    * %__device_links_no_driver() to update links to suppliers for it as
>    * appropriate.
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   void device_links_no_driver(struct device *dev)
>   {
> @@ -652,7 +673,7 @@ void device_links_no_driver(struct devic
>   	device_links_write_lock();
>   
>   	list_for_each_entry(link, &dev->links.consumers, s_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		/*
> @@ -680,7 +701,7 @@ void device_links_no_driver(struct devic
>    * invoke %__device_links_no_driver() to update links to suppliers for it as
>    * appropriate.
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   void device_links_driver_cleanup(struct device *dev)
>   {
> @@ -689,7 +710,7 @@ void device_links_driver_cleanup(struct
>   	device_links_write_lock();
>   
>   	list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
> @@ -702,7 +723,7 @@ void device_links_driver_cleanup(struct
>   		 */
>   		if (link->status == DL_STATE_SUPPLIER_UNBIND &&
>   		    link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
> -			__device_link_del(&link->kref);
> +			device_link_drop_managed(link);
>   
>   		WRITE_ONCE(link->status, DL_STATE_DORMANT);
>   	}
> @@ -724,7 +745,7 @@ void device_links_driver_cleanup(struct
>    *
>    * Return 'false' if there are no probing or active consumers.
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   bool device_links_busy(struct device *dev)
>   {
> @@ -734,7 +755,7 @@ bool device_links_busy(struct device *de
>   	device_links_write_lock();
>   
>   	list_for_each_entry(link, &dev->links.consumers, s_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		if (link->status == DL_STATE_CONSUMER_PROBE
> @@ -764,7 +785,7 @@ bool device_links_busy(struct device *de
>    * driver to unbind and start over (the consumer will not re-probe as we have
>    * changed the state of the link already).
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    */
>   void device_links_unbind_consumers(struct device *dev)
>   {
> @@ -776,7 +797,7 @@ void device_links_unbind_consumers(struc
>   	list_for_each_entry(link, &dev->links.consumers, s_node) {
>   		enum device_link_state status;
>   
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		status = link->status;
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1624,7 +1624,7 @@ void pm_runtime_remove(struct device *de
>    * runtime PM references to the device, drop the usage counter of the device
>    * (as many times as needed).
>    *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
>    *
>    * Since the device is guaranteed to be runtime-active at the point this is
>    * called, nothing else needs to be done here.
> @@ -1641,7 +1641,7 @@ void pm_runtime_clean_up_links(struct de
>   	idx = device_links_read_lock();
>   
>   	list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> -		if (link->flags & DL_FLAG_STATELESS)
> +		if (!(link->flags & DL_FLAG_MANAGED))
>   			continue;
>   
>   		while (refcount_dec_not_one(&link->rpm_active))
> Index: linux-pm/include/linux/device.h
> ===================================================================
> --- linux-pm.orig/include/linux/device.h
> +++ linux-pm/include/linux/device.h
> @@ -829,12 +829,13 @@ enum device_link_state {
>   /*
>    * Device link flags.
>    *
> - * STATELESS: The core won't track the presence of supplier/consumer drivers.
> + * STATELESS: The core will not remove this link automatically.
>    * AUTOREMOVE_CONSUMER: Remove the link automatically on consumer driver unbind.
>    * PM_RUNTIME: If set, the runtime PM framework will use this link.
>    * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
>    * AUTOREMOVE_SUPPLIER: Remove the link automatically on supplier driver unbind.
>    * AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
> + * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
>    */
>   #define DL_FLAG_STATELESS		BIT(0)
>   #define DL_FLAG_AUTOREMOVE_CONSUMER	BIT(1)
> @@ -842,6 +843,7 @@ enum device_link_state {
>   #define DL_FLAG_RPM_ACTIVE		BIT(3)
>   #define DL_FLAG_AUTOREMOVE_SUPPLIER	BIT(4)
>   #define DL_FLAG_AUTOPROBE_CONSUMER	BIT(5)
> +#define DL_FLAG_MANAGED			BIT(6)
>   
>   /**
>    * struct device_link - Device link representation.
> Index: linux-pm/Documentation/driver-api/device_link.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/device_link.rst
> +++ linux-pm/Documentation/driver-api/device_link.rst
> @@ -78,8 +78,8 @@ typically deleted in its ``->remove`` ca
>   driver is compiled as a module, the device link is added on module load and
>   orderly deleted on unload.  The same restrictions that apply to device link
>   addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
> -to deletion.  Device links with ``DL_FLAG_STATELESS`` unset (i.e. managed
> -device links) are deleted automatically by the driver core.
> +to deletion.  Device links managed by the driver core are deleted automatically
> +by it.
>   
>   Several flags may be specified on device link addition, two of which
>   have already been mentioned above:  ``DL_FLAG_STATELESS`` to express that no
>
>
>
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Powered by blists - more mailing lists