From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:15
Hello,
This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using device links, which lets
SYSMMU controller's runtime PM to follow master's device runtime PM (the
device which actually performs DMA transaction). The main idea
behind this solution is an observation that any DMA activity from master
device can be done only when master device is active, thus when master
device is suspended SYSMMU controller device can also be suspended.
This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).
Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
between devices" patchset:
http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
I've included them here, because it is hard to find them all on mailing
list archives.
Patches 6-8 are fixes to device dependencies/links code, which were
required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
PM code expert, so please double check if my changes are really correct.
This patchset requires my previous changes to Exynos IOMMU driver
submitted in the "Exynos IOMMU: improve clock management" thread:
http://www.spinics.net/lists/arm-kernel/msg505695.html
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
Changelog:
v2:
- replaced PM notifiers with generic device dependencies/links developped
by Rafael J. Wysocki
v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version
Patch summary:
Marek Szyprowski (5):
driver core: Avoid endless recursion if device has more than one link
driver core: Add support for links to already probed drivers
PM core: Fix restoring devices with links during system PM transition
iommu/exynos: Remove excessive, useless debug
iommu/exynos: Add proper runtime pm support
Rafael J. Wysocki (5):
driver core: Add a wrapper around __device_release_driver()
driver core: Functional dependencies tracking support
PM core: Make async suspend/resume of devices use device links
PM core: Make runtime PM of devices use device links
PM core: Optimize the use of device links for runtime PM
drivers/base/base.h | 13 ++
drivers/base/core.c | 410 +++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 65 +++++--
drivers/base/power/main.c | 68 ++++++-
drivers/base/power/runtime.c | 130 +++++++++++++-
drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
include/linux/device.h | 41 +++++
include/linux/pm.h | 1 +
include/linux/pm_runtime.h | 6 +
9 files changed, 809 insertions(+), 146 deletions(-)
--
1.9.1
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:19
From: "Rafael J. Wysocki" <redacted>
Modify the runtime PM framework to use device links to ensure that
supplier devices will not be suspended if any of their consumer
devices are active.
The idea is to reference count suppliers on the consumer's resume
and drop references to them on its suspend. The information on
whether or not the supplier has been reference counted by the
consumer's (runtime) resume is stored in a new field (rpm_active)
in the link object for each link.
It may be necessary to clean up those references when the
supplier is unbinding and that's why the links whose status is
DEVICE_LINK_SUPPLIER_UNBIND are skipped by the runtime suspend
and resume code.
The above means that if the consumer device is probed in the
runtime-active state, the supplier has to be resumed and reference
counted by device_link_add() so the code works as expected on its
(runtime) suspend. There is a new flag, DEVICE_LINK_RPM_ACTIVE,
to tell device_link_add() about that (in which case the caller
is responsible for making sure that the consumer really will
be runtime-active when runtime PM is enabled for it).
The other new link flag, DEVICE_LINK_PM_RUNTIME, tells the core
whether or not the link should be used for runtime PM at all.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 15 +++++++
drivers/base/dd.c | 1 +
drivers/base/power/runtime.c | 93 +++++++++++++++++++++++++++++++++++++++++---
include/linux/device.h | 5 +++
include/linux/pm_runtime.h | 2 +
5 files changed, 111 insertions(+), 5 deletions(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:22
From: "Rafael J. Wysocki" <redacted>
If the device has no links to suppliers that should be used for
runtime PM (links with DEVICE_LINK_PM_RUNTIME set), there is no
reason to walk the list of suppliers for that device during
runtime suspend and resume.
Add a simple mechanism to detect that case and possibly avoid the
extra unnecessary overhead.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 6 ++++++
drivers/base/power/runtime.c | 23 ++++++++++++++++++++---
include/linux/pm.h | 1 +
include/linux/pm_runtime.h | 4 ++++
4 files changed, 31 insertions(+), 3 deletions(-)
@@ -161,6 +164,9 @@ static void devlink_del(struct devlink *link)dev_info(link->consumer,"Dropping the link to %s\n",dev_name(link->supplier));+if(link->flags&DEVICE_LINK_PM_RUNTIME)+pm_runtime_drop_link(link->consumer);+list_del_rcu(&link->s_node);list_del_rcu(&link->c_node);call_srcu(&device_links_srcu,&link->rcu_head,__devlink_free_srcu);
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:25
From: "Rafael J. Wysocki" <redacted>
Make the device suspend/resume part of the core system
suspend/resume code use device links to ensure that supplier
and consumer devices will be suspended and resumed in the right
order in case of async suspend/resume.
The idea, roughly, is to use dpm_wait() to wait for all consumers
before a supplier device suspend and to wait for all suppliers
before a consumer device resume.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/base.h | 2 ++
drivers/base/core.c | 4 +--
drivers/base/power/main.c | 68 ++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 66 insertions(+), 8 deletions(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:26
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
active its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/iommu/exynos-iommu.c | 218 ++++++++++++++++++++-----------------------
1 file changed, 99 insertions(+), 119 deletions(-)
@@ -233,8 +233,8 @@ struct sysmmu_drvdata {structclk*aclk;/* SYSMMU's aclk clock */structclk*pclk;/* SYSMMU's pclk clock */structclk*clk_master;/* master's device clock */-intactivations;/* number of calls to sysmmu_enable */spinlock_tlock;/* lock for modyfying state */+intactive;/* current status */structexynos_iommu_domain*domain;/* domain we belong to */structlist_headdomain_node;/* node for domain clients list */structlist_headowner_node;/* node for owner controllers list */
@@ -247,25 +247,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)returncontainer_of(dom,structexynos_iommu_domain,domain);}-staticboolset_sysmmu_active(structsysmmu_drvdata*data)-{-/* return true if the System MMU was not active previously-anditneedstobeinitialized*/-return++data->activations==1;-}--staticboolset_sysmmu_inactive(structsysmmu_drvdata*data)-{-/* return true if the System MMU is needed to be disabled */-BUG_ON(data->activations<1);-return--data->activations==0;-}--staticboolis_sysmmu_active(structsysmmu_drvdata*data)-{-returndata->activations>0;-}-staticvoidsysmmu_unblock(structsysmmu_drvdata*data){writel(CTRL_ENABLE,data->sfrbase+REG_MMU_CTRL);
@@ -440,32 +421,6 @@ static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)__sysmmu_disable_clocks(data);}-staticbool__sysmmu_disable(structsysmmu_drvdata*data)-{-booldisabled;-unsignedlongflags;--spin_lock_irqsave(&data->lock,flags);--disabled=set_sysmmu_inactive(data);--if(disabled){-data->pgtable=0;-data->domain=NULL;--__sysmmu_disable_nocount(data);--dev_dbg(data->sysmmu,"Disabled\n");-}else{-dev_dbg(data->sysmmu,"%d times left to disable\n",-data->activations);-}--spin_unlock_irqrestore(&data->lock,flags);--returndisabled;-}-staticvoid__sysmmu_init_config(structsysmmu_drvdata*data){unsignedintcfg;
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:27:53
Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/iommu/exynos-iommu.c | 3 ---
1 file changed, 3 deletions(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:28:28
This patch fixes endless recursion, which happends when device has
more than one link.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:28:51
From: "Rafael J. Wysocki" <redacted>
Add an internal wrapper around __device_release_driver() that will
acquire device locks and do the necessary checks before calling it.
The next patch will make use of it.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/dd.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:29:29
When devices are being runtime resumed during the system PM transition to
suspend state, the link suppliers might be already resumed and
have runtime pm disabled. This is normal case. This patch adds special
support for such case. Simple call to pm_runtime_get_syncreturns error
when device has runtime PM disabled, what results in incorrect runtime PM
state during system wide PM transition.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/power/runtime.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:29:31
Set proper link state if link is created between already probed supplier
device and to be probed consumer device.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-06-17 06:30:09
From: "Rafael J. Wysocki" <redacted>
Currently, there is a problem with handling cases where functional
dependencies between devices are involved.
What I mean by a "functional dependency" is when the driver of device
B needs both device A and its driver to be present and functional to
be able to work. This implies that the driver of A needs to be
working for B to be probed successfully and it cannot be unbound from
the device before the B's driver. This also has certain consequences
for power management of these devices (suspend/resume and runtime PM
ordering).
Add support for representing those functional dependencies between
devices to allow the driver core to track them and act on them in
certain cases where they matter.
The argument for doing that in the driver core is that there are
quite a few distinct use cases related to that, they are relatively
hard to get right in a driver (if one wants to address all of them
properly) and it only gets worse if multiplied by the number of
drivers potentially needing to do it. Morever, at least one case
(asynchronous system suspend/resume) cannot be handled in a single
driver at all, because it requires the driver of A to wait for B to
suspend (during system suspend) and the driver of B to wait for
A to resume (during system resume).
To that end, represent links between devices (or more precisely
between device+driver combos) as a struct devlink object containing
pointers to the devices in question, a list node for each of them,
status information, flags, a lock and an RCU head for synchronization.
Also add two new list heads, supplier_links and consumer_links, to
struct device to represent the lists of links to the devices that
depend on the given one (consumers) and to the devices depended on
by it (suppliers), respectively.
The entire data structure consisting of all of the lists of link
objects for all devices is protected by SRCU (for list walking)
and a by mutex (for link object addition/removal). In addition
to that, each link object has an internal status field whose
value reflects what's happening to the devices pointed to by
the link. That status field is protected by an internal spinlock.
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
Link objects are deleted either explicitly, by calling
device_link_del() on the link object in question, or automatically,
when the consumer device is unbound from its driver or when one
of the target devices is deleted, depending on the link type.
There are two types of link objects, persistent and non-persistent.
The difference between them is that the persistent links stay around
until one of the target devices is deleted, which the non-persistent
ones are deleted when the consumer driver is unbound from its device
(ie. they are assumed to be valid only as long as the consumer device
has a driver bound to it). The DEVICE_LINK_PERSISTENT flag has to
be passed to device_link_add() so as to create a persistent link.
One of the actions carried out by device_link_add() is to reorder
the lists used for device shutdown and system suspend/resume to
put the consumer device along with all of its children and all of
its consumers (and so on, recursively) to the ends of those list
in order to ensure the right ordering between the all of the supplier
and consumer devices.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/base.h | 11 ++
drivers/base/core.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 42 +++++-
include/linux/device.h | 36 +++++
4 files changed, 470 insertions(+), 5 deletions(-)
@@ -44,6 +44,367 @@ static int __init sysfs_deprecated_setup(char *arg)early_param("sysfs.deprecated",sysfs_deprecated_setup);#endif+/* Device links support. */++DEFINE_STATIC_SRCU(device_links_srcu);+staticDEFINE_MUTEX(device_links_lock);++staticintdevice_reorder_to_tail(structdevice*dev,void*not_used)+{+structdevlink*link;++devices_kset_move_last(dev);+device_pm_move_last(dev);+device_for_each_child(dev,NULL,device_reorder_to_tail);+list_for_each_entry(link,&dev->consumer_links,c_node)+device_reorder_to_tail(link->consumer,NULL);++return0;+}++/**+*device_link_add-Createalinkbetweentwodevices.+*@consumer:Consumerendofthelink.+*@supplier:Supplierendofthelink.+*@flags:Linkflags.+*+*Atleastoneoftheflagsmustbeset.IfDEVICE_LINK_PROBE_TIMEisset,the+*callerisexpectedtoknowthat(a)thesupplierdeviceispresentandactive+*(ie.itsdriverisfunctional)and(b)theconsumerdeviceisprobingatthe+*momentandthereforetheinitialstateofthelinkwillbe"consumer probe"+*inthatcase.IfDEVICE_LINK_PROBE_TIMEisnotset,DEVICE_LINK_PERSISTENT+*mustbeset(meaningthatthelinkwillnotgoawaywhentheconsumerdriver+*goesaway).+*+*Asideeffectofthelinkcreationisre-orderingofdpm_listandthe+*devices_ksetlistbymovingtheconsumerdeviceandalldevicesdepending+*onittotheendsofthoselists.+*/+structdevlink*device_link_add(structdevice*consumer,+structdevice*supplier,u32flags)+{+structdevlink*link;++if(!consumer||!supplier||!flags)+returnNULL;++mutex_lock(&device_links_lock);++list_for_each_entry(link,&supplier->supplier_links,s_node)+if(link->consumer==consumer)+gotoout;++link=kmalloc(sizeof(*link),GFP_KERNEL);+if(!link)+gotoout;++get_device(supplier);+link->supplier=supplier;+INIT_LIST_HEAD(&link->s_node);+get_device(consumer);+link->consumer=consumer;+INIT_LIST_HEAD(&link->c_node);+link->flags=flags;+link->status=(flags&DEVICE_LINK_PROBE_TIME)?+DEVICE_LINK_CONSUMER_PROBE:DEVICE_LINK_DORMANT;+spin_lock_init(&link->lock);++/*+*Movetheconsumerandallofthedevicesdependingonittotheend+*ofdpm_listandthedevices_ksetlist.+*+*Wehavetoholddpm_listlockedthroughoutallthatorelsewemay+*endupsuspendingwithawrongorderingofit.+*/+device_pm_lock();+device_reorder_to_tail(consumer,NULL);+device_pm_unlock();++list_add_tail_rcu(&link->s_node,&supplier->supplier_links);+list_add_tail_rcu(&link->c_node,&consumer->consumer_links);++dev_info(consumer,"Linked as a consumer to %s\n",dev_name(supplier));++out:+mutex_unlock(&device_links_lock);+returnlink;+}+EXPORT_SYMBOL_GPL(device_link_add);++staticvoid__devlink_free_srcu(structrcu_head*rhead)+{+structdevlink*link;++link=container_of(rhead,structdevlink,rcu_head);+put_device(link->consumer);+put_device(link->supplier);+kfree(link);+}++staticvoiddevlink_del(structdevlink*link)+{+dev_info(link->consumer,"Dropping the link to %s\n",+dev_name(link->supplier));++list_del_rcu(&link->s_node);+list_del_rcu(&link->c_node);+call_srcu(&device_links_srcu,&link->rcu_head,__devlink_free_srcu);+}++/**+*device_link_del-Deletealinkbetweentwodevices.+*@link:Devicelinktodelete.+*/+voiddevice_link_del(structdevlink*link)+{+mutex_lock(&device_links_lock);+devlink_del(link);+mutex_unlock(&device_links_lock);+}+EXPORT_SYMBOL_GPL(device_link_del);++staticintdevice_links_read_lock(void)+{+returnsrcu_read_lock(&device_links_srcu);+}++staticvoiddevice_links_read_unlock(intidx)+{+returnsrcu_read_unlock(&device_links_srcu,idx);+}++staticvoiddevice_links_missing_supplier(structdevice*dev)+{+structdevlink*link;++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);++if(link->status==DEVICE_LINK_CONSUMER_PROBE)+link->status=DEVICE_LINK_AVAILABLE;++spin_unlock(&link->lock);+}+}++/**+*device_links_check_suppliers-Checksupplierdevicesforthisone.+*@dev:Consumerdevice.+*+*Checklinksfromthisdevicetoanysuppliers.Walkthelistofthedevice's+*consumerlinksandseeifallofthesuppliersareavailable.Ifnot,simply+*return-EPROBE_DEFER.+*+*WalkthelistunderSRCUandcheckeachlink'sstatusfieldunderitslock.+*+*Weneedtoguaranteethatthesupplierwillnotgoawayafterthecheckhas+*beenpositivehere.Itonlycangoawayin__device_release_driver()and+*thatfunctionchecksthedevice'slinkstoconsumers.Thismeansweneedto+*markthelinkas"consumer probe in progress"tomakethesupplierremoval+*waitforustocomplete(orbadthingsmayhappen).+*/+intdevice_links_check_suppliers(structdevice*dev)+{+structdevlink*link;+intidx,ret=0;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);+if(link->status!=DEVICE_LINK_AVAILABLE){+spin_unlock(&link->lock);+device_links_missing_supplier(dev);+ret=-EPROBE_DEFER;+break;+}+link->status=DEVICE_LINK_CONSUMER_PROBE;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+returnret;+}++/**+*device_links_driver_bound-Updatedevicelinksafterprobingitsdriver.+*@dev:Devicetoupdatethelinksfor.+*+*Theprobehasbeensuccessful,soupdatelinksfromthisdevicetoany+*consumersbychangingtheirstatusto"available".+*+*Alsochangethestatusof@dev'slinkstosuppliersto"active".+*/+voiddevice_links_driver_bound(structdevice*dev)+{+structdevlink*link;+intidx;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_DORMANT);+link->status=DEVICE_LINK_AVAILABLE;+spin_unlock(&link->lock);+}++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_CONSUMER_PROBE);+link->status=DEVICE_LINK_ACTIVE;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/**+*device_links_driver_gone-Updatelinksafterdriverremoval.+*@dev:Devicewhosedriverhasgoneaway.+*+*Updatelinkstoconsumersfor@devbychangingtheirstatusto"dormant".+*/+voiddevice_links_driver_gone(structdevice*dev)+{+structdevlink*link;+intidx;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+WARN_ON(!(link->flags&DEVICE_LINK_PERSISTENT));+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_SUPPLIER_UNBIND);+link->status=DEVICE_LINK_DORMANT;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/**+*device_links_no_driver-Updatelinksofadevicewithoutadriver.+*@dev:Devicewithoutadrvier.+*+*Deleteallnon-persistentlinksfromthisdevicetoanysuppliers.+*Persistentlinksstayaround,buttheirstatusischangedto"available",+*unlesstheyalreadyareinthe"supplier unbind in progress"stateinwhich+*casetheyneednotbeupdated.+*/+voiddevice_links_no_driver(structdevice*dev)+{+structdevlink*link,*ln;++mutex_lock(&device_links_lock);++list_for_each_entry_safe_reverse(link,ln,&dev->consumer_links,c_node)+if(link->flags&DEVICE_LINK_PERSISTENT){+spin_lock(&link->lock);++if(link->status!=DEVICE_LINK_SUPPLIER_UNBIND)+link->status=DEVICE_LINK_AVAILABLE;++spin_unlock(&link->lock);+}else{+devlink_del(link);+}++mutex_unlock(&device_links_lock);+}++/**+*device_links_busy-Checkifthereareanybusylinkstoconsumers.+*@dev:Devicetocheck.+*+*Checkeachconsumerofthedeviceandreturn'true'itifitslink'sstatus+*isoneof"consumer probe"or"active"(meaningthatthegivenconsumeris+*probingrightnoworitsdriverispresent).Otherwise,changethelink+*stateto"supplier unbind"topreventtheconsumerfrombeingprobed+*successfullygoingforward.+*+*Return'false'iftherearenoprobingoractiveconsumers.+*/+booldevice_links_busy(structdevice*dev)+{+structdevlink*link;+intidx;+boolret=false;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+spin_lock(&link->lock);+if(link->status==DEVICE_LINK_CONSUMER_PROBE+||link->status==DEVICE_LINK_ACTIVE){+spin_unlock(&link->lock);+ret=true;+break;+}+link->status=DEVICE_LINK_SUPPLIER_UNBIND;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+returnret;+}++/**+*device_links_unbind_consumers-Forceunbindconsumersofthegivendevice.+*@dev:Devicetounbindtheconsumersof.+*+*Walkthelistoflinkstoconsumersfor@devandifanyofthemisinthe+*"consumer probe"state,waitforalldeviceprobesinprogresstocomplete+*andstartover.+*+*Ifthat'snotthecase,changethestatusofthelinkto"supplier unbind"+*andcheckifthelinkwasinthe"active"state.Ifso,forcetheconsumer+*drivertounbindandstartover(theconsumerwillnotre-probeaswehave+*changedthestateofthelinkalready).+*/+voiddevice_links_unbind_consumers(structdevice*dev)+{+structdevlink*link;+intidx;++start:+idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+enumdevlink_statusstatus;++spin_lock(&link->lock);+status=link->status;+if(status==DEVICE_LINK_CONSUMER_PROBE){+spin_unlock(&link->lock);++device_links_read_unlock(idx);++wait_for_device_probe();+gotostart;+}+link->status=DEVICE_LINK_SUPPLIER_UNBIND;+if(status==DEVICE_LINK_ACTIVE){+structdevice*consumer=link->consumer;++get_device(consumer);+spin_unlock(&link->lock);++device_links_read_unlock(idx);++device_release_driver_internal(consumer,NULL,+consumer->parent);+put_device(consumer);+gotostart;+}+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/* Device links support end. */+int(*platform_notify)(structdevice*dev)=NULL;int(*platform_notify_remove)(structdevice*dev)=NULL;staticstructkobject*dev_kobj;
@@ -706,6 +706,34 @@ struct device_dma_parameters {unsignedlongsegment_boundary_mask;};+enumdevlink_status{+DEVICE_LINK_DORMANT=0,/* Link not in use. */+DEVICE_LINK_AVAILABLE,/* Supplier driver is present. */+DEVICE_LINK_ACTIVE,/* Consumer driver is present too. */+DEVICE_LINK_CONSUMER_PROBE,/* Consumer is probing. */+DEVICE_LINK_SUPPLIER_UNBIND,/* Supplier is unbinding. */+};++/*+*Devicelinkflags.+*+*PERSISTENT:Donotdeletethelinkonconsumerdevicedriverunbind.+*PROBE_TIME:Assumesupplierdevicefunctionalwhencreatingthelink.+*/+#define DEVICE_LINK_PERSISTENT (1 << 0)+#define DEVICE_LINK_PROBE_TIME (1 << 1)++structdevlink{+structdevice*supplier;+structlist_heads_node;+structdevice*consumer;+structlist_headc_node;+enumdevlink_statusstatus;+u32flags;+spinlock_tlock;+structrcu_headrcu_head;+};+/***structdevice-Thebasicdevicestructure*@parent:Thedevice's"parent"device,thedevicetowhichitisattached.
@@ -731,6 +759,8 @@ struct device_dma_parameters {*on.Thisshrinksthe"Board Support Packages"(BSPs)and*minimizesboard-specific#ifdefsindrivers.*@driver_data:Privatepointerfordriverspecificinfo.+*@supplier_links:Linkstoconsumerdevices.+*@consumer_links:Linkstosupplierdevices.*@power:Fordevicepowermanagement.*SeeDocumentation/power/devices.txtfordetails.*@pm_domain:Providecallbacksthatareexecutedduringsystemsuspend,
@@ -797,6 +827,8 @@ struct device {coredoesn'ttouchit*/void*driver_data;/* Driver data, set and get withdev_set/get_drvdata*/+structlist_headsupplier_links;+structlist_headconsumer_links;structdev_pm_infopower;structdev_pm_domain*pm_domain;
Hi Marek,
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
From: "Rafael J. Wysocki" <redacted>
Currently, there is a problem with handling cases where functional
dependencies between devices are involved.
What I mean by a "functional dependency" is when the driver of device
B needs both device A and its driver to be present and functional to
be able to work. This implies that the driver of A needs to be
working for B to be probed successfully and it cannot be unbound from
the device before the B's driver. This also has certain consequences
for power management of these devices (suspend/resume and runtime PM
ordering).
Add support for representing those functional dependencies between
devices to allow the driver core to track them and act on them in
certain cases where they matter.
Rafael has indicated that he intends to respin this series:
https://lkml.org/lkml/2016/6/8/1061
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
Thanks,
Lukas
quoted hunk
Link objects are deleted either explicitly, by calling
device_link_del() on the link object in question, or automatically,
when the consumer device is unbound from its driver or when one
of the target devices is deleted, depending on the link type.
There are two types of link objects, persistent and non-persistent.
The difference between them is that the persistent links stay around
until one of the target devices is deleted, which the non-persistent
ones are deleted when the consumer driver is unbound from its device
(ie. they are assumed to be valid only as long as the consumer device
has a driver bound to it). The DEVICE_LINK_PERSISTENT flag has to
be passed to device_link_add() so as to create a persistent link.
One of the actions carried out by device_link_add() is to reorder
the lists used for device shutdown and system suspend/resume to
put the consumer device along with all of its children and all of
its consumers (and so on, recursively) to the ends of those list
in order to ensure the right ordering between the all of the supplier
and consumer devices.
Signed-off-by: Rafael J. Wysocki <redacted>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/base.h | 11 ++
drivers/base/core.c | 386 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 42 +++++-
include/linux/device.h | 36 +++++
4 files changed, 470 insertions(+), 5 deletions(-)
@@ -44,6 +44,367 @@ static int __init sysfs_deprecated_setup(char *arg)early_param("sysfs.deprecated",sysfs_deprecated_setup);#endif+/* Device links support. */++DEFINE_STATIC_SRCU(device_links_srcu);+staticDEFINE_MUTEX(device_links_lock);++staticintdevice_reorder_to_tail(structdevice*dev,void*not_used)+{+structdevlink*link;++devices_kset_move_last(dev);+device_pm_move_last(dev);+device_for_each_child(dev,NULL,device_reorder_to_tail);+list_for_each_entry(link,&dev->consumer_links,c_node)+device_reorder_to_tail(link->consumer,NULL);++return0;+}++/**+*device_link_add-Createalinkbetweentwodevices.+*@consumer:Consumerendofthelink.+*@supplier:Supplierendofthelink.+*@flags:Linkflags.+*+*Atleastoneoftheflagsmustbeset.IfDEVICE_LINK_PROBE_TIMEisset,the+*callerisexpectedtoknowthat(a)thesupplierdeviceispresentandactive+*(ie.itsdriverisfunctional)and(b)theconsumerdeviceisprobingatthe+*momentandthereforetheinitialstateofthelinkwillbe"consumer probe"+*inthatcase.IfDEVICE_LINK_PROBE_TIMEisnotset,DEVICE_LINK_PERSISTENT+*mustbeset(meaningthatthelinkwillnotgoawaywhentheconsumerdriver+*goesaway).+*+*Asideeffectofthelinkcreationisre-orderingofdpm_listandthe+*devices_ksetlistbymovingtheconsumerdeviceandalldevicesdepending+*onittotheendsofthoselists.+*/+structdevlink*device_link_add(structdevice*consumer,+structdevice*supplier,u32flags)+{+structdevlink*link;++if(!consumer||!supplier||!flags)+returnNULL;++mutex_lock(&device_links_lock);++list_for_each_entry(link,&supplier->supplier_links,s_node)+if(link->consumer==consumer)+gotoout;++link=kmalloc(sizeof(*link),GFP_KERNEL);+if(!link)+gotoout;++get_device(supplier);+link->supplier=supplier;+INIT_LIST_HEAD(&link->s_node);+get_device(consumer);+link->consumer=consumer;+INIT_LIST_HEAD(&link->c_node);+link->flags=flags;+link->status=(flags&DEVICE_LINK_PROBE_TIME)?+DEVICE_LINK_CONSUMER_PROBE:DEVICE_LINK_DORMANT;+spin_lock_init(&link->lock);++/*+*Movetheconsumerandallofthedevicesdependingonittotheend+*ofdpm_listandthedevices_ksetlist.+*+*Wehavetoholddpm_listlockedthroughoutallthatorelsewemay+*endupsuspendingwithawrongorderingofit.+*/+device_pm_lock();+device_reorder_to_tail(consumer,NULL);+device_pm_unlock();++list_add_tail_rcu(&link->s_node,&supplier->supplier_links);+list_add_tail_rcu(&link->c_node,&consumer->consumer_links);++dev_info(consumer,"Linked as a consumer to %s\n",dev_name(supplier));++out:+mutex_unlock(&device_links_lock);+returnlink;+}+EXPORT_SYMBOL_GPL(device_link_add);++staticvoid__devlink_free_srcu(structrcu_head*rhead)+{+structdevlink*link;++link=container_of(rhead,structdevlink,rcu_head);+put_device(link->consumer);+put_device(link->supplier);+kfree(link);+}++staticvoiddevlink_del(structdevlink*link)+{+dev_info(link->consumer,"Dropping the link to %s\n",+dev_name(link->supplier));++list_del_rcu(&link->s_node);+list_del_rcu(&link->c_node);+call_srcu(&device_links_srcu,&link->rcu_head,__devlink_free_srcu);+}++/**+*device_link_del-Deletealinkbetweentwodevices.+*@link:Devicelinktodelete.+*/+voiddevice_link_del(structdevlink*link)+{+mutex_lock(&device_links_lock);+devlink_del(link);+mutex_unlock(&device_links_lock);+}+EXPORT_SYMBOL_GPL(device_link_del);++staticintdevice_links_read_lock(void)+{+returnsrcu_read_lock(&device_links_srcu);+}++staticvoiddevice_links_read_unlock(intidx)+{+returnsrcu_read_unlock(&device_links_srcu,idx);+}++staticvoiddevice_links_missing_supplier(structdevice*dev)+{+structdevlink*link;++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);++if(link->status==DEVICE_LINK_CONSUMER_PROBE)+link->status=DEVICE_LINK_AVAILABLE;++spin_unlock(&link->lock);+}+}++/**+*device_links_check_suppliers-Checksupplierdevicesforthisone.+*@dev:Consumerdevice.+*+*Checklinksfromthisdevicetoanysuppliers.Walkthelistofthedevice's+*consumerlinksandseeifallofthesuppliersareavailable.Ifnot,simply+*return-EPROBE_DEFER.+*+*WalkthelistunderSRCUandcheckeachlink'sstatusfieldunderitslock.+*+*Weneedtoguaranteethatthesupplierwillnotgoawayafterthecheckhas+*beenpositivehere.Itonlycangoawayin__device_release_driver()and+*thatfunctionchecksthedevice'slinkstoconsumers.Thismeansweneedto+*markthelinkas"consumer probe in progress"tomakethesupplierremoval+*waitforustocomplete(orbadthingsmayhappen).+*/+intdevice_links_check_suppliers(structdevice*dev)+{+structdevlink*link;+intidx,ret=0;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);+if(link->status!=DEVICE_LINK_AVAILABLE){+spin_unlock(&link->lock);+device_links_missing_supplier(dev);+ret=-EPROBE_DEFER;+break;+}+link->status=DEVICE_LINK_CONSUMER_PROBE;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+returnret;+}++/**+*device_links_driver_bound-Updatedevicelinksafterprobingitsdriver.+*@dev:Devicetoupdatethelinksfor.+*+*Theprobehasbeensuccessful,soupdatelinksfromthisdevicetoany+*consumersbychangingtheirstatusto"available".+*+*Alsochangethestatusof@dev'slinkstosuppliersto"active".+*/+voiddevice_links_driver_bound(structdevice*dev)+{+structdevlink*link;+intidx;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_DORMANT);+link->status=DEVICE_LINK_AVAILABLE;+spin_unlock(&link->lock);+}++list_for_each_entry_rcu(link,&dev->consumer_links,c_node){+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_CONSUMER_PROBE);+link->status=DEVICE_LINK_ACTIVE;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/**+*device_links_driver_gone-Updatelinksafterdriverremoval.+*@dev:Devicewhosedriverhasgoneaway.+*+*Updatelinkstoconsumersfor@devbychangingtheirstatusto"dormant".+*/+voiddevice_links_driver_gone(structdevice*dev)+{+structdevlink*link;+intidx;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+WARN_ON(!(link->flags&DEVICE_LINK_PERSISTENT));+spin_lock(&link->lock);+WARN_ON(link->status!=DEVICE_LINK_SUPPLIER_UNBIND);+link->status=DEVICE_LINK_DORMANT;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/**+*device_links_no_driver-Updatelinksofadevicewithoutadriver.+*@dev:Devicewithoutadrvier.+*+*Deleteallnon-persistentlinksfromthisdevicetoanysuppliers.+*Persistentlinksstayaround,buttheirstatusischangedto"available",+*unlesstheyalreadyareinthe"supplier unbind in progress"stateinwhich+*casetheyneednotbeupdated.+*/+voiddevice_links_no_driver(structdevice*dev)+{+structdevlink*link,*ln;++mutex_lock(&device_links_lock);++list_for_each_entry_safe_reverse(link,ln,&dev->consumer_links,c_node)+if(link->flags&DEVICE_LINK_PERSISTENT){+spin_lock(&link->lock);++if(link->status!=DEVICE_LINK_SUPPLIER_UNBIND)+link->status=DEVICE_LINK_AVAILABLE;++spin_unlock(&link->lock);+}else{+devlink_del(link);+}++mutex_unlock(&device_links_lock);+}++/**+*device_links_busy-Checkifthereareanybusylinkstoconsumers.+*@dev:Devicetocheck.+*+*Checkeachconsumerofthedeviceandreturn'true'itifitslink'sstatus+*isoneof"consumer probe"or"active"(meaningthatthegivenconsumeris+*probingrightnoworitsdriverispresent).Otherwise,changethelink+*stateto"supplier unbind"topreventtheconsumerfrombeingprobed+*successfullygoingforward.+*+*Return'false'iftherearenoprobingoractiveconsumers.+*/+booldevice_links_busy(structdevice*dev)+{+structdevlink*link;+intidx;+boolret=false;++idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+spin_lock(&link->lock);+if(link->status==DEVICE_LINK_CONSUMER_PROBE+||link->status==DEVICE_LINK_ACTIVE){+spin_unlock(&link->lock);+ret=true;+break;+}+link->status=DEVICE_LINK_SUPPLIER_UNBIND;+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+returnret;+}++/**+*device_links_unbind_consumers-Forceunbindconsumersofthegivendevice.+*@dev:Devicetounbindtheconsumersof.+*+*Walkthelistoflinkstoconsumersfor@devandifanyofthemisinthe+*"consumer probe"state,waitforalldeviceprobesinprogresstocomplete+*andstartover.+*+*Ifthat'snotthecase,changethestatusofthelinkto"supplier unbind"+*andcheckifthelinkwasinthe"active"state.Ifso,forcetheconsumer+*drivertounbindandstartover(theconsumerwillnotre-probeaswehave+*changedthestateofthelinkalready).+*/+voiddevice_links_unbind_consumers(structdevice*dev)+{+structdevlink*link;+intidx;++start:+idx=device_links_read_lock();++list_for_each_entry_rcu(link,&dev->supplier_links,s_node){+enumdevlink_statusstatus;++spin_lock(&link->lock);+status=link->status;+if(status==DEVICE_LINK_CONSUMER_PROBE){+spin_unlock(&link->lock);++device_links_read_unlock(idx);++wait_for_device_probe();+gotostart;+}+link->status=DEVICE_LINK_SUPPLIER_UNBIND;+if(status==DEVICE_LINK_ACTIVE){+structdevice*consumer=link->consumer;++get_device(consumer);+spin_unlock(&link->lock);++device_links_read_unlock(idx);++device_release_driver_internal(consumer,NULL,+consumer->parent);+put_device(consumer);+gotostart;+}+spin_unlock(&link->lock);+}++device_links_read_unlock(idx);+}++/* Device links support end. */+int(*platform_notify)(structdevice*dev)=NULL;int(*platform_notify_remove)(structdevice*dev)=NULL;staticstructkobject*dev_kobj;
@@ -706,6 +706,34 @@ struct device_dma_parameters {unsignedlongsegment_boundary_mask;};+enumdevlink_status{+DEVICE_LINK_DORMANT=0,/* Link not in use. */+DEVICE_LINK_AVAILABLE,/* Supplier driver is present. */+DEVICE_LINK_ACTIVE,/* Consumer driver is present too. */+DEVICE_LINK_CONSUMER_PROBE,/* Consumer is probing. */+DEVICE_LINK_SUPPLIER_UNBIND,/* Supplier is unbinding. */+};++/*+*Devicelinkflags.+*+*PERSISTENT:Donotdeletethelinkonconsumerdevicedriverunbind.+*PROBE_TIME:Assumesupplierdevicefunctionalwhencreatingthelink.+*/+#define DEVICE_LINK_PERSISTENT (1 << 0)+#define DEVICE_LINK_PROBE_TIME (1 << 1)++structdevlink{+structdevice*supplier;+structlist_heads_node;+structdevice*consumer;+structlist_headc_node;+enumdevlink_statusstatus;+u32flags;+spinlock_tlock;+structrcu_headrcu_head;+};+/***structdevice-Thebasicdevicestructure*@parent:Thedevice's"parent"device,thedevicetowhichitisattached.
@@ -731,6 +759,8 @@ struct device_dma_parameters {*on.Thisshrinksthe"Board Support Packages"(BSPs)and*minimizesboard-specific#ifdefsindrivers.*@driver_data:Privatepointerfordriverspecificinfo.+*@supplier_links:Linkstoconsumerdevices.+*@consumer_links:Linkstosupplierdevices.*@power:Fordevicepowermanagement.*SeeDocumentation/power/devices.txtfordetails.*@pm_domain:Providecallbacksthatareexecutedduringsystemsuspend,
@@ -797,6 +827,8 @@ struct device {coredoesn'ttouchit*/void*driver_data;/* Driver data, set and get withdev_set/get_drvdata*/+structlist_headsupplier_links;+structlist_headconsumer_links;structdev_pm_infopower;structdev_pm_domain*pm_domain;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2016-06-17 12:55:06
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
Hi Marek,
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
Currently, there is a problem with handling cases where functional
dependencies between devices are involved.
What I mean by a "functional dependency" is when the driver of device
B needs both device A and its driver to be present and functional to
be able to work. This implies that the driver of A needs to be
working for B to be probed successfully and it cannot be unbound from
the device before the B's driver. This also has certain consequences
for power management of these devices (suspend/resume and runtime PM
ordering).
Add support for representing those functional dependencies between
devices to allow the driver core to track them and act on them in
certain cases where they matter.
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
Thanks,
Rafael
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier. The PCIe hotplug ports of Thunderbolt
host controllers are the consumers.
If thunderbolt.ko is not loaded, no special precautions are needed for
the hotplug ports.
However *if* it is loaded and the system is suspended and there are
Thunderbolt devices plugged in, then on resume the hotplug ports must
wait for thunderbolt.ko to reestablish the PCI tunnels. If they do not
wait, things will go south because when pci_pm_resume_noirq() is called
for the devices *below* the hotplug port, we will try to put them into D0
(via pci_pm_default_resume_early()) and call pci_restore_state() even
though those hotplugged devices are not yet reachable.
So what I'd like to do is call device_link_add() when thunderbolt.ko
is loaded to shove a dependency onto the hotplug ports. But the hotplug
ports will long since have been bound at that point (to portdrv).
So it should be allowed to call device_link_add() ex post facto, after
the consumer has been bound.
Let me know if I still failed to convey the problem in an intelligible
way.
I had a similar problem with the Runtime PM for Thunderbolt series
(which BTW is still awaiting reviews):
http://www.spinics.net/lists/linux-pci/msg51158.html
The PM core allows calls to dev_pm_domain_set() only during ->probe or
if the device is unbound. Same constraint as with device_link_add().
This constraint was a major obstacle for me and I had to jump through
numerous additional hoops to work around it:
Thunderbolt controllers on Macs can be put into D3cold, but not with
standard ACPI methods, so platform_pci_power_manageable() is false for
these devices. Now this nothing unusual, the same applies to dual GPU
laptops (custom ACPI DSMs for Nvidia Optimus and AMD PowerXpress).
For such discrete GPUs, a struct dev_pm_domain is set during ->probe
(drivers/gpu/vga/vga_switcheroo.c: vga_switcheroo_init_domain_pm_ops()).
But I can't do that for Thunderbolt because the device in question is
a PCIe upstream port. Ideally I would shove a dev_pm_domain on the
upstream port when thunderbolt.ko loads, but again at that point
the port has long since been bound (to portdrv) so I can't call
dev_pm_domain_set().
The workaround I settled on is to attach to the upstream port as a
service driver, call down to the service driver's ->runtime_suspend
hooks when the port runtime suspends and power the controller down.
In addition, I had to make sure that saved_state isn't clobbered on
resume (patch [09/13]).
It would be good if someone who has a bird's eye view of the PM core
(i.e., you) could make a decision
(1) if it's possible and worthwhile to allow dev_pm_domain_set() for
already bound devices, and
(2) if the PCI core should be be able to deal with devices which are
runtime suspended to D3cold but not by the platform (because that's
what patch [09/13] does).
If the answer to (2) is yes then there's some more stuff to fix:
Discrete GPUs on dual GPU laptops can be manually suspended to D3cold
behind the PM core's back by loading nouveau / radeon / amdgpu with
runpm=0 and writing OFF to the vga_switcheroo debugfs interface.
This is currently broken in conjunction with system sleep (the GPU
is no longer accessible after resume) because pci_pm_resume_noirq()
calls pci_restore_state() even though the device is in D3cold and not
power-manageable by the platform.
Thanks,
Lukas
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.
Going to check where exactly things break.
With best wishes,
Tobias
Marek Szyprowski wrote:
Hello,
This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using device links, which lets
SYSMMU controller's runtime PM to follow master's device runtime PM (the
device which actually performs DMA transaction). The main idea
behind this solution is an observation that any DMA activity from master
device can be done only when master device is active, thus when master
device is suspended SYSMMU controller device can also be suspended.
This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).
Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
between devices" patchset:
http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
I've included them here, because it is hard to find them all on mailing
list archives.
Patches 6-8 are fixes to device dependencies/links code, which were
required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
PM code expert, so please double check if my changes are really correct.
This patchset requires my previous changes to Exynos IOMMU driver
submitted in the "Exynos IOMMU: improve clock management" thread:
http://www.spinics.net/lists/arm-kernel/msg505695.html
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
Changelog:
v2:
- replaced PM notifiers with generic device dependencies/links developped
by Rafael J. Wysocki
v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version
Patch summary:
Marek Szyprowski (5):
driver core: Avoid endless recursion if device has more than one link
driver core: Add support for links to already probed drivers
PM core: Fix restoring devices with links during system PM transition
iommu/exynos: Remove excessive, useless debug
iommu/exynos: Add proper runtime pm support
Rafael J. Wysocki (5):
driver core: Add a wrapper around __device_release_driver()
driver core: Functional dependencies tracking support
PM core: Make async suspend/resume of devices use device links
PM core: Make runtime PM of devices use device links
PM core: Optimize the use of device links for runtime PM
drivers/base/base.h | 13 ++
drivers/base/core.c | 410 +++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 65 +++++--
drivers/base/power/main.c | 68 ++++++-
drivers/base/power/runtime.c | 130 +++++++++++++-
drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
include/linux/device.h | 41 +++++
include/linux/pm.h | 1 +
include/linux/pm_runtime.h | 6 +
9 files changed, 809 insertions(+), 146 deletions(-)
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the reason?
With best wishes,
Tobias
With best wishes,
Tobias
Marek Szyprowski wrote:
quoted
Hello,
This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using device links, which lets
SYSMMU controller's runtime PM to follow master's device runtime PM (the
device which actually performs DMA transaction). The main idea
behind this solution is an observation that any DMA activity from master
device can be done only when master device is active, thus when master
device is suspended SYSMMU controller device can also be suspended.
This patchset solves the situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).
Patches 1-5 are resend of the "[RFC][PATCH 0/5] Functional dependencies
between devices" patchset:
http://thread.gmane.org/gmane.linux.power-management.general/67424/focus=2126379
I've included them here, because it is hard to find them all on mailing
list archives.
Patches 6-8 are fixes to device dependencies/links code, which were
required to use this solution for Exynos IOMMU driver. I'm not PM/runtime
PM code expert, so please double check if my changes are really correct.
This patchset requires my previous changes to Exynos IOMMU driver
submitted in the "Exynos IOMMU: improve clock management" thread:
http://www.spinics.net/lists/arm-kernel/msg505695.html
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
Changelog:
v2:
- replaced PM notifiers with generic device dependencies/links developped
by Rafael J. Wysocki
v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version
Patch summary:
Marek Szyprowski (5):
driver core: Avoid endless recursion if device has more than one link
driver core: Add support for links to already probed drivers
PM core: Fix restoring devices with links during system PM transition
iommu/exynos: Remove excessive, useless debug
iommu/exynos: Add proper runtime pm support
Rafael J. Wysocki (5):
driver core: Add a wrapper around __device_release_driver()
driver core: Functional dependencies tracking support
PM core: Make async suspend/resume of devices use device links
PM core: Make runtime PM of devices use device links
PM core: Optimize the use of device links for runtime PM
drivers/base/base.h | 13 ++
drivers/base/core.c | 410 +++++++++++++++++++++++++++++++++++++++++++
drivers/base/dd.c | 65 +++++--
drivers/base/power/main.c | 68 ++++++-
drivers/base/power/runtime.c | 130 +++++++++++++-
drivers/iommu/exynos-iommu.c | 221 +++++++++++------------
include/linux/device.h | 41 +++++
include/linux/pm.h | 1 +
include/linux/pm_runtime.h | 6 +
9 files changed, 809 insertions(+), 146 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-07-18 10:32:21
Hi Tobias,
On 2016-07-15 15:21, Tobias Jakobi wrote:
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the reason?
Thanks for testing. I will check this issue. Could you send me your .config?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Tobias,
On 2016-07-15 15:21, Tobias Jakobi wrote:
quoted
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the
reason?
Thanks for testing. I will check this issue. Could you send me your
.config?
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-07-18 13:51:00
Dear Tobias
On 2016-07-18 13:00, Tobias Jakobi wrote:
Marek Szyprowski wrote:
quoted
On 2016-07-15 15:21, Tobias Jakobi wrote:
quoted
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the
reason?
Thanks for testing. I will check this issue. Could you send me your
.config?
no_console_suspend switch won't provide more information, but I managed
to reproduce your issue. I'm really confused how enabling runtime pm can
cause problems with usb/smsc95xx ethernet driver (that is the reason for
failed reboot). Maybe it is somehow related to the global relations
between devices and drivers and the fact that creating the runtime pm
links change the order of operations. I will check this again when
Rafael send updated patches. Here is the log I got (after waiting some
time):
# reboot
Broadcast message from root at target (ttySAC1) (Mon Jul 18 13:33:38 2016):
The system is going down for reboot NOW!
INIT: Switching to runlevel: 6
INIT: Sending processes the TERM signal
[info] Using makefile-style concurrent boot in runlevel 6.
[ ok ] Stopping cgroup management proxy daemon: cgproxy[....] Stopping
internet superserver: inetd.
[....] Stopping cgroup management daemon: cgmanagermax77686-rtc
max77686-rtc: RTC alarm IRQ: 119
[ ok ] Shutting down ALSA...done.
random: nonblocking pool is initialized
[info] Saving the system clock.
[info] Hardware Clock updated to Mon Jul 18 13:33:40 UTC 2016.
[ ok ] Asking all remaining processes to terminate...done.
[ ok ] All processes ended within 1 seconds...done.
[ ok ] Stopping rpcbind daemon....
[ ok ] Deconfiguring network interfaces...done.
[ ok ] Unmounting temporary filesystems...done.
[ ok ] Deactivating swap...done.
EXT4-fs (mmcblk1p2): re-mounted. Opts: (null)
[info] Will now restart.
smsc95xx 1-2:1.0 eth0: Failed to read reg index 0x00000114: -110
smsc95xx 1-2:1.0 eth0: Error reading MII_ACCESS
smsc95xx 1-2:1.0 eth0: MII is busy in smsc95xx_mdio_read
smsc95xx 1-2:1.0 eth0: Failed to read MII_BMSR
INFO: task kworker/0:1:410 blocked for more than 120 seconds.
Not tainted 4.7.0-rc7-debug+ #155
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/0:1 D c06850a8 0 410 2 0x00000000
Workqueue: events_freezable mmc_rescan
Backtrace:
[<c0684e90>] (__schedule) from [<c068560c>] (schedule+0x44/0xb0)
r10:00000000 r9:eebe1d88 r8:c0686234 r7:00000000 r6:00000002 r5:7fffffff
r4:7fffffff
[<c06855c8>] (schedule) from [<c068a398>] (schedule_timeout+0x154/0x1b0)
[<c068a244>] (schedule_timeout) from [<c0686250>]
(wait_for_common+0xe0/0x174)
r8:c0686234 r7:00000000 r6:00000002 r5:eebe1d8c r4:7fffffff
[<c0686170>] (wait_for_common) from [<c0686430>]
(wait_for_completion+0x18/0x1c)
r10:00000001 r9:00000000 r8:00000000 r7:eebe1d88 r6:eebe1d78 r5:ee260000
r4:eebe1de4
[<c0686418>] (wait_for_completion) from [<c04e72c8>]
(mmc_wait_for_req+0xc8/0x15c)
[<c04e7200>] (mmc_wait_for_req) from [<c04e73c8>]
(mmc_wait_for_cmd+0x6c/0xa4)
r8:eef73d00 r7:00000003 r6:ee260000 r5:c0a02448 r4:eebe1de4 r3:00000000
[<c04e735c>] (mmc_wait_for_cmd) from [<c04edbf4>]
(mmc_send_status+0x8c/0xb0)
r7:eebe1eb0 r6:ee260000 r5:ee260000 r4:00000000
[<c04edb68>] (mmc_send_status) from [<c04eaf80>] (mmc_alive+0x18/0x1c)
r4:ee260000
[<c04eaf68>] (mmc_alive) from [<c04e93d0>]
(_mmc_detect_card_removed+0x40/0x90)
[<c04e9390>] (_mmc_detect_card_removed) from [<c04eba80>]
(mmc_detect+0x2c/0x78)
r5:ee2602bc r4:ee260000
[<c04eba54>] (mmc_detect) from [<c04e96ac>] (mmc_rescan+0x1b0/0x324)
r5:ee2602bc r4:ee260368
[<c04e94fc>] (mmc_rescan) from [<c013a818>] (process_one_work+0x194/0x414)
r8:eef73d00 r7:eebe1eb0 r6:eef70280 r5:ee260368 r4:eea24080 r3:c04e94fc
[<c013a684>] (process_one_work) from [<c013ab0c>] (worker_thread+0x34/0x4d4)
r10:eef70280 r9:c0a02100 r8:00000008 r7:eef70280 r6:eea24098 r5:eef702b4
r4:eea24080
[<c013aad8>] (worker_thread) from [<c0141e18>] (kthread+0xf8/0x11c)
r10:00000000 r9:00000000 r8:00000000 r7:c013aad8 r6:eea24080 r5:00000000
r4:eea29d00
[<c0141d20>] (kthread) from [<c0107ff0>] (ret_from_fork+0x14/0x24)
r7:00000000 r6:00000000 r5:c0141d20 r4:eea29d00
2 locks held by kworker/0:1/410:
#0: ("events_freezable"){.+.+.+}, at: [<c013a7ac>]
process_one_work+0x128/0x414
#1: ((&(&host->detect)->work)){+.+.+.}, at: [<c013a7ac>]
process_one_work+0x128/0x414
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Dear Tobias
On 2016-07-18 13:00, Tobias Jakobi wrote:
quoted
Marek Szyprowski wrote:
quoted
On 2016-07-15 15:21, Tobias Jakobi wrote:
quoted
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks
reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the
reason?
Thanks for testing. I will check this issue. Could you send me your
.config?
no_console_suspend switch won't provide more information, but I managed
to reproduce your issue. I'm really confused how enabling runtime pm can
cause problems with usb/smsc95xx ethernet driver (that is the reason for
failed reboot). Maybe it is somehow related to the global relations
between devices and drivers and the fact that creating the runtime pm
links change the order of operations. I will check this again when
Rafael send updated patches. Here is the log I got (after waiting some
time):
thanks for looking into this! I'll try to reproduce this on my board. I
have to admit that I didn't wait too long for the hung task message to
appear.
I wonder if this has something to do with regulator code cutting some
supplies too early. Is this on a X2 or a U2/U3? I'm not sure if we
currently model the regulator setup correctly here (IIRC then buck8 is
supplying the LAN/USB block on U2/U3).
- Tobias
# reboot
Broadcast message from root at target (ttySAC1) (Mon Jul 18 13:33:38 2016):
The system is going down for reboot NOW!
INIT: Switching to runlevel: 6
INIT: Sending processes the TERM signal
[info] Using makefile-style concurrent boot in runlevel 6.
[ ok ] Stopping cgroup management proxy daemon: cgproxy[....] Stopping
internet superserver: inetd.
[....] Stopping cgroup management daemon: cgmanagermax77686-rtc
max77686-rtc: RTC alarm IRQ: 119
[ ok ] Shutting down ALSA...done.
random: nonblocking pool is initialized
[info] Saving the system clock.
[info] Hardware Clock updated to Mon Jul 18 13:33:40 UTC 2016.
[ ok ] Asking all remaining processes to terminate...done.
[ ok ] All processes ended within 1 seconds...done.
[ ok ] Stopping rpcbind daemon....
[ ok ] Deconfiguring network interfaces...done.
[ ok ] Unmounting temporary filesystems...done.
[ ok ] Deactivating swap...done.
EXT4-fs (mmcblk1p2): re-mounted. Opts: (null)
[info] Will now restart.
smsc95xx 1-2:1.0 eth0: Failed to read reg index 0x00000114: -110
smsc95xx 1-2:1.0 eth0: Error reading MII_ACCESS
smsc95xx 1-2:1.0 eth0: MII is busy in smsc95xx_mdio_read
smsc95xx 1-2:1.0 eth0: Failed to read MII_BMSR
INFO: task kworker/0:1:410 blocked for more than 120 seconds.
Not tainted 4.7.0-rc7-debug+ #155
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/0:1 D c06850a8 0 410 2 0x00000000
Workqueue: events_freezable mmc_rescan
Backtrace:
[<c0684e90>] (__schedule) from [<c068560c>] (schedule+0x44/0xb0)
r10:00000000 r9:eebe1d88 r8:c0686234 r7:00000000 r6:00000002 r5:7fffffff
r4:7fffffff
[<c06855c8>] (schedule) from [<c068a398>] (schedule_timeout+0x154/0x1b0)
[<c068a244>] (schedule_timeout) from [<c0686250>]
(wait_for_common+0xe0/0x174)
r8:c0686234 r7:00000000 r6:00000002 r5:eebe1d8c r4:7fffffff
[<c0686170>] (wait_for_common) from [<c0686430>]
(wait_for_completion+0x18/0x1c)
r10:00000001 r9:00000000 r8:00000000 r7:eebe1d88 r6:eebe1d78 r5:ee260000
r4:eebe1de4
[<c0686418>] (wait_for_completion) from [<c04e72c8>]
(mmc_wait_for_req+0xc8/0x15c)
[<c04e7200>] (mmc_wait_for_req) from [<c04e73c8>]
(mmc_wait_for_cmd+0x6c/0xa4)
r8:eef73d00 r7:00000003 r6:ee260000 r5:c0a02448 r4:eebe1de4 r3:00000000
[<c04e735c>] (mmc_wait_for_cmd) from [<c04edbf4>]
(mmc_send_status+0x8c/0xb0)
r7:eebe1eb0 r6:ee260000 r5:ee260000 r4:00000000
[<c04edb68>] (mmc_send_status) from [<c04eaf80>] (mmc_alive+0x18/0x1c)
r4:ee260000
[<c04eaf68>] (mmc_alive) from [<c04e93d0>]
(_mmc_detect_card_removed+0x40/0x90)
[<c04e9390>] (_mmc_detect_card_removed) from [<c04eba80>]
(mmc_detect+0x2c/0x78)
r5:ee2602bc r4:ee260000
[<c04eba54>] (mmc_detect) from [<c04e96ac>] (mmc_rescan+0x1b0/0x324)
r5:ee2602bc r4:ee260368
[<c04e94fc>] (mmc_rescan) from [<c013a818>] (process_one_work+0x194/0x414)
r8:eef73d00 r7:eebe1eb0 r6:eef70280 r5:ee260368 r4:eea24080 r3:c04e94fc
[<c013a684>] (process_one_work) from [<c013ab0c>]
(worker_thread+0x34/0x4d4)
r10:eef70280 r9:c0a02100 r8:00000008 r7:eef70280 r6:eea24098 r5:eef702b4
r4:eea24080
[<c013aad8>] (worker_thread) from [<c0141e18>] (kthread+0xf8/0x11c)
r10:00000000 r9:00000000 r8:00000000 r7:c013aad8 r6:eea24080 r5:00000000
r4:eea29d00
[<c0141d20>] (kthread) from [<c0107ff0>] (ret_from_fork+0x14/0x24)
r7:00000000 r6:00000000 r5:c0141d20 r4:eea29d00
2 locks held by kworker/0:1/410:
#0: ("events_freezable"){.+.+.+}, at: [<c013a7ac>]
process_one_work+0x128/0x414
#1: ((&(&host->detect)->work)){+.+.+.}, at: [<c013a7ac>]
process_one_work+0x128/0x414
Best regards
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2016-07-19 06:26:27
Hi Tobias
On 2016-07-18 18:43, Tobias Jakobi wrote:
Marek Szyprowski wrote:
quoted
On 2016-07-18 13:00, Tobias Jakobi wrote:
quoted
Marek Szyprowski wrote:
quoted
On 2016-07-15 15:21, Tobias Jakobi wrote:
quoted
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks
reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the
reason?
Thanks for testing. I will check this issue. Could you send me your
.config?
no_console_suspend switch won't provide more information, but I managed
to reproduce your issue. I'm really confused how enabling runtime pm can
cause problems with usb/smsc95xx ethernet driver (that is the reason for
failed reboot). Maybe it is somehow related to the global relations
between devices and drivers and the fact that creating the runtime pm
links change the order of operations. I will check this again when
Rafael send updated patches. Here is the log I got (after waiting some
time):
thanks for looking into this! I'll try to reproduce this on my board. I
have to admit that I didn't wait too long for the hung task message to
appear.
I wonder if this has something to do with regulator code cutting some
supplies too early. Is this on a X2 or a U2/U3?
I've reproduced it on U3.
I'm not sure if we
currently model the regulator setup correctly here (IIRC then buck8 is
supplying the LAN/USB block on U2/U3).
IMHO it is not really related to regulator operations, but the sequence
of shutting down logical devices in the system. For some reasons when pm
links
are used, something changes the order of operations in system shutdown
procedure, what causes smsc95xx to hang. I have no idea why, but I don't
have
time to investigate it further. I will wait for the next release of Rafael's
pm links patches and then check everything again.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
From: Rafael J. Wysocki <hidden> Date: 2016-07-20 00:28:29
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
Thanks,
Rafael
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
Currently we achieve that via quirk_apple_wait_for_thunderbolt() in
drivers/pci/quirks.c. It would be more elegant if we could make this
relationship explicit with "device links" and let the core handle it.
Or am I mistaken and this particular use case is not what "device links"
are intended for?
Thanks,
Lukas
From: Rafael J. Wysocki <hidden> Date: 2016-07-20 12:47:57
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
Currently we achieve that via quirk_apple_wait_for_thunderbolt() in
drivers/pci/quirks.c. It would be more elegant if we could make this
relationship explicit with "device links" and let the core handle it.
Or am I mistaken and this particular use case is not what "device links"
are intended for?
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
Best regards,
Lukas
From: Rafael J. Wysocki <hidden> Date: 2016-07-20 22:46:41
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
OK, so why would there be a problem with creating links from the NHI (producer)
to the ports (consumers) before binding portdrv to them?
Thanks,
Rafael
On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
OK, so why would there be a problem with creating links from the NHI
(producer) to the ports (consumers) before binding portdrv to them?
Because the ordering in which drivers bind isn't guaranteed. At least
on my machine (Debian), portdrv always binds before thunderbolt.
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
Thanks,
Lukas
From: Rafael J. Wysocki <hidden> Date: 2016-07-21 00:20:24
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
OK, so why would there be a problem with creating links from the NHI
(producer) to the ports (consumers) before binding portdrv to them?
Because the ordering in which drivers bind isn't guaranteed. At least
on my machine (Debian), portdrv always binds before thunderbolt.
But what drivers have to do with that really? Do you need drivers to
know that the dependency is there?
Just add likns *before* even probing for drivers (yes, you can do that)
and the core will handle that for you.
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.
And it really is about what is needed (at least in principle) to run your
device. If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.
Thanks,
Rafael
Hi Tobias
On 2016-07-18 18:43, Tobias Jakobi wrote:
quoted
Marek Szyprowski wrote:
quoted
On 2016-07-18 13:00, Tobias Jakobi wrote:
quoted
Marek Szyprowski wrote:
quoted
On 2016-07-15 15:21, Tobias Jakobi wrote:
quoted
Tobias Jakobi wrote:
quoted
Hello Marek,
I've tested the patchset on 4.7-rc7 and noticed that it breaks
reboot on
my ODROID-X2.
Going to check where exactly things break.
Sadly it's the last patch where everything comes together:
"iommu/exynos: Add proper runtime pm support"
I still have to check if forcing runpm status to 'on' makes a
difference. I suspect that the aggressive clock gating might be the
reason?
Thanks for testing. I will check this issue. Could you send me your
.config?
no_console_suspend switch won't provide more information, but I managed
to reproduce your issue. I'm really confused how enabling runtime pm can
cause problems with usb/smsc95xx ethernet driver (that is the reason for
failed reboot). Maybe it is somehow related to the global relations
between devices and drivers and the fact that creating the runtime pm
links change the order of operations. I will check this again when
Rafael send updated patches. Here is the log I got (after waiting some
time):
thanks for looking into this! I'll try to reproduce this on my board. I
have to admit that I didn't wait too long for the hung task message to
appear.
I wonder if this has something to do with regulator code cutting some
supplies too early. Is this on a X2 or a U2/U3?
I've reproduced it on U3.
Here's what I get on my X2.
* Remounting remaining filesystems read-only ...
* Remounting / read only ...
[ 59.695857] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
[ ok ]
[ ok ]
[ 59.858672] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[ 66.537116] smsc95xx 1-2.1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[ 66.538887] smsc95xx 1-2.1.1:1.0 eth0: Error reading MII_ACCESS
[ 66.544803] smsc95xx 1-2.1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[ 66.551487] smsc95xx 1-2.1.1:1.0 eth0: Failed to read MII_BMSR
[ 93.597127] usb 1-2-port2: cannot reset (err = -110)
[ 94.596714] usb 1-2-port2: cannot reset (err = -110)
[ 95.596737] usb 1-2-port2: cannot reset (err = -110)
[ 96.596722] usb 1-2-port2: cannot reset (err = -110)
[ 97.596735] usb 1-2-port2: cannot reset (err = -110)
[ 97.596800] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
[ 98.602116] usb 1-2-port2: cannot disable (err = -110)
[ 99.601744] usb 1-2-port2: cannot reset (err = -110)
[ 100.601730] usb 1-2-port2: cannot reset (err = -110)
[ 101.601743] usb 1-2-port2: cannot reset (err = -110)
[ 102.601729] usb 1-2-port2: cannot reset (err = -110)
[ 103.601746] usb 1-2-port2: cannot reset (err = -110)
[ 103.601811] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
[ 104.606737] usb 1-2-port2: cannot disable (err = -110)
[ 105.606756] usb 1-2-port2: cannot reset (err = -110)
[ 106.606742] usb 1-2-port2: cannot reset (err = -110)
[ 107.606758] usb 1-2-port2: cannot reset (err = -110)
[ 108.606747] usb 1-2-port2: cannot reset (err = -110)
[ 109.606763] usb 1-2-port2: cannot reset (err = -110)
[ 109.606835] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
[ 110.611748] usb 1-2-port2: cannot disable (err = -110)
[ 111.611766] usb 1-2-port2: cannot reset (err = -110)
[ 112.611758] usb 1-2-port2: cannot reset (err = -110)
[ 113.611769] usb 1-2-port2: cannot reset (err = -110)
[ 114.611758] usb 1-2-port2: cannot reset (err = -110)
[ 115.611776] usb 1-2-port2: cannot reset (err = -110)
[ 115.611846] usb 1-2-port2: Cannot enable. Maybe the USB cable is bad?
[ 116.616764] usb 1-2-port2: cannot disable (err = -110)
[ 117.617148] usb 1-2-port2: cannot disable (err = -110)
[ 122.616775] hub 1-2:1.0: hub_ext_port_status failed (err = -110)
Output stops there, I don't see any more output with respect to hung
tasks (even though hung task detection is on and I've waited for like 10
minutes).
Anyway, I looks like that smsc95xx_unbind() is never called, and hence
the check_carrier() delayed work queue isn't cancelled.
However I'm not convinced that this is the real problem. I have manually
triggered the unbind before shutdown, and while the error message from
the smsc95xx driver have disappeared, the other messages are still produced.
My guess is that we are seeing these messages because reboot is not
working, and not the other way round.
- Tobias
quoted
I'm not sure if we
currently model the regulator setup correctly here (IIRC then buck8 is
supplying the LAN/USB block on U2/U3).
IMHO it is not really related to regulator operations, but the sequence
of shutting down logical devices in the system. For some reasons when pm
links
are used, something changes the order of operations in system shutdown
procedure, what causes smsc95xx to hang. I have no idea why, but I don't
have
time to investigate it further. I will wait for the next release of
Rafael's
pm links patches and then check everything again.
Best regards
On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
quoted
On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
OK, so why would there be a problem with creating links from the NHI
(producer) to the ports (consumers) before binding portdrv to them?
Because the ordering in which drivers bind isn't guaranteed. At least
on my machine (Debian), portdrv always binds before thunderbolt.
But what drivers have to do with that really? Do you need drivers to
know that the dependency is there?
Just add likns *before* even probing for drivers (yes, you can do that)
and the core will handle that for you.
Forgive me for being dense: How do you suggest to add links before
probing drivers? Only way I could think of is with a PCI quirk.
Which is what we're already doing right now (see drivers/pci/quirk.c:
quirk_apple_wait_for_thunderbolt()). And it ain't pretty.
quoted
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.
And it really is about what is needed (at least in principle) to run your
device. If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.
Real life isn't as simple as that.
In this case, we have consumers (hotplug ports) which are doing fine
if the driver for the supplier (NHI) is not loaded. But once it loads,
the links must be in place. Seems only logical to put the links in
place when they're needed, i.e. at load time of the supplier's driver.
Which the patch set doesn't allow right now.
Best regards,
Lukas
From: Rafael J. Wysocki <hidden> Date: 2016-07-28 00:25:35
On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
quoted
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
quoted
On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote:
quoted
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote:
quoted
On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote:
quoted
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote:
quoted
On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote:
quoted
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner [off-list ref] wrote:
quoted
On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote:
quoted
From: "Rafael J. Wysocki" <redacted>
We also have such a functional dependency for Thunderbolt on Macs:
On resume from system sleep, the PCIe hotplug ports may not resume
before the thunderbolt driver has reestablished the PCI tunnels.
Currently this is enforced by quirk_apple_wait_for_thunderbolt()
in drivers/pci/quirks.c. It would be good if we could represent
this dependency using something like Rafael's approach instead of
open coding it, however one detail in Rafael's patches is problematic:
quoted
New links are added by calling device_link_add() which may happen
either before the consumer device is probed or when probing it, in
which case the caller needs to ensure that the driver of the
supplier device is present and functional and the DEVICE_LINK_PROBE_TIME
flag should be passed to device_link_add() to reflect that.
The thunderbolt driver cannot call device_link_add() before the
PCIe hotplug ports are bound to a driver unless we amend portdrv
to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs
if the thunderbolt driver isn't loaded.
It would therefore be beneficial if device_link_add() can be
called even *after* the consumer is bound.
I don't quite follow.
Who's the provider and who's the consumer here?
thunderbolt.ko is the supplier.
But it binds to the children of the ports that are supposed to be its
consumers?
Why is that even expected to work?
No, the consumers are aunts (or uncles) of the supplier, if you will. :-)
The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in
the drawing below). The supplier is the NHI:
(Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
+-- Downstream Bridge 1 --
+-- Downstream Bridge 2 --
...
We're calling pci_power_up() and pci_restore_state() from
pci_pm_resume_noirq(). And that will fail for devices below
the hotplug ports if the PCI tunnels haven't been re-established
yet by the NHI.
So the NHI is a PCIe device, right?
Does the Thunderbolt driver bind to that device?
The NHI is a PCI device but not a bridge. It has class 0x88000.
Yes, thunderbolt.ko binds to the NHI.
And portdrv binds to the upstream bridge and downstream bridges.
Those have class 0x60400.
OK, so why would there be a problem with creating links from the NHI
(producer) to the ports (consumers) before binding portdrv to them?
Because the ordering in which drivers bind isn't guaranteed. At least
on my machine (Debian), portdrv always binds before thunderbolt.
But what drivers have to do with that really? Do you need drivers to
know that the dependency is there?
Just add likns *before* even probing for drivers (yes, you can do that)
and the core will handle that for you.
Forgive me for being dense: How do you suggest to add links before
probing drivers? Only way I could think of is with a PCI quirk.
Which is what we're already doing right now (see drivers/pci/quirk.c:
quirk_apple_wait_for_thunderbolt()). And it ain't pretty.
Well, maybe not, but doing it once during enumeration would be better than
on every resume.
Plus there is runtime PM to cover.
quoted
quoted
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.
And it really is about what is needed (at least in principle) to run your
device. If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.
Real life isn't as simple as that.
In this case, we have consumers (hotplug ports) which are doing fine
if the driver for the supplier (NHI) is not loaded. But once it loads,
the links must be in place.
Hmm.
What if it is not loaded and the system suspends. Will everything work
as expected after the subsequent resume?
Thanks,
Rafael
On Thu, Jul 28, 2016 at 02:30:31AM +0200, Rafael J. Wysocki wrote:
On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
quoted
On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
quoted
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
quoted
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.
And it really is about what is needed (at least in principle) to run your
device. If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.
Real life isn't as simple as that.
In this case, we have consumers (hotplug ports) which are doing fine
if the driver for the supplier (NHI) is not loaded. But once it loads,
the links must be in place.
Hmm.
What if it is not loaded and the system suspends. Will everything work
as expected after the subsequent resume?
The short answer is yes.
Long answer:
With Thunderbolt, the switch fabric is told to set up PCI tunnels
through the NHI (Native Host Interface). Once set up, the tunnels
stay as they are and attached devices are reachable. However after
a power cycle of the controller (suspend/resume), the tunnels are
gone and need to be re-established.
On Macs, there are two software components communicating with the
NHI: The first one is an EFI driver which sets up tunnels to all
devices present on boot and lights up all attached DP-over-Thunderbolt
displays. Once ExitBootServices is called, the EFI driver is shut
down but the configured tunnels stay as they are. The kernel is thus
able to enumerate attached PCI devices.
The second component is the OS driver, thunderbolt.ko. It is needed
to set up tunnels to hot-plugged devices (i.e., not present at boot).
It is also needed to re-establish tunnels after suspend/resume.
The necessity of quirk_apple_wait_for_thunderbolt() arises because
we walk the entire PCI hierarchy during ->resume_noirq and call
pci_power_up() and pci_restore_state() for each device. Now remember,
the PCI tunnels are gone after a power cycle, so the attached devices
aren't reachable. Waking them and restoring their state will fail
unless the thunderbolt driver reconfigures the switch fabric first.
=> So if there are no devices attached and thunderbolt.ko isn't loaded,
everything is fine. No device link needed.
=> If devices are attached and thunderbolt.ko is loaded, then the hotplug
ports need to wait for re-establishment of the PCI tunnels.
Device link is needed.
=> If devices were attached on boot and thunderbolt.ko isn't loaded, they
will be unreachable after resume. Nothing we can do about that.
No device link needed.
So this is a case of a "weak" device link, "weak" referring to the fact
that it's only needed if the supplier is bound.
All that said, I don't know if this case exists often enough that it's
worth making allowances for it in the driver core.
Sorry for the wall of text, just want to make sure we're on the same page
and all possible use cases of device links are discussed and considered.
Thanks,
Lukas
From: Rafael J. Wysocki <hidden> Date: 2016-09-06 23:03:47
On Friday, June 17, 2016 08:26:56 AM Marek Szyprowski wrote:
quoted hunk
This patch fixes endless recursion, which happends when device has
more than one link.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
If I'm not mistaken, this should not be necessary unless dev has a link
pointing to itself as a consumer. That would be a bug, though.
I can add a WARN_ON() to catch this case, but then if there's a link from
a consumer of dev pointing back to dev as a consumer, that still will loop
forever.
I guess we need to detect circular dependencies and fail link creation in
such cases. Oh well.
Thanks,
Rafael
From: Rafael J. Wysocki <hidden> Date: 2016-09-06 23:07:28
On Friday, June 17, 2016 08:26:57 AM Marek Szyprowski wrote:
quoted hunk
Set proper link state if link is created between already probed supplier
device and to be probed consumer device.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/base/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
The supplier->driver check is insufficient and racy.
It is insufficient, because supplier->driver is also set during supplier probe
and the probe may still not be successful.
It is racy, because supplier->driver may be modified right after this check.
The only way to address the issue at hand I can see is to add a flag to
indicate to device_link_add() that the supplier has already been probed
successfully.
Thanks,
Rafael
From: Rafael J. Wysocki <hidden> Date: 2016-09-06 23:18:44
On Friday, June 17, 2016 08:26:58 AM Marek Szyprowski wrote:
When devices are being runtime resumed during the system PM transition to
suspend state, the link suppliers might be already resumed and
have runtime pm disabled. This is normal case. This patch adds special
support for such case. Simple call to pm_runtime_get_sync returns error
when device has runtime PM disabled, what results in incorrect runtime PM
state during system wide PM transition.
The problem is real, but the proposed solution is questionable.
The reason why power.disable_depth is different from 0 may not be system suspend,
in which case it should fail even if the status happens to be RPM_ACTIVE.
Thanks,
Rafael
From: Rafael J. Wysocki <hidden> Date: 2016-09-06 23:51:49
On Thursday, July 28, 2016 05:28:31 PM Lukas Wunner wrote:
On Thu, Jul 28, 2016 at 02:30:31AM +0200, Rafael J. Wysocki wrote:
quoted
On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote:
quoted
On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote:
quoted
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote:
quoted
I guess I could amend portdrv to return -EPROBE_DEFER on Macs if
no driver is bound to the NHI. Doesn't feel pretty to me though.
Ultimately this seems to be the same issue as with calling
dev_pm_domain_set() for a bound device. Perhaps device_link_add()
can likewise be allowed if a runtime PM ref is held for the devices
and the call happens under lock_system_sleep()?
No, the whole synchronization scheme in the links code would have had to be
changed for that to really work.
And it really is about what is needed (at least in principle) to run your
device. If you think you need device X with a driver to handle device Y
correctly, then either you need it all the time, from probe to remove, or
you just don't really need it at all.
Real life isn't as simple as that.
In this case, we have consumers (hotplug ports) which are doing fine
if the driver for the supplier (NHI) is not loaded. But once it loads,
the links must be in place.
Hmm.
What if it is not loaded and the system suspends. Will everything work
as expected after the subsequent resume?
The short answer is yes.
OK
I think it's possible to add a link flag to address this case.
Namely, if that flag is passed to device_link_add(), the link will be
added in the DEVICE_LINK_ACTIVE state right away, but that will need to
be synchronized against all possible transitions of the consumer device
(at least).
It's better to do that in a separate patch for this reason IMO.
Thanks,
Rafael