Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Bjorn Helgaas]

[+cc linux-kernel]

On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
> 
> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
>
> Tested on MacBookPro10,1
> 
> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > This series powers Thunderbolt controllers on Macs down when nothing is
> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
> >
> > Briefly, a custom ACPI method provided by Apple is used to cut power to
> > the controller.  A GPE is enabled while the controller is powered down
> > which side-band signals a plug event, whereupon power is reinstated using
> > the ACPI method.  Note that even though this mechanism is ACPI-based,
> > it does not use _PSx methods and is thus entirely nonstandard.

I think the current arrangement was that Andreas would ack Thunderbolt
patches and I would merge them via the PCI tree.  That makes some sense
because Thunderbolt and PCIe are related, but the more I think about
it, the less I'm happy with it.

This series is a good example.  I'm sure it's good work and
worthwhile.  But I can't really say anything about the content of it
because most of it is Thunderbolt-specific and there's no public spec.
It seems like this is basically a collection of reverse-engineered
quirks that happen to work with the current state of Linux PM on
certain Macs.  We don't know what might change on future Macs.  We
don't know what might break when we make changes to Linux PM.

I can't test this series, nor do I want to.  I can't test most of the
patches I merge, but I can at least read the spec and see whether the
patches make sense.  What I would *like* is to have public Thunderbolt
specs and a kernel developer's guide so we know what to expect from
the hardware and the firmware and we can write code that should work
not just on current Macs, but also on non-Macs and future Macs.

I don't think the current situation is really maintainable, and I'm
not comfortable merging code that I can't maintain.

I know I don't understand the whole situation, so somebody please tell
me why I'm being unreasonable here.

> > A Thunderbolt controller appears to the OS as a set of PCI devices:  One
> > NHI (Native Host Interface) and multiple bridges.  Power is cut to the
> > entire set of devices:
> >
> >   (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
> >                                      +-- Downstream Bridge 1 --
> >                                      +-- Downstream Bridge 2 --
> >                                      ...
> >
> > v1 of this series shoehorned power control into the NHI driver.  This
> > violated the Linux pm model which assumes that a child cannot resume
> > before its parent. As seen above, the NHI is a child, so the child cut
> > power to the bridges above it.
> >
> > v2 resolves this by positioning power control on the controller's
> > topmost device, which is the upstream bridge.  That is achieved by
> > binding to it as a Thunderbolt port service driver.  portdrv already
> > calls down to each service driver on ->suspend and ->resume and I
> > extended that scheme to further PM callbacks.  E.g. when the upstream
> > bridge is runtime suspended, portdrv invokes the ->runtime_suspend
> > callback of each attached service driver, and the Thunderbolt service
> > driver's callback in turn invokes the ACPI method to cut power to the
> > controller.
> >
> >
> > For such a nonstandard ACPI-based PM mechanism one would normally assign
> > a dev_pm_domain to the upstream bridge which overrides the PCI subsystem
> > PM callbacks.  But that's not an option here because dev_pm_domain_set()
> > can only be called during driver probe.  The driver is portdrv and
> > obviously loads earlier than the thunderbolt port service driver.
> > So one has to make do with the PCI subsystem PM callbacks.
> >
> > The PCI core currently assumes that devices can only be put into D3cold
> > by the platform, i.e. using the standard ACPI _PSx methods.  I extended
> > the PCI core so that it can deal with devices which are put into D3cold
> > by the driver callbacks.  It turns out only two changes are needed to
> > make this work, and they are in patches [09/13] and [10/13].  Runtime
> > suspend works out of the box, but runtime resume tries to set the device
> > power state *before* invoking the driver callback, and this goes awry
> > since the device is still in D3cold.  I solved this by returning an error
> > in pci_raw_set_power_state() if the device's current_state is D3cold
> > (patch [09/13]).
> >
> > Theoretically it would also be possible to patch the missing _PSx methods
> > into the ACPI namespace at runtime but I suspect it wouldn't be pretty:
> > I think I'd have to include pre-compiled AML methods in the kernel and
> > modify those blobs at runtime (adjust GPE number etc) before patching
> > them into the namespace.
> >
> >
> > To make direct-complete work for such non-platform-power-managed devices,
> > I also had to modify pci_target_state() (patch [10/13]).
> >
> > Finally, I wanted to avoid the mandatory runtime resume after direct-
> > complete which was introduced by Rafael with 58a1fbbb2ee8 ("PM / PCI /
> > ACPI: Kick devices that might have been reset by firmware"), so I added
> > the possibility to opt out of it (patch [11/13]).
> >
> >
> > I've pushed these patches to GitHub where they can be reviewed more
> > comfortably with green/red highlighting:
> > https://github.com/l1k/linux/commits/thunderbolt_runpm_v2
> >
> > For reference, here's a link to v1:
> > http://thread.gmane.org/gmane.linux.power-management.general/73197
> >
> > Thanks in advance for your comments.
> >
> > Lukas
> >
> >
> > Lukas Wunner (13):
> >   PCI: Recognize Thunderbolt devices
> >   PCI: Allow D3 for Thunderbolt ports
> >   PCI: Add Thunderbolt portdrv service type
> >   PCI: Generalize portdrv pm iterator
> >   PCI: Use portdrv pm iterator on further callbacks
> >   PCI: pciehp: Support runtime pm
> >   PCI: pciehp: Ignore interrupts during D3cold
> >   PCI: Allow runtime PM for Thunderbolt hotplug ports on Macs
> >   PCI: Do not write to PM control register while in D3cold
> >   PCI: Avoid going from D3cold to D3hot for system sleep
> >   PM / sleep: Allow opt-out from runtime resume after direct-complete
> >   thunderbolt: Support runtime pm on upstream bridge
> >   thunderbolt: Support runtime pm on NHI
> >
> >  drivers/base/power/generic_ops.c  |   3 +-
> >  drivers/pci/hotplug/pciehp_ctrl.c |   9 +-
> >  drivers/pci/hotplug/pciehp_hpc.c  |   4 +
> >  drivers/pci/pci.c                 |  50 ++----
> >  drivers/pci/pci.h                 |   2 +
> >  drivers/pci/pcie/portdrv.h        |   6 +-
> >  drivers/pci/pcie/portdrv_core.c   |  47 +-----
> >  drivers/pci/pcie/portdrv_pci.c    |  88 ++++++++--
> >  drivers/pci/probe.c               |  17 ++
> >  drivers/thunderbolt/Kconfig       |   4 +-
> >  drivers/thunderbolt/Makefile      |   4 +-
> >  drivers/thunderbolt/nhi.c         |  32 +++-
> >  drivers/thunderbolt/switch.c      |   9 +
> >  drivers/thunderbolt/tb.c          |  13 ++
> >  drivers/thunderbolt/upstream.c    | 345 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h               |   1 +
> >  include/linux/pcieport_if.h       |   7 +
> >  include/linux/pm.h                |   1 +
> >  18 files changed, 539 insertions(+), 103 deletions(-)
> >  create mode 100644 drivers/thunderbolt/upstream.c
> >
> > --
> > 2.8.1
> >

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Andreas Noever]

On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc linux-kernel]
>
> On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
>>
>> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
>>
>> Tested on MacBookPro10,1
>>
>> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> > This series powers Thunderbolt controllers on Macs down when nothing is
>> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
>> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
>> >
>> > Briefly, a custom ACPI method provided by Apple is used to cut power to
>> > the controller.  A GPE is enabled while the controller is powered down
>> > which side-band signals a plug event, whereupon power is reinstated using
>> > the ACPI method.  Note that even though this mechanism is ACPI-based,
>> > it does not use _PSx methods and is thus entirely nonstandard.
>
> I think the current arrangement was that Andreas would ack Thunderbolt
> patches and I would merge them via the PCI tree.  That makes some sense
> because Thunderbolt and PCIe are related, but the more I think about
> it, the less I'm happy with it.
>
> This series is a good example.  I'm sure it's good work and
> worthwhile.  But I can't really say anything about the content of it
> because most of it is Thunderbolt-specific and there's no public spec.
> It seems like this is basically a collection of reverse-engineered
> quirks that happen to work with the current state of Linux PM on
> certain Macs.  We don't know what might change on future Macs.  We
> don't know what might break when we make changes to Linux PM.
>
> I can't test this series, nor do I want to.  I can't test most of the
> patches I merge, but I can at least read the spec and see whether the
> patches make sense.  What I would *like* is to have public Thunderbolt
> specs and a kernel developer's guide so we know what to expect from
> the hardware and the firmware and we can write code that should work
> not just on current Macs, but also on non-Macs and future Macs.
>
> I don't think the current situation is really maintainable, and I'm
> not comfortable merging code that I can't maintain.
Most of the code is contained within the thunderbolt driver. I think
there is quite some precedence for reverse engineered drivers without
specs being part of the kernel. My understanding was that, since I am
listed in MAINTAINERS, I am responsible for the driver. Now our
changes often need improvements to the pci core, which is why I think
merging through your tree is a good idea (without transferring
responsibility). The changes to the drivers/pci should be supported by
the PCI-spec and make sense without knowing about thunderbolt (but it
might be the case that thunderbolt is the only user of these
features).

Specifically for this series we want to:
 - whitelist thunderbolt bridges for PM. Detecting those bridges is
non-standard but I think this is acceptable, since this
blacklist/whitelist is basically a quirk.
 - Load our portdrv on tb bridges. PCI just sees another portdriver
and all the reverse engineered magic lives inside the driver.
 - Forward more PM callbacks to portdrivers (not tb specific)
 - hotplug D3cold fixes: resume around board_added/remove_board,
ignore interrupts in d3cold (not tb specific and probably a general
bugfix)
 - Make pci not fail if bridges have been put into D3cold by some
external mechanism.

So maybe you could review the pci changes as a solution to the problem
"we want to load a custom portdriver which can put bridges into d3cold
in a device specific way". We certainly to not expect you to take
responsibility for the thunderbolt driver.


> I know I don't understand the whole situation, so somebody please tell
> me why I'm being unreasonable here.
>
>> > A Thunderbolt controller appears to the OS as a set of PCI devices:  One
>> > NHI (Native Host Interface) and multiple bridges.  Power is cut to the
>> > entire set of devices:
>> >
>> >   (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI
>> >                                      +-- Downstream Bridge 1 --
>> >                                      +-- Downstream Bridge 2 --
>> >                                      ...
>> >
>> > v1 of this series shoehorned power control into the NHI driver.  This
>> > violated the Linux pm model which assumes that a child cannot resume
>> > before its parent. As seen above, the NHI is a child, so the child cut
>> > power to the bridges above it.
>> >
>> > v2 resolves this by positioning power control on the controller's
>> > topmost device, which is the upstream bridge.  That is achieved by
>> > binding to it as a Thunderbolt port service driver.  portdrv already
>> > calls down to each service driver on ->suspend and ->resume and I
>> > extended that scheme to further PM callbacks.  E.g. when the upstream
>> > bridge is runtime suspended, portdrv invokes the ->runtime_suspend
>> > callback of each attached service driver, and the Thunderbolt service
>> > driver's callback in turn invokes the ACPI method to cut power to the
>> > controller.
>> >
>> >
>> > For such a nonstandard ACPI-based PM mechanism one would normally assign
>> > a dev_pm_domain to the upstream bridge which overrides the PCI subsystem
>> > PM callbacks.  But that's not an option here because dev_pm_domain_set()
>> > can only be called during driver probe.  The driver is portdrv and
>> > obviously loads earlier than the thunderbolt port service driver.
>> > So one has to make do with the PCI subsystem PM callbacks.
>> >
>> > The PCI core currently assumes that devices can only be put into D3cold
>> > by the platform, i.e. using the standard ACPI _PSx methods.  I extended
>> > the PCI core so that it can deal with devices which are put into D3cold
>> > by the driver callbacks.  It turns out only two changes are needed to
>> > make this work, and they are in patches [09/13] and [10/13].  Runtime
>> > suspend works out of the box, but runtime resume tries to set the device
>> > power state *before* invoking the driver callback, and this goes awry
>> > since the device is still in D3cold.  I solved this by returning an error
>> > in pci_raw_set_power_state() if the device's current_state is D3cold
>> > (patch [09/13]).
>> >
>> > Theoretically it would also be possible to patch the missing _PSx methods
>> > into the ACPI namespace at runtime but I suspect it wouldn't be pretty:
>> > I think I'd have to include pre-compiled AML methods in the kernel and
>> > modify those blobs at runtime (adjust GPE number etc) before patching
>> > them into the namespace.
>> >
>> >
>> > To make direct-complete work for such non-platform-power-managed devices,
>> > I also had to modify pci_target_state() (patch [10/13]).
>> >
>> > Finally, I wanted to avoid the mandatory runtime resume after direct-
>> > complete which was introduced by Rafael with 58a1fbbb2ee8 ("PM / PCI /
>> > ACPI: Kick devices that might have been reset by firmware"), so I added
>> > the possibility to opt out of it (patch [11/13]).
>> >
>> >
>> > I've pushed these patches to GitHub where they can be reviewed more
>> > comfortably with green/red highlighting:
>> > https://github.com/l1k/linux/commits/thunderbolt_runpm_v2
>> >
>> > For reference, here's a link to v1:
>> > http://thread.gmane.org/gmane.linux.power-management.general/73197
>> >
>> > Thanks in advance for your comments.
>> >
>> > Lukas
>> >
>> >
>> > Lukas Wunner (13):
>> >   PCI: Recognize Thunderbolt devices
>> >   PCI: Allow D3 for Thunderbolt ports
>> >   PCI: Add Thunderbolt portdrv service type
>> >   PCI: Generalize portdrv pm iterator
>> >   PCI: Use portdrv pm iterator on further callbacks
>> >   PCI: pciehp: Support runtime pm
>> >   PCI: pciehp: Ignore interrupts during D3cold
>> >   PCI: Allow runtime PM for Thunderbolt hotplug ports on Macs
>> >   PCI: Do not write to PM control register while in D3cold
>> >   PCI: Avoid going from D3cold to D3hot for system sleep
>> >   PM / sleep: Allow opt-out from runtime resume after direct-complete
>> >   thunderbolt: Support runtime pm on upstream bridge
>> >   thunderbolt: Support runtime pm on NHI
>> >
>> >  drivers/base/power/generic_ops.c  |   3 +-
>> >  drivers/pci/hotplug/pciehp_ctrl.c |   9 +-
>> >  drivers/pci/hotplug/pciehp_hpc.c  |   4 +
>> >  drivers/pci/pci.c                 |  50 ++----
>> >  drivers/pci/pci.h                 |   2 +
>> >  drivers/pci/pcie/portdrv.h        |   6 +-
>> >  drivers/pci/pcie/portdrv_core.c   |  47 +-----
>> >  drivers/pci/pcie/portdrv_pci.c    |  88 ++++++++--
>> >  drivers/pci/probe.c               |  17 ++
>> >  drivers/thunderbolt/Kconfig       |   4 +-
>> >  drivers/thunderbolt/Makefile      |   4 +-
>> >  drivers/thunderbolt/nhi.c         |  32 +++-
>> >  drivers/thunderbolt/switch.c      |   9 +
>> >  drivers/thunderbolt/tb.c          |  13 ++
>> >  drivers/thunderbolt/upstream.c    | 345 ++++++++++++++++++++++++++++++++++++++
>> >  include/linux/pci.h               |   1 +
>> >  include/linux/pcieport_if.h       |   7 +
>> >  include/linux/pm.h                |   1 +
>> >  18 files changed, 539 insertions(+), 103 deletions(-)
>> >  create mode 100644 drivers/thunderbolt/upstream.c
>> >
>> > --
>> > 2.8.1
>> >

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Bjorn Helgaas]

On Tue, Jun 14, 2016 at 09:14:27PM +0200, Andreas Noever wrote:
> On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc linux-kernel]
> >
> > On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
> >>
> >> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
> >>
> >> Tested on MacBookPro10,1
> >>
> >> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> > This series powers Thunderbolt controllers on Macs down when nothing is
> >> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
> >> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
> >> >
> >> > Briefly, a custom ACPI method provided by Apple is used to cut power to
> >> > the controller.  A GPE is enabled while the controller is powered down
> >> > which side-band signals a plug event, whereupon power is reinstated using
> >> > the ACPI method.  Note that even though this mechanism is ACPI-based,
> >> > it does not use _PSx methods and is thus entirely nonstandard.
> >
> > I think the current arrangement was that Andreas would ack Thunderbolt
> > patches and I would merge them via the PCI tree.  That makes some sense
> > because Thunderbolt and PCIe are related, but the more I think about
> > it, the less I'm happy with it.
> >
> > This series is a good example.  I'm sure it's good work and
> > worthwhile.  But I can't really say anything about the content of it
> > because most of it is Thunderbolt-specific and there's no public spec.
> > It seems like this is basically a collection of reverse-engineered
> > quirks that happen to work with the current state of Linux PM on
> > certain Macs.  We don't know what might change on future Macs.  We
> > don't know what might break when we make changes to Linux PM.
> >
> > I can't test this series, nor do I want to.  I can't test most of the
> > patches I merge, but I can at least read the spec and see whether the
> > patches make sense.  What I would *like* is to have public Thunderbolt
> > specs and a kernel developer's guide so we know what to expect from
> > the hardware and the firmware and we can write code that should work
> > not just on current Macs, but also on non-Macs and future Macs.
> >
> > I don't think the current situation is really maintainable, and I'm
> > not comfortable merging code that I can't maintain.
> Most of the code is contained within the thunderbolt driver. I think
> there is quite some precedence for reverse engineered drivers without
> specs being part of the kernel. My understanding was that, since I am
> listed in MAINTAINERS, I am responsible for the driver. Now our
> changes often need improvements to the pci core, which is why I think
> merging through your tree is a good idea (without transferring
> responsibility). The changes to the drivers/pci should be supported by
> the PCI-spec and make sense without knowing about thunderbolt (but it
> might be the case that thunderbolt is the only user of these
> features).
> 
> Specifically for this series we want to:
>  - whitelist thunderbolt bridges for PM. Detecting those bridges is
> non-standard but I think this is acceptable, since this
> blacklist/whitelist is basically a quirk.
>  - Load our portdrv on tb bridges. PCI just sees another portdriver
> and all the reverse engineered magic lives inside the driver.
>  - Forward more PM callbacks to portdrivers (not tb specific)
>  - hotplug D3cold fixes: resume around board_added/remove_board,
> ignore interrupts in d3cold (not tb specific and probably a general
> bugfix)
>  - Make pci not fail if bridges have been put into D3cold by some
> external mechanism.
> 
> So maybe you could review the pci changes as a solution to the problem
> "we want to load a custom portdriver which can put bridges into d3cold
> in a device specific way". We certainly to not expect you to take
> responsibility for the thunderbolt driver.

That's a fine solution as far as I'm personally concerned.  I think
it's poor for Linux overall, because I think it's fragile, and it's
disappointing that a technology as important as Thunderbolt is so
poorly supported by the promulgators.  But if you're willing to work
in that environment, that's great.

You maintain the thunderbolt code and merge changes, and I'll review
the pieces that touch drivers/pci.  I do have a couple comments on
those pieces, but I don't think they'll be major.

I just want to get out of the business of merging drivers/thunderbolt
code that I can't maintain.

Bjorn

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Lukas Wunner]

[+cc Linus, Greg KH]

On Tue, Jun 14, 2016 at 03:22:28PM -0500, Bjorn Helgaas wrote:
> On Tue, Jun 14, 2016 at 09:14:27PM +0200, Andreas Noever wrote:
> > On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
> > > > On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > This series powers Thunderbolt controllers on Macs down when
> > > > > nothing is plugged in, saving 1.7 W on machines with a Light Ridge
> > > > > controller and reportedly 4 W on Cactus Ridge 4C and Falcon Ridge
> > > > > 4C.
> > > > >
> > > > > Briefly, a custom ACPI method provided by Apple is used to cut
> > > > > power to the controller.  A GPE is enabled while the controller
> > > > > is powered down which side-band signals a plug event, whereupon
> > > > > power is reinstated using the ACPI method.  Note that even though
> > > > > this mechanism is ACPI-based, it does not use _PSx methods and is
> > > > > thus entirely nonstandard.
> > > >
> > > > Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
> > > > Tested on MacBookPro10,1
> > >
> > > I think the current arrangement was that Andreas would ack Thunderbolt
> > > patches and I would merge them via the PCI tree.  That makes some sense
> > > because Thunderbolt and PCIe are related, but the more I think about
> > > it, the less I'm happy with it.
> > >
> > > This series is a good example.  I'm sure it's good work and
> > > worthwhile.  But I can't really say anything about the content of it
> > > because most of it is Thunderbolt-specific and there's no public spec.
> > > It seems like this is basically a collection of reverse-engineered
> > > quirks that happen to work with the current state of Linux PM on
> > > certain Macs.  We don't know what might change on future Macs.  We
> > > don't know what might break when we make changes to Linux PM.
> > >
> > > I can't test this series, nor do I want to.  I can't test most of the
> > > patches I merge, but I can at least read the spec and see whether the
> > > patches make sense.  What I would *like* is to have public Thunderbolt
> > > specs and a kernel developer's guide so we know what to expect from
> > > the hardware and the firmware and we can write code that should work
> > > not just on current Macs, but also on non-Macs and future Macs.
> > >
> > > I don't think the current situation is really maintainable, and I'm
> > > not comfortable merging code that I can't maintain.
> >
> > Most of the code is contained within the thunderbolt driver. I think
> > there is quite some precedence for reverse engineered drivers without
> > specs being part of the kernel. My understanding was that, since I am
> > listed in MAINTAINERS, I am responsible for the driver. Now our
> > changes often need improvements to the pci core, which is why I think
> > merging through your tree is a good idea (without transferring
> > responsibility). The changes to the drivers/pci should be supported by
> > the PCI-spec and make sense without knowing about thunderbolt (but it
> > might be the case that thunderbolt is the only user of these
> > features). [...]
> > 
> > So maybe you could review the pci changes as a solution to the problem
> > "we want to load a custom portdriver which can put bridges into d3cold
> > in a device specific way". We certainly to not expect you to take
> > responsibility for the thunderbolt driver.
> 
> That's a fine solution as far as I'm personally concerned.  I think
> it's poor for Linux overall, because I think it's fragile, and it's
> disappointing that a technology as important as Thunderbolt is so
> poorly supported by the promulgators.  But if you're willing to work
> in that environment, that's great.
> 
> You maintain the thunderbolt code and merge changes, and I'll review
> the pieces that touch drivers/pci.  I do have a couple comments on
> those pieces, but I don't think they'll be major.
> 
> I just want to get out of the business of merging drivers/thunderbolt
> code that I can't maintain.

So how should changes to drivers/thunderbolt/ be merged in the future?

Andreas could probably send pulls directly to Linus, but I'm not sure
what the requirements are. I believe Linus wants signed tags. The trust
path from Linus to me is 4 hops and I've signed Andreas' key today,
yielding a 5 hop trust path:
http://pgp.cs.uu.nl/mk_path.cgi?FROM=0x79BE3E4300411886&TO=0x2AAF22EB
http://pgp.surfnet.nl:11371/pks/lookup?op=vindex&search=0xB1FCD9A3

Is there an upper limit on the acceptable length of the trust path?
Does the key have to be signed by another maintainer?

I guess the alternative would be that Greg KH picks up the patches,
as he did with the initial version of the Thunderbolt driver back in
2014. I'm not sure if that makes sense as I assume he has numerous
other things on his plate. (Which is not to belittle your own or
Linus' workload.)

Most subsystems seem to practice a four-eyes principle, i.e. a
Reviewed-by should be provided by someone else if author and committer
are the same person. I'll be glad to provide that for Andreas' own
patches such as 2ffa9a5d76a7, or help otherwise if I can.


As concerns this particular series, 10 of the 13 patches, i.e. the
majority, concern the PCI core, 1 concerns the PM core and only 2
concern the Thunderbolt driver. Since the PCI core is generally seeing
a lot more activity than the Thunderbolt driver, the probability of
merge conflicts is much higher if this series is merged through a
different tree than yours. It seems to be common practice to just
accept an Acked-by from other subsystem maintainers as a green light
to merge without looking closer at those patches.


I agree with your assessment that the lack of public documentation on
Thunderbolt is deplorable. However the PCIe spec does define what a
PCIe switch is and how it functions, and Thunderbolt is precisely that.
I.e. it documents a portion of Thunderbolt without ever saying so
explicitly.

You cite the lack of a public spec as a reason for unmaintainability,
yet your subsystem contains code to support Thunderbolt on non-Macs,
in the form of acpiphp. Was the maintainability argument ever mounted
against acpiphp? Intel engineers with access to the spec contributed
the changes for acpiphp to make Thunderbolt work on non-Macs. Is their
code more maintainable than reverse-engineered code?

Best regards,

Lukas

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Linus Torvalds]

On Wed, Jun 15, 2016 at 8:40 AM, Lukas Wunner <lukas@wunner.de> wrote:
>
> So how should changes to drivers/thunderbolt/ be merged in the future?
>
> Andreas could probably send pulls directly to Linus, but I'm not sure
> what the requirements are. I believe Linus wants signed tags. The trust
> path from Linus to me is 4 hops and I've signed Andreas' key today,
> yielding a 5 hop trust path:
>
> Is there an upper limit on the acceptable length of the trust path?
> Does the key have to be signed by another maintainer?

I care not one whit about the idiotic gpg "trust path" crap.

To me, signatures are not about technicalities. I absolutely abhor all
the crazy people who think that signatures are about automatic web of
trust, and spend a lot of time on things like subkeys that expire
every six months etc (you know who you are). To me, that is just
complete gpg masturbation, and completely misses the point about
"trust".

Trust is not about the gpg signature. Trust is about the *person*. And
the gpg signature is a good and reasonable approximation of an ID. But
it's not some kind of absolute thing.

I'd much rather get an email from a current maintainer that I trust,
saying "look, there's going to be a new maintainer for this part of
the tree, and I signed his gpg keym and the fingerprint of that is
so-and-so.

Then, I'll do a "gpg --fetch-key", so that I have that particular key
in my keyring, and can verify that "ok, yes, I recognize the key that
signed it".

At no point do I start counting hops.

And if you lose your key, screw the whole crazy "key revocation
protocol". Its a joke. Most people who lost their keys will not have
any revocation key either. Just let me and others know. I'll just
remove that key from my keychain.

What makes me look at a key is "I've never seen this key before". The
most common reason is the people who do that f*cking annoying "let's
refresh signing keys every six months whether I need it or not because
I auto-expire them". Then I'll have to look at why the hell I'm
getting a signed pull request with a new key.

So don't worry about technicalities. I've pulled from people who had
not a single signature on their keychain, because they just were in
the wrong spot. I'd rather have a signed pull even then, just so that
I see that I get the pull requests from the same person each time, and
hopefully in a week (or month, or two), that key will get signatures.

Obviously, if you can get five people I know personally signing your
key, that makes me worry less about your particular identity, and
that's fine.

But the *real* trust is something that builds up over time as people
are good maintainers. It has absolutely nothing to do with gpg key
details. And that *real* trust is what matters a whole lot more than a
few random bits that just happen to be part of a pgp key.

                Linus

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Andreas Noever]

On Tue, Jun 14, 2016 at 10:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Jun 14, 2016 at 09:14:27PM +0200, Andreas Noever wrote:
>> On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > [+cc linux-kernel]
>> >
>> > On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
>> >>
>> >> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
>> >>
>> >> Tested on MacBookPro10,1
>> >>
>> >> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> >> > This series powers Thunderbolt controllers on Macs down when nothing is
>> >> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
>> >> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
>> >> >
>> >> > Briefly, a custom ACPI method provided by Apple is used to cut power to
>> >> > the controller.  A GPE is enabled while the controller is powered down
>> >> > which side-band signals a plug event, whereupon power is reinstated using
>> >> > the ACPI method.  Note that even though this mechanism is ACPI-based,
>> >> > it does not use _PSx methods and is thus entirely nonstandard.
>> >
>> > I think the current arrangement was that Andreas would ack Thunderbolt
>> > patches and I would merge them via the PCI tree.  That makes some sense
>> > because Thunderbolt and PCIe are related, but the more I think about
>> > it, the less I'm happy with it.
>> >
>> > This series is a good example.  I'm sure it's good work and
>> > worthwhile.  But I can't really say anything about the content of it
>> > because most of it is Thunderbolt-specific and there's no public spec.
>> > It seems like this is basically a collection of reverse-engineered
>> > quirks that happen to work with the current state of Linux PM on
>> > certain Macs.  We don't know what might change on future Macs.  We
>> > don't know what might break when we make changes to Linux PM.
>> >
>> > I can't test this series, nor do I want to.  I can't test most of the
>> > patches I merge, but I can at least read the spec and see whether the
>> > patches make sense.  What I would *like* is to have public Thunderbolt
>> > specs and a kernel developer's guide so we know what to expect from
>> > the hardware and the firmware and we can write code that should work
>> > not just on current Macs, but also on non-Macs and future Macs.
>> >
>> > I don't think the current situation is really maintainable, and I'm
>> > not comfortable merging code that I can't maintain.
>> Most of the code is contained within the thunderbolt driver. I think
>> there is quite some precedence for reverse engineered drivers without
>> specs being part of the kernel. My understanding was that, since I am
>> listed in MAINTAINERS, I am responsible for the driver. Now our
>> changes often need improvements to the pci core, which is why I think
>> merging through your tree is a good idea (without transferring
>> responsibility). The changes to the drivers/pci should be supported by
>> the PCI-spec and make sense without knowing about thunderbolt (but it
>> might be the case that thunderbolt is the only user of these
>> features).
>>
>> Specifically for this series we want to:
>>  - whitelist thunderbolt bridges for PM. Detecting those bridges is
>> non-standard but I think this is acceptable, since this
>> blacklist/whitelist is basically a quirk.
>>  - Load our portdrv on tb bridges. PCI just sees another portdriver
>> and all the reverse engineered magic lives inside the driver.
>>  - Forward more PM callbacks to portdrivers (not tb specific)
>>  - hotplug D3cold fixes: resume around board_added/remove_board,
>> ignore interrupts in d3cold (not tb specific and probably a general
>> bugfix)
>>  - Make pci not fail if bridges have been put into D3cold by some
>> external mechanism.
>>
>> So maybe you could review the pci changes as a solution to the problem
>> "we want to load a custom portdriver which can put bridges into d3cold
>> in a device specific way". We certainly to not expect you to take
>> responsibility for the thunderbolt driver.
>
> That's a fine solution as far as I'm personally concerned.  I think
> it's poor for Linux overall, because I think it's fragile, and it's
> disappointing that a technology as important as Thunderbolt is so
> poorly supported by the promulgators.  But if you're willing to work
> in that environment, that's great.
>
> You maintain the thunderbolt code and merge changes, and I'll review
> the pieces that touch drivers/pci.  I do have a couple comments on
> those pieces, but I don't think they'll be major.
>
> I just want to get out of the business of merging drivers/thunderbolt
> code that I can't maintain.

[+ Greg]

Hi Greg,

do you mind if we revert to the old scheme and merge TB changes
through your tree?

Regards,
Andreas


> Bjorn

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Greg KH]

On Thu, Jul 07, 2016 at 07:39:12PM +0200, Andreas Noever wrote:
> On Tue, Jun 14, 2016 at 10:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Jun 14, 2016 at 09:14:27PM +0200, Andreas Noever wrote:
> >> On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> > [+cc linux-kernel]
> >> >
> >> > On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
> >> >>
> >> >> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
> >> >>
> >> >> Tested on MacBookPro10,1
> >> >>
> >> >> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> >> > This series powers Thunderbolt controllers on Macs down when nothing is
> >> >> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
> >> >> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
> >> >> >
> >> >> > Briefly, a custom ACPI method provided by Apple is used to cut power to
> >> >> > the controller.  A GPE is enabled while the controller is powered down
> >> >> > which side-band signals a plug event, whereupon power is reinstated using
> >> >> > the ACPI method.  Note that even though this mechanism is ACPI-based,
> >> >> > it does not use _PSx methods and is thus entirely nonstandard.
> >> >
> >> > I think the current arrangement was that Andreas would ack Thunderbolt
> >> > patches and I would merge them via the PCI tree.  That makes some sense
> >> > because Thunderbolt and PCIe are related, but the more I think about
> >> > it, the less I'm happy with it.
> >> >
> >> > This series is a good example.  I'm sure it's good work and
> >> > worthwhile.  But I can't really say anything about the content of it
> >> > because most of it is Thunderbolt-specific and there's no public spec.
> >> > It seems like this is basically a collection of reverse-engineered
> >> > quirks that happen to work with the current state of Linux PM on
> >> > certain Macs.  We don't know what might change on future Macs.  We
> >> > don't know what might break when we make changes to Linux PM.
> >> >
> >> > I can't test this series, nor do I want to.  I can't test most of the
> >> > patches I merge, but I can at least read the spec and see whether the
> >> > patches make sense.  What I would *like* is to have public Thunderbolt
> >> > specs and a kernel developer's guide so we know what to expect from
> >> > the hardware and the firmware and we can write code that should work
> >> > not just on current Macs, but also on non-Macs and future Macs.
> >> >
> >> > I don't think the current situation is really maintainable, and I'm
> >> > not comfortable merging code that I can't maintain.
> >> Most of the code is contained within the thunderbolt driver. I think
> >> there is quite some precedence for reverse engineered drivers without
> >> specs being part of the kernel. My understanding was that, since I am
> >> listed in MAINTAINERS, I am responsible for the driver. Now our
> >> changes often need improvements to the pci core, which is why I think
> >> merging through your tree is a good idea (without transferring
> >> responsibility). The changes to the drivers/pci should be supported by
> >> the PCI-spec and make sense without knowing about thunderbolt (but it
> >> might be the case that thunderbolt is the only user of these
> >> features).
> >>
> >> Specifically for this series we want to:
> >>  - whitelist thunderbolt bridges for PM. Detecting those bridges is
> >> non-standard but I think this is acceptable, since this
> >> blacklist/whitelist is basically a quirk.
> >>  - Load our portdrv on tb bridges. PCI just sees another portdriver
> >> and all the reverse engineered magic lives inside the driver.
> >>  - Forward more PM callbacks to portdrivers (not tb specific)
> >>  - hotplug D3cold fixes: resume around board_added/remove_board,
> >> ignore interrupts in d3cold (not tb specific and probably a general
> >> bugfix)
> >>  - Make pci not fail if bridges have been put into D3cold by some
> >> external mechanism.
> >>
> >> So maybe you could review the pci changes as a solution to the problem
> >> "we want to load a custom portdriver which can put bridges into d3cold
> >> in a device specific way". We certainly to not expect you to take
> >> responsibility for the thunderbolt driver.
> >
> > That's a fine solution as far as I'm personally concerned.  I think
> > it's poor for Linux overall, because I think it's fragile, and it's
> > disappointing that a technology as important as Thunderbolt is so
> > poorly supported by the promulgators.  But if you're willing to work
> > in that environment, that's great.
> >
> > You maintain the thunderbolt code and merge changes, and I'll review
> > the pieces that touch drivers/pci.  I do have a couple comments on
> > those pieces, but I don't think they'll be major.
> >
> > I just want to get out of the business of merging drivers/thunderbolt
> > code that I can't maintain.
> 
> [+ Greg]
> 
> Hi Greg,
> 
> do you mind if we revert to the old scheme and merge TB changes
> through your tree?

I will be glad to take them, feel free to send them on to me.

thanks,

greg k-h

Re: [PATCH v2 00/13] Runtime PM for Thunderbolt on Macs

[Andreas Noever]

On Sat, Jul 9, 2016 at 7:23 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jul 07, 2016 at 07:39:12PM +0200, Andreas Noever wrote:
>> On Tue, Jun 14, 2016 at 10:22 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Tue, Jun 14, 2016 at 09:14:27PM +0200, Andreas Noever wrote:
>> >> On Tue, Jun 14, 2016 at 6:37 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >> > [+cc linux-kernel]
>> >> >
>> >> > On Sat, May 21, 2016 at 11:48:42AM +0200, Andreas Noever wrote:
>> >> >>
>> >> >> Signed-off-by: Andreas Noever <andreas.noever@gmail.com>
>> >> >>
>> >> >> Tested on MacBookPro10,1
>> >> >>
>> >> >> On Fri, May 13, 2016 at 1:15 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> >> >> > This series powers Thunderbolt controllers on Macs down when nothing is
>> >> >> > plugged in, saving 1.7 W on machines with a Light Ridge controller and
>> >> >> > reportedly 4 W on Cactus Ridge 4C and Falcon Ridge 4C.
>> >> >> >
>> >> >> > Briefly, a custom ACPI method provided by Apple is used to cut power to
>> >> >> > the controller.  A GPE is enabled while the controller is powered down
>> >> >> > which side-band signals a plug event, whereupon power is reinstated using
>> >> >> > the ACPI method.  Note that even though this mechanism is ACPI-based,
>> >> >> > it does not use _PSx methods and is thus entirely nonstandard.
>> >> >
>> >> > I think the current arrangement was that Andreas would ack Thunderbolt
>> >> > patches and I would merge them via the PCI tree.  That makes some sense
>> >> > because Thunderbolt and PCIe are related, but the more I think about
>> >> > it, the less I'm happy with it.
>> >> >
>> >> > This series is a good example.  I'm sure it's good work and
>> >> > worthwhile.  But I can't really say anything about the content of it
>> >> > because most of it is Thunderbolt-specific and there's no public spec.
>> >> > It seems like this is basically a collection of reverse-engineered
>> >> > quirks that happen to work with the current state of Linux PM on
>> >> > certain Macs.  We don't know what might change on future Macs.  We
>> >> > don't know what might break when we make changes to Linux PM.
>> >> >
>> >> > I can't test this series, nor do I want to.  I can't test most of the
>> >> > patches I merge, but I can at least read the spec and see whether the
>> >> > patches make sense.  What I would *like* is to have public Thunderbolt
>> >> > specs and a kernel developer's guide so we know what to expect from
>> >> > the hardware and the firmware and we can write code that should work
>> >> > not just on current Macs, but also on non-Macs and future Macs.
>> >> >
>> >> > I don't think the current situation is really maintainable, and I'm
>> >> > not comfortable merging code that I can't maintain.
>> >> Most of the code is contained within the thunderbolt driver. I think
>> >> there is quite some precedence for reverse engineered drivers without
>> >> specs being part of the kernel. My understanding was that, since I am
>> >> listed in MAINTAINERS, I am responsible for the driver. Now our
>> >> changes often need improvements to the pci core, which is why I think
>> >> merging through your tree is a good idea (without transferring
>> >> responsibility). The changes to the drivers/pci should be supported by
>> >> the PCI-spec and make sense without knowing about thunderbolt (but it
>> >> might be the case that thunderbolt is the only user of these
>> >> features).
>> >>
>> >> Specifically for this series we want to:
>> >>  - whitelist thunderbolt bridges for PM. Detecting those bridges is
>> >> non-standard but I think this is acceptable, since this
>> >> blacklist/whitelist is basically a quirk.
>> >>  - Load our portdrv on tb bridges. PCI just sees another portdriver
>> >> and all the reverse engineered magic lives inside the driver.
>> >>  - Forward more PM callbacks to portdrivers (not tb specific)
>> >>  - hotplug D3cold fixes: resume around board_added/remove_board,
>> >> ignore interrupts in d3cold (not tb specific and probably a general
>> >> bugfix)
>> >>  - Make pci not fail if bridges have been put into D3cold by some
>> >> external mechanism.
>> >>
>> >> So maybe you could review the pci changes as a solution to the problem
>> >> "we want to load a custom portdriver which can put bridges into d3cold
>> >> in a device specific way". We certainly to not expect you to take
>> >> responsibility for the thunderbolt driver.
>> >
>> > That's a fine solution as far as I'm personally concerned.  I think
>> > it's poor for Linux overall, because I think it's fragile, and it's
>> > disappointing that a technology as important as Thunderbolt is so
>> > poorly supported by the promulgators.  But if you're willing to work
>> > in that environment, that's great.
>> >
>> > You maintain the thunderbolt code and merge changes, and I'll review
>> > the pieces that touch drivers/pci.  I do have a couple comments on
>> > those pieces, but I don't think they'll be major.
>> >
>> > I just want to get out of the business of merging drivers/thunderbolt
>> > code that I can't maintain.
>>
>> [+ Greg]
>>
>> Hi Greg,
>>
>> do you mind if we revert to the old scheme and merge TB changes
>> through your tree?
>
> I will be glad to take them, feel free to send them on to me.

Thanks!
Andreas

> thanks,
>
> greg k-h