All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"open list:AMD IOMMU (AMD-VI)" <iommu@lists.linux-foundation.org>,
	linux-samsung-soc@vger.kernel.org,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Joerg Roedel <joro@8bytes.org>, Inki Dae <inki.dae@samsung.com>,
	Kukjin Kim <kgene@kernel.org>,
	Krzysztof Kozlowski <k.kozlowski@samsung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andreas Noever <andreas.noever@gmail.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
Date: Thu, 28 Jul 2016 17:28:31 +0200	[thread overview]
Message-ID: <20160728152831.GA1929@wunner.de> (raw)
In-Reply-To: <2841548.NIYVpJFBcH@vostro.rjw.lan>

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:
> > 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:
> > > > 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

WARNING: multiple messages have this Message-ID (diff)
From: lukas@wunner.de (Lukas Wunner)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 02/10] driver core: Functional dependencies tracking support
Date: Thu, 28 Jul 2016 17:28:31 +0200	[thread overview]
Message-ID: <20160728152831.GA1929@wunner.de> (raw)
In-Reply-To: <2841548.NIYVpJFBcH@vostro.rjw.lan>

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:
> > 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:
> > > > 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

  reply	other threads:[~2016-07-28 15:28 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-17  6:26 [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski
2016-06-17  6:26 ` Marek Szyprowski
2016-06-17  6:26 ` Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 01/10] driver core: Add a wrapper around __device_release_driver() Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 02/10] driver core: Functional dependencies tracking support Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17 10:36   ` Lukas Wunner
2016-06-17 10:36     ` Lukas Wunner
2016-06-17 12:54     ` Rafael J. Wysocki
2016-06-17 12:54       ` Rafael J. Wysocki
2016-06-17 12:54       ` Rafael J. Wysocki
2016-06-17 14:07       ` Lukas Wunner
2016-06-17 14:07         ` Lukas Wunner
2016-06-17 14:07         ` Lukas Wunner
2016-07-20  0:33         ` Rafael J. Wysocki
2016-07-20  0:33           ` Rafael J. Wysocki
2016-07-20  0:33           ` Rafael J. Wysocki
2016-07-20  0:33           ` Rafael J. Wysocki
2016-07-20  6:24           ` Lukas Wunner
2016-07-20  6:24             ` Lukas Wunner
2016-07-20  6:24             ` Lukas Wunner
2016-07-20  6:24             ` Lukas Wunner
2016-07-20 12:52             ` Rafael J. Wysocki
2016-07-20 12:52               ` Rafael J. Wysocki
2016-07-20 12:52               ` Rafael J. Wysocki
2016-07-20 12:52               ` Rafael J. Wysocki
2016-07-20 15:23               ` Lukas Wunner
2016-07-20 15:23                 ` Lukas Wunner
2016-07-20 15:23                 ` Lukas Wunner
2016-07-20 15:23                 ` Lukas Wunner
2016-07-20 22:51                 ` Rafael J. Wysocki
2016-07-20 22:51                   ` Rafael J. Wysocki
2016-07-20 22:51                   ` Rafael J. Wysocki
2016-07-20 22:51                   ` Rafael J. Wysocki
2016-07-20 23:25                   ` Lukas Wunner
2016-07-20 23:25                     ` Lukas Wunner
2016-07-20 23:25                     ` Lukas Wunner
2016-07-20 23:25                     ` Lukas Wunner
2016-07-21  0:25                     ` Rafael J. Wysocki
2016-07-21  0:25                       ` Rafael J. Wysocki
2016-07-21  0:25                       ` Rafael J. Wysocki
2016-07-21  0:25                       ` Rafael J. Wysocki
2016-07-24 22:48                       ` Lukas Wunner
2016-07-24 22:48                         ` Lukas Wunner
2016-07-24 22:48                         ` Lukas Wunner
2016-07-28  0:30                         ` Rafael J. Wysocki
2016-07-28  0:30                           ` Rafael J. Wysocki
2016-07-28  0:30                           ` Rafael J. Wysocki
2016-07-28 15:28                           ` Lukas Wunner [this message]
2016-07-28 15:28                             ` Lukas Wunner
2016-07-28 15:28                             ` Lukas Wunner
2016-09-06 23:57                             ` Rafael J. Wysocki
2016-09-06 23:57                               ` Rafael J. Wysocki
2016-09-06 23:57                               ` Rafael J. Wysocki
2016-09-06 23:57                               ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 03/10] PM core: Make async suspend/resume of devices use device links Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 04/10] PM core: Make runtime PM " Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 05/10] PM core: Optimize the use of device links for runtime PM Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26 ` [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-09-06 23:09   ` Rafael J. Wysocki
2016-09-06 23:09     ` Rafael J. Wysocki
2016-09-06 23:09     ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 07/10] driver core: Add support for links to already probed drivers Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-09-06 23:13   ` Rafael J. Wysocki
2016-09-06 23:13     ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-09-06 23:24   ` Rafael J. Wysocki
2016-09-06 23:24     ` Rafael J. Wysocki
2016-06-17  6:26 ` [PATCH v2 09/10] iommu/exynos: Remove excessive, useless debug Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:26   ` Marek Szyprowski
2016-06-17  6:27 ` [PATCH v2 10/10] iommu/exynos: Add proper runtime pm support Marek Szyprowski
2016-06-17  6:27   ` Marek Szyprowski
2016-06-17  6:27   ` Marek Szyprowski
2016-07-14 15:41 ` [PATCH v2 00/10] Exynos IOMMU: proper runtime PM support (use device dependencies) Tobias Jakobi
2016-07-14 15:41   ` Tobias Jakobi
2016-07-15 13:21   ` Tobias Jakobi
2016-07-15 13:21     ` Tobias Jakobi
2016-07-15 13:21     ` Tobias Jakobi
2016-07-18 10:32     ` Marek Szyprowski
2016-07-18 10:32       ` Marek Szyprowski
2016-07-18 11:00       ` Tobias Jakobi
2016-07-18 11:00         ` Tobias Jakobi
2016-07-18 13:50         ` Marek Szyprowski
2016-07-18 13:50           ` Marek Szyprowski
2016-07-18 13:50           ` Marek Szyprowski
2016-07-18 16:43           ` Tobias Jakobi
2016-07-18 16:43             ` Tobias Jakobi
2016-07-18 16:43             ` Tobias Jakobi
2016-07-19  6:26             ` Marek Szyprowski
2016-07-19  6:26               ` Marek Szyprowski
2016-07-19  6:26               ` Marek Szyprowski
2016-07-24 18:02               ` Tobias Jakobi
2016-07-24 18:02                 ` Tobias Jakobi
2016-07-24 18:02                 ` Tobias Jakobi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160728152831.GA1929@wunner.de \
    --to=lukas@wunner.de \
    --cc=andreas.noever@gmail.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=broonie@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=inki.dae@samsung.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=k.kozlowski@samsung.com \
    --cc=kgene@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=m.szyprowski@samsung.com \
    --cc=rjw@rjwysocki.net \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.