| Message ID | 1415847123-15558-1-git-send-email-michael.jamet@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
[+cc Rafael, Andreas] On Wed, Nov 12, 2014 at 7:52 PM, Michael Jamet <michael.jamet@intel.com> wrote: > Every Thunderbolt-based devices or Thunderbolt-connected > devices should not allocate PCI I/O resources > per Thunderbolt specs. Please include a pointer to those specs in the changelog. > On a Thunderbolt PC, BIOS is responsible to allocate IO > resources. Kernel shouldn't allocate the PCI I/O resources > as it interferes with BIOS operation. > Doing this may cause the devices in the Thunderbolt chain > not being detected or added, or worse to stuck the > Thunderbolt Host controller. These new kernel/firmware coordination requirements need to be documented. If they're already part of a PCIe ECN or PCI firmware spec, just provide a pointer. > To prevent this, we detect a chain contains a Thunderbolt > device by checking the Thunderbolt PCI device id. I'm really not happy about checking a list of device IDs to identify Thunderbolt devices. Surely there's a better way, because a list like this has to be updated regularly. Bjorn > The validation is carried out at the pci_enable_device_flags() > function that checks the PCI device or bridge is Thunderbolt > chained and avoid IO resources allocation. > > In addition, decode_bar() and pci_bridge_check_ranges() > function has been internally checked for Thunderbolt > as well to ensure no IO resources are allocated. > > Signed-off-by: Michael Jamet <michael.jamet@intel.com> > --- > drivers/pci/pci.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/pci.h | 2 ++ > drivers/pci/probe.c | 3 ++- > drivers/pci/setup-bus.c | 4 ++-- > include/linux/pci.h | 2 ++ > 5 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 625a4ac..41c2619 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -198,6 +198,62 @@ static int __pci_bus_find_cap_start(struct pci_bus *bus, > } > > /** > + * pci_is_thunderbolt_device - checks PCI device is Thunderbolt-based. > + * The only existing way is to check the device id is allocated to Thunderbolt. > + * @dev: the PCI device structure to check against > + * > + * Returns true if the PCI device is of the Thunderbolt type. > + */ > +bool pci_is_thunderbolt_device(struct pci_dev *dev) > +{ > + if ((dev->vendor == PCI_VENDOR_ID_INTEL) && > + ((dev->device == 0xcaca) > + || (dev->device == 0x1513) > + || (dev->device == 0xcbca) > + || (dev->device == 0x151A) > + || (dev->device == 0x151B) > + || (dev->device == 0x1549) > + || (dev->device == 0x1547) > + || (dev->device == 0x1548) > + || (dev->device == 0x1566) > + || (dev->device == 0x1567) > + || (dev->device == 0x1569) > + || (dev->device == 0x1568) > + || (dev->device == 0x156A) > + || (dev->device == 0x156B) > + || (dev->device == 0x156C) > + || (dev->device == 0x156D) > + || (dev->device == 0x1579) > + || (dev->device == 0x157A) > + || (dev->device == 0x157D) > + || (dev->device == 0x157E))) > + return true; > + > + return false; > +} > +EXPORT_SYMBOL(pci_is_thunderbolt_device); > + > +/** > + * pci_is_connected_to_thunderbolt - check if connected to a Thunderbolt chain. > + * @dev: the PCI device structure to check against > + * > + * Returns true if the PCI device s connected is connected to a > + * Thunderbolt-based in the chain. > + */ > +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev) > +{ > + struct pci_dev *bridge; > + > + if (pci_is_thunderbolt_device(dev)) > + return true; > + bridge = pci_upstream_bridge(dev); > + if (bridge) > + return pci_is_connected_to_thunderbolt(bridge); > + return false; > +} > +EXPORT_SYMBOL(pci_is_connected_to_thunderbolt); > + > +/** > * pci_find_capability - query for devices' capabilities > * @dev: PCI device to query > * @cap: capability code > @@ -1285,6 +1341,14 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) > if (atomic_inc_return(&dev->enable_cnt) > 1) > return 0; /* already enabled */ > > + /* > + * if IO resource have been requested, but the device is connected > + * to Thunderbolt chain, don't allocate IO resource > + */ > + if ((flags & IORESOURCE_IO) > + && pci_is_connected_to_thunderbolt(dev)) > + flags &= ~IORESOURCE_IO; > + > bridge = pci_upstream_bridge(dev); > if (bridge) > pci_enable_bridge(bridge); > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 0601890..fb9a3b1 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -4,6 +4,8 @@ > #define PCI_CFG_SPACE_SIZE 256 > #define PCI_CFG_SPACE_EXP_SIZE 4096 > > +bool pci_is_thunderbolt_device(struct pci_dev *dev); > + > extern const unsigned char pcie_link_speed[]; > > /* Functions internal to the PCI core code */ > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 5ed9930..d8171af 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -131,7 +131,8 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar) > > if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) { > flags = bar & ~PCI_BASE_ADDRESS_IO_MASK; > - flags |= IORESOURCE_IO; > + if (!pci_is_connected_to_thunderbolt(dev)) > + flags |= IORESOURCE_IO; > return flags; > } > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 0482235..79ac38f 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -663,12 +663,12 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) > b_res[1].flags |= IORESOURCE_MEM; > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > - if (!io) { > + if (!io || pci_is_thunderbolt_device(bridge)) { > pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); > pci_read_config_word(bridge, PCI_IO_BASE, &io); > pci_write_config_word(bridge, PCI_IO_BASE, 0x0); > } > - if (io) > + if (io && !pci_is_thunderbolt_device(bridge)) > b_res[0].flags |= IORESOURCE_IO; > > /* DECchip 21050 pass 2 errata: the bridge may miss an address > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 2a8c405..09ddc2c 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -931,6 +931,8 @@ int __must_check pci_enable_device_io(struct pci_dev *dev); > int __must_check pci_enable_device_mem(struct pci_dev *dev); > int __must_check pci_reenable_device(struct pci_dev *); > int __must_check pcim_enable_device(struct pci_dev *pdev); > + > +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev); > void pcim_pin_device(struct pci_dev *pdev); > > static inline int pci_is_enabled(struct pci_dev *pdev) > -- > 1.9.1 > > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: [] >> To prevent this, we detect a chain contains a Thunderbolt >> device by checking the Thunderbolt PCI device id. > > I'm really not happy about checking a list of device IDs to identify > Thunderbolt devices. Surely there's a better way, because a list like > this has to be updated regularly. I recently proposed internally to use quirks (pci_fixup_early) for that, but apparently Michael didn't have time to answer. It might be he can just comment here since the patch already public.
On Wed, Nov 12, 2014 at 7:30 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > [] > >>> To prevent this, we detect a chain contains a Thunderbolt >>> device by checking the Thunderbolt PCI device id. >> >> I'm really not happy about checking a list of device IDs to identify >> Thunderbolt devices. Surely there's a better way, because a list like >> this has to be updated regularly. > > I recently proposed internally to use quirks (pci_fixup_early) for > that, but apparently Michael didn't have time to answer. It might be > he can just comment here since the patch already public. In any case: this will interfere with thunderbolt hotplug on Apple systems, where we do not have BIOS support and have to handle hotplug events and assign resources ourselves. So please add a DMI check for Apple (the reverse of what we do in http://lxr.free-electrons.com/source/drivers/thunderbolt/nhi.c?v=3.17#L664 ). Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Wednesday, November 12, 2014 19:29 > To: Jamet, Michael > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Levy, Amir > (Jer); Alloun, Dan; Rafael Wysocki; Andreas Noever > Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O > resources. > > [+cc Rafael, Andreas] > > On Wed, Nov 12, 2014 at 7:52 PM, Michael Jamet > <michael.jamet@intel.com> wrote: > > Every Thunderbolt-based devices or Thunderbolt-connected devices > > should not allocate PCI I/O resources per Thunderbolt specs. > > Please include a pointer to those specs in the changelog. > Unfortunately these specs are not publically available. > > On a Thunderbolt PC, BIOS is responsible to allocate IO resources. > > Kernel shouldn't allocate the PCI I/O resources as it interferes with > > BIOS operation. > > Doing this may cause the devices in the Thunderbolt chain not being > > detected or added, or worse to stuck the Thunderbolt Host controller. > > These new kernel/firmware coordination requirements need to be > documented. If they're already part of a PCIe ECN or PCI firmware spec, just > provide a pointer. > Same, this refers to same specs. > > To prevent this, we detect a chain contains a Thunderbolt device by > > checking the Thunderbolt PCI device id. > > I'm really not happy about checking a list of device IDs to identify > Thunderbolt devices. Surely there's a better way, because a list like this has > to be updated regularly. > > Bjorn > This was also discussed internally and the only way to identify Thunderbolt devices is to check the device IDs. As you said, this will require us to maintain and keep the list up-to-date as we deliver new devices. Michael > > The validation is carried out at the pci_enable_device_flags() > > function that checks the PCI device or bridge is Thunderbolt chained > > and avoid IO resources allocation. > > > > In addition, decode_bar() and pci_bridge_check_ranges() function has > > been internally checked for Thunderbolt as well to ensure no IO > > resources are allocated. > > > > Signed-off-by: Michael Jamet <michael.jamet@intel.com> > > --- > > drivers/pci/pci.c | 64 > +++++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/pci/pci.h | 2 ++ > > drivers/pci/probe.c | 3 ++- > > drivers/pci/setup-bus.c | 4 ++-- > > include/linux/pci.h | 2 ++ > > 5 files changed, 72 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index > > 625a4ac..41c2619 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -198,6 +198,62 @@ static int __pci_bus_find_cap_start(struct > > pci_bus *bus, } > > > > /** > > + * pci_is_thunderbolt_device - checks PCI device is Thunderbolt-based. > > + * The only existing way is to check the device id is allocated to > Thunderbolt. > > + * @dev: the PCI device structure to check against > > + * > > + * Returns true if the PCI device is of the Thunderbolt type. > > + */ > > +bool pci_is_thunderbolt_device(struct pci_dev *dev) { > > + if ((dev->vendor == PCI_VENDOR_ID_INTEL) && > > + ((dev->device == 0xcaca) > > + || (dev->device == 0x1513) > > + || (dev->device == 0xcbca) > > + || (dev->device == 0x151A) > > + || (dev->device == 0x151B) > > + || (dev->device == 0x1549) > > + || (dev->device == 0x1547) > > + || (dev->device == 0x1548) > > + || (dev->device == 0x1566) > > + || (dev->device == 0x1567) > > + || (dev->device == 0x1569) > > + || (dev->device == 0x1568) > > + || (dev->device == 0x156A) > > + || (dev->device == 0x156B) > > + || (dev->device == 0x156C) > > + || (dev->device == 0x156D) > > + || (dev->device == 0x1579) > > + || (dev->device == 0x157A) > > + || (dev->device == 0x157D) > > + || (dev->device == 0x157E))) > > + return true; > > + > > + return false; > > +} > > +EXPORT_SYMBOL(pci_is_thunderbolt_device); > > + > > +/** > > + * pci_is_connected_to_thunderbolt - check if connected to a > Thunderbolt chain. > > + * @dev: the PCI device structure to check against > > + * > > + * Returns true if the PCI device s connected is connected to a > > + * Thunderbolt-based in the chain. > > + */ > > +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev) { > > + struct pci_dev *bridge; > > + > > + if (pci_is_thunderbolt_device(dev)) > > + return true; > > + bridge = pci_upstream_bridge(dev); > > + if (bridge) > > + return pci_is_connected_to_thunderbolt(bridge); > > + return false; > > +} > > +EXPORT_SYMBOL(pci_is_connected_to_thunderbolt); > > + > > +/** > > * pci_find_capability - query for devices' capabilities > > * @dev: PCI device to query > > * @cap: capability code > > @@ -1285,6 +1341,14 @@ static int pci_enable_device_flags(struct pci_dev > *dev, unsigned long flags) > > if (atomic_inc_return(&dev->enable_cnt) > 1) > > return 0; /* already enabled */ > > > > + /* > > + * if IO resource have been requested, but the device is connected > > + * to Thunderbolt chain, don't allocate IO resource > > + */ > > + if ((flags & IORESOURCE_IO) > > + && pci_is_connected_to_thunderbolt(dev)) > > + flags &= ~IORESOURCE_IO; > > + > > bridge = pci_upstream_bridge(dev); > > if (bridge) > > pci_enable_bridge(bridge); diff --git > > a/drivers/pci/pci.h b/drivers/pci/pci.h index 0601890..fb9a3b1 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -4,6 +4,8 @@ > > #define PCI_CFG_SPACE_SIZE 256 > > #define PCI_CFG_SPACE_EXP_SIZE 4096 > > > > +bool pci_is_thunderbolt_device(struct pci_dev *dev); > > + > > extern const unsigned char pcie_link_speed[]; > > > > /* Functions internal to the PCI core code */ diff --git > > a/drivers/pci/probe.c b/drivers/pci/probe.c index 5ed9930..d8171af > > 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -131,7 +131,8 @@ static inline unsigned long decode_bar(struct > > pci_dev *dev, u32 bar) > > > > if ((bar & PCI_BASE_ADDRESS_SPACE) == > PCI_BASE_ADDRESS_SPACE_IO) { > > flags = bar & ~PCI_BASE_ADDRESS_IO_MASK; > > - flags |= IORESOURCE_IO; > > + if (!pci_is_connected_to_thunderbolt(dev)) > > + flags |= IORESOURCE_IO; > > return flags; > > } > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index > > 0482235..79ac38f 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -663,12 +663,12 @@ static void pci_bridge_check_ranges(struct > pci_bus *bus) > > b_res[1].flags |= IORESOURCE_MEM; > > > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > > - if (!io) { > > + if (!io || pci_is_thunderbolt_device(bridge)) { > > pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); > > pci_read_config_word(bridge, PCI_IO_BASE, &io); > > pci_write_config_word(bridge, PCI_IO_BASE, 0x0); > > } > > - if (io) > > + if (io && !pci_is_thunderbolt_device(bridge)) > > b_res[0].flags |= IORESOURCE_IO; > > > > /* DECchip 21050 pass 2 errata: the bridge may miss an > > address diff --git a/include/linux/pci.h b/include/linux/pci.h index > > 2a8c405..09ddc2c 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -931,6 +931,8 @@ int __must_check pci_enable_device_io(struct > > pci_dev *dev); int __must_check pci_enable_device_mem(struct pci_dev > > *dev); int __must_check pci_reenable_device(struct pci_dev *); int > > __must_check pcim_enable_device(struct pci_dev *pdev); > > + > > +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev); > > void pcim_pin_device(struct pci_dev *pdev); > > > > static inline int pci_is_enabled(struct pci_dev *pdev) > > -- > > 1.9.1 > > > > --------------------------------------------------------------------- > > Intel Israel (74) Limited > > > > This e-mail and any attachments may contain confidential material for > > the sole use of the intended recipient(s). Any review or distribution > > by others is strictly prohibited. If you are not the intended > > recipient, please contact the sender and delete all copies. > > --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBBbmR5IFNoZXZjaGVua28gW21h aWx0bzphbmR5LnNoZXZjaGVua29AZ21haWwuY29tXQ0KPiBTZW50OiBXZWRuZXNkYXksIE5vdmVt YmVyIDEyLCAyMDE0IDIwOjMxDQo+IFRvOiBCam9ybiBIZWxnYWFzDQo+IENjOiBKYW1ldCwgTWlj aGFlbDsgbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsgbGludXgta2VybmVsQHZnZXIua2VybmVs Lm9yZzsNCj4gTGV2eSwgQW1pciAoSmVyKTsgQWxsb3VuLCBEYW47IFJhZmFlbCBXeXNvY2tpOyBB bmRyZWFzIE5vZXZlcg0KPiBTdWJqZWN0OiBSZTogW1BBVENIXSBwY2k6IHN1cHBvcnQgVGh1bmRl cmJvbHQgcmVxdWlyZW1lbnRzIGZvciBJL08NCj4gcmVzb3VyY2VzLg0KPiANCj4gT24gV2VkLCBO b3YgMTIsIDIwMTQgYXQgNzoyOSBQTSwgQmpvcm4gSGVsZ2FhcyA8YmhlbGdhYXNAZ29vZ2xlLmNv bT4NCj4gd3JvdGU6DQo+IA0KPiBbXQ0KPiANCj4gPj4gVG8gcHJldmVudCB0aGlzLCB3ZSBkZXRl Y3QgYSBjaGFpbiBjb250YWlucyBhIFRodW5kZXJib2x0IGRldmljZSBieQ0KPiA+PiBjaGVja2lu ZyB0aGUgVGh1bmRlcmJvbHQgUENJIGRldmljZSBpZC4NCj4gPg0KPiA+IEknbSByZWFsbHkgbm90 IGhhcHB5IGFib3V0IGNoZWNraW5nIGEgbGlzdCBvZiBkZXZpY2UgSURzIHRvIGlkZW50aWZ5DQo+ ID4gVGh1bmRlcmJvbHQgZGV2aWNlcy4gIFN1cmVseSB0aGVyZSdzIGEgYmV0dGVyIHdheSwgYmVj YXVzZSBhIGxpc3QgbGlrZQ0KPiA+IHRoaXMgaGFzIHRvIGJlIHVwZGF0ZWQgcmVndWxhcmx5Lg0K PiANCj4gSSByZWNlbnRseSBwcm9wb3NlZCBpbnRlcm5hbGx5IHRvIHVzZSBxdWlya3MgKHBjaV9m aXh1cF9lYXJseSkgZm9yIHRoYXQsIGJ1dA0KPiBhcHBhcmVudGx5IE1pY2hhZWwgZGlkbid0IGhh dmUgdGltZSB0byBhbnN3ZXIuIEl0IG1pZ2h0IGJlIGhlIGNhbiBqdXN0DQo+IGNvbW1lbnQgaGVy ZSBzaW5jZSB0aGUgcGF0Y2ggYWxyZWFkeSBwdWJsaWMuDQo+IA0KPiAtLQ0KPiBXaXRoIEJlc3Qg UmVnYXJkcywNCj4gQW5keSBTaGV2Y2hlbmtvDQoNCkluZGVlZCwgSSd2ZSBleHBsb3JlZCB0aGUg cXVpcmtzIG9wdGlvbi4NClVuZm9ydHVuYXRlbHkgdGhlIGZpeHVwIGhvb2sgc2VlbXMgdG8gYmUg Y2FsbGVkIHRvbyBsYXRlIGluIHRoZSBjb2RlIA0Kc2luY2UgcGNpX2VuYWJsZV9kZXZpY2UoKWNh bGxzIHBjaWJpb3NfZW5hYmxlX2RldmljZSgpIHdoaWNoIGFjdHVhbGx5IA0Kc2VuZCBhIFBDSSBj b21tYW5kIHdpdGggIENNRCB8PVBDSV9DT01NQU5EX0lPIGFuZCBjb25maWd1cmUgdGhlIFBDSSBI Vy4gDQpUaGUgSU9SRVNPVVJDRV9NRU0gYW5kIElPUkVTT1VSQ0VfSU8gZmxhZ3MgYXJlIHNldCBh dCB0aGUgdGltZSB0aGUNCnBjaV9lbmFibGVfZGV2aWNlKCksIHBjaV9lbmFibGVfZGV2aWNlX21l bSgpIG9yIHBjaV9lbmFibGVfZGV2aWNlX2lvKCkgDQppcyBjYWxsZWQgZHVyaW5nIGRldmljZSBp bml0aWFsaXphdGlvbnMsIHNvIGFuIGVhcmx5IGZpeHVwIGNhbGwgd29uJ3QgaGVscCBlaXRoZXIu DQpBdCB0aGlzIHN0YWdlLCB0aGUgUENJIEhXIGlzIGNvbmZpZ3VyZWQgYW5kIGEgZml4dXAgY2Fs bCB3b24ndCAgcmVzb2x2ZSB0aGUgaXNzdWUNCnVubGVzcyBhbm90aGVyIFBDSSBjb21tYW5kIGlz IHNlbnQgdG8gcmV2ZXJ0IGl0IA0KKGdlbmVyYWxseSBub3QgYWR2aXNlZCB0byBleGVjdXRlIGFn YWluIG9uIGhhcmR3YXJlKS4NCg0KTWljaGFlbCANCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpJbnRlbCBJc3JhZWwg KDc0KSBMaW1pdGVkCgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRzIG1heSBjb250YWlu IGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUgdXNlIG9mIHRoZSBpbnRlbmRlZCBy ZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCmJ5IG90aGVycyBpcyBzdHJp Y3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQKcmVjaXBpZW50LCBw bGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFuZCBkZWxldGUgYWxsIGNvcGllcy4K -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Andreas Noever [mailto:andreas.noever@gmail.com] > Sent: Wednesday, November 12, 2014 22:19 > To: Andy Shevchenko > Cc: Bjorn Helgaas; Jamet, Michael; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; Levy, Amir (Jer); Alloun, Dan; Rafael Wysocki > Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O > resources. > > On Wed, Nov 12, 2014 at 7:30 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <bhelgaas@google.com> > wrote: > > > > [] > > > >>> To prevent this, we detect a chain contains a Thunderbolt device by > >>> checking the Thunderbolt PCI device id. > >> > >> I'm really not happy about checking a list of device IDs to identify > >> Thunderbolt devices. Surely there's a better way, because a list > >> like this has to be updated regularly. > > > > I recently proposed internally to use quirks (pci_fixup_early) for > > that, but apparently Michael didn't have time to answer. It might be > > he can just comment here since the patch already public. > > In any case: this will interfere with thunderbolt hotplug on Apple systems, > where we do not have BIOS support and have to handle hotplug events and > assign resources ourselves. So please add a DMI check for Apple (the reverse > of what we do in > http://lxr.free- > electrons.com/source/drivers/thunderbolt/nhi.c?v=3.17#L664 > ). > > Thanks, > Andreas This is correct that the hotplug handling mechanism and interaction with BIOS is different. However, this patch also applies for any case, since the I/O space is limited and need to be preserved, so we must prevent allocation of I/O resources from the devices and avoiding exhaustion while chaining them. Michael --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Tue, Nov 18, 2014 at 9:20 AM, Jamet, Michael <michael.jamet@intel.com> wrote: >> -----Original Message----- >> From: Andreas Noever [mailto:andreas.noever@gmail.com] >> Sent: Wednesday, November 12, 2014 22:19 >> To: Andy Shevchenko >> Cc: Bjorn Helgaas; Jamet, Michael; linux-pci@vger.kernel.org; linux- >> kernel@vger.kernel.org; Levy, Amir (Jer); Alloun, Dan; Rafael Wysocki >> Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O >> resources. >> >> On Wed, Nov 12, 2014 at 7:30 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >> > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <bhelgaas@google.com> >> wrote: >> > >> > [] >> > >> >>> To prevent this, we detect a chain contains a Thunderbolt device by >> >>> checking the Thunderbolt PCI device id. >> >> >> >> I'm really not happy about checking a list of device IDs to identify >> >> Thunderbolt devices. Surely there's a better way, because a list >> >> like this has to be updated regularly. >> > >> > I recently proposed internally to use quirks (pci_fixup_early) for >> > that, but apparently Michael didn't have time to answer. It might be >> > he can just comment here since the patch already public. >> >> In any case: this will interfere with thunderbolt hotplug on Apple systems, >> where we do not have BIOS support and have to handle hotplug events and >> assign resources ourselves. So please add a DMI check for Apple (the reverse >> of what we do in >> http://lxr.free- >> electrons.com/source/drivers/thunderbolt/nhi.c?v=3.17#L664 >> ). >> >> Thanks, >> Andreas > > This is correct that the hotplug handling mechanism and interaction with BIOS is different. > However, this patch also applies for any case, since the I/O space is limited > and need to be preserved, so we must prevent allocation of I/O resources > from the devices and avoiding exhaustion while chaining them. Now I am slightly confused. Does the BIOS (on non Apple hardware) leave I/O resources always unassigned? Your first post seemed to imply that it does assign some and that we should not interfere. In that case we would have to assign them on Apple systems ourselves. On the other hand if no TB device uses I/O resources and the BIOS never assigns them, then why do devices fail if we exhaust them? Andreas > Michael > > > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 12:57 AM, Jamet, Michael <michael.jamet@intel.com> wrote: >> -----Original Message----- >> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >> Sent: Wednesday, November 12, 2014 19:29 >> To: Jamet, Michael >> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Levy, Amir >> (Jer); Alloun, Dan; Rafael Wysocki; Andreas Noever >> Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O >> resources. >> >> [+cc Rafael, Andreas] >> >> On Wed, Nov 12, 2014 at 7:52 PM, Michael Jamet >> <michael.jamet@intel.com> wrote: >> > Every Thunderbolt-based devices or Thunderbolt-connected devices >> > should not allocate PCI I/O resources per Thunderbolt specs. >> >> Please include a pointer to those specs in the changelog. >> > > Unfortunately these specs are not publically available. > >> > On a Thunderbolt PC, BIOS is responsible to allocate IO resources. >> > Kernel shouldn't allocate the PCI I/O resources as it interferes with >> > BIOS operation. >> > Doing this may cause the devices in the Thunderbolt chain not being >> > detected or added, or worse to stuck the Thunderbolt Host controller. >> >> These new kernel/firmware coordination requirements need to be >> documented. If they're already part of a PCIe ECN or PCI firmware spec, just >> provide a pointer. >> > > Same, this refers to same specs. > >> > To prevent this, we detect a chain contains a Thunderbolt device by >> > checking the Thunderbolt PCI device id. >> >> I'm really not happy about checking a list of device IDs to identify >> Thunderbolt devices. Surely there's a better way, because a list like this has >> to be updated regularly. >> >> Bjorn >> > > This was also discussed internally and the only way to identify Thunderbolt devices is to check the device IDs. > As you said, this will require us to maintain and keep the list up-to-date as we deliver new devices. I don't really see how this can work. You're asking me to put changes based on a secret spec into generic code that is used on every machine with PCI. I have no way to maintain something like that. This seems like a major screw up in the design and documentation of Thunderbolt. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > This was also discussed internally and the only way to identify Thunderbolt devices is to check the device IDs. > > As you said, this will require us to maintain and keep the list up-to-date as we deliver new devices. > > I don't really see how this can work. You're asking me to put changes > based on a secret spec into generic code that is used on every machine > with PCI. I have no way to maintain something like that. > > This seems like a major screw up in the design and documentation of Thunderbolt. See https://developer.apple.com/library/mac/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/ThunderboltDevGuide.pdf page 10 for one of the brief public notes on the not relying on I/O space. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 2:17 AM, One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote: >> > This was also discussed internally and the only way to identify Thunderbolt devices is to check the device IDs. >> > As you said, this will require us to maintain and keep the list up-to-date as we deliver new devices. >> >> I don't really see how this can work. You're asking me to put changes >> based on a secret spec into generic code that is used on every machine >> with PCI. I have no way to maintain something like that. > >> >> This seems like a major screw up in the design and documentation of Thunderbolt. > > See > https://developer.apple.com/library/mac/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/ThunderboltDevGuide.pdf > > page 10 for one of the brief public notes on the not relying on I/O space. I agree with that recommendation not to rely on I/O space. It applies equally to *all* PCI devices, not just to Thunderbolt. Presumably this patch fixes a problem. The changelog says: Kernel shouldn't allocate the PCI I/O resources as it interferes with BIOS operation. Doing this may cause the devices in the Thunderbolt chain not being detected or added, or worse to stuck the Thunderbolt Host controller. The problem of devices not being detected sounds like a general problem (I assume the problem is actually that we do enumerate the device, but we may not be able to assign I/O port space to it, which means we may not be able to operate it). This could happen with any device. If you can come up with a generic way to deal with it, that might work. Note that we do already have pci_enable_device_mem() for drivers that don't need I/O space to operate their device. If assigning I/O port space to a device can hang the Thunderbolt controller, that sounds like a controller defect, and maybe you could write a quirk to work around it. I'm not opposed to adding device-specific workarounds for things like that. I just have trouble with putting undocumented workarounds in the common path that everybody uses. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBCam9ybiBIZWxnYWFzIFttYWls dG86YmhlbGdhYXNAZ29vZ2xlLmNvbV0NCj4gU2VudDogV2VkbmVzZGF5LCBEZWNlbWJlciAwMywg MjAxNCAwMjoxOQ0KPiBUbzogT25lIFRob3VzYW5kIEdub21lcw0KPiBDYzogSmFtZXQsIE1pY2hh ZWw7IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWtlcm5lbEB2Z2VyLmtlcm5lbC5v cmc7DQo+IExldnksIEFtaXIgKEplcik7IEFsbG91biwgRGFuOyBSYWZhZWwgV3lzb2NraTsgQW5k cmVhcyBOb2V2ZXINCj4gU3ViamVjdDogUmU6IFtQQVRDSF0gcGNpOiBzdXBwb3J0IFRodW5kZXJi b2x0IHJlcXVpcmVtZW50cyBmb3IgSS9PIHJlc291cmNlcy4NCj4gDQo+IE9uIE1vbiwgTm92IDI0 LCAyMDE0IGF0IDI6MTcgQU0sIE9uZSBUaG91c2FuZCBHbm9tZXMNCj4gPGdub21lc0BseG9yZ3Vr LnVrdXUub3JnLnVrPiB3cm90ZToNCj4gPj4gPiBUaGlzIHdhcyBhbHNvIGRpc2N1c3NlZCBpbnRl cm5hbGx5IGFuZCB0aGUgb25seSB3YXkgdG8gaWRlbnRpZnkgVGh1bmRlcmJvbHQNCj4gZGV2aWNl cyBpcyB0byBjaGVjayB0aGUgZGV2aWNlIElEcy4NCj4gPj4gPiBBcyB5b3Ugc2FpZCwgdGhpcyB3 aWxsIHJlcXVpcmUgdXMgdG8gbWFpbnRhaW4gYW5kIGtlZXAgdGhlIGxpc3QgdXAtdG8tZGF0ZSBh cw0KPiB3ZSBkZWxpdmVyIG5ldyBkZXZpY2VzLg0KPiA+Pg0KPiA+PiBJIGRvbid0IHJlYWxseSBz ZWUgaG93IHRoaXMgY2FuIHdvcmsuICBZb3UncmUgYXNraW5nIG1lIHRvIHB1dA0KPiA+PiBjaGFu Z2VzIGJhc2VkIG9uIGEgc2VjcmV0IHNwZWMgaW50byBnZW5lcmljIGNvZGUgdGhhdCBpcyB1c2Vk IG9uDQo+ID4+IGV2ZXJ5IG1hY2hpbmUgd2l0aCBQQ0kuICBJIGhhdmUgbm8gd2F5IHRvIG1haW50 YWluIHNvbWV0aGluZyBsaWtlIHRoYXQuDQo+ID4NCj4gPj4NCj4gPj4gVGhpcyBzZWVtcyBsaWtl IGEgbWFqb3Igc2NyZXcgdXAgaW4gdGhlIGRlc2lnbiBhbmQgZG9jdW1lbnRhdGlvbiBvZg0KPiBU aHVuZGVyYm9sdC4NCj4gPg0KPiA+IFNlZQ0KPiA+IGh0dHBzOi8vZGV2ZWxvcGVyLmFwcGxlLmNv bS9saWJyYXJ5L21hYy9kb2N1bWVudGF0aW9uL0hhcmR3YXJlRHJpdmVycy8NCj4gPiBDb25jZXB0 dWFsL1RodW5kZXJib2x0RGV2R3VpZGUvVGh1bmRlcmJvbHREZXZHdWlkZS5wZGYNCj4gPg0KPiA+ IHBhZ2UgMTAgZm9yIG9uZSBvZiB0aGUgYnJpZWYgcHVibGljIG5vdGVzIG9uIHRoZSBub3QgcmVs eWluZyBvbiBJL08gc3BhY2UuDQo+IA0KPiBJIGFncmVlIHdpdGggdGhhdCByZWNvbW1lbmRhdGlv biBub3QgdG8gcmVseSBvbiBJL08gc3BhY2UuICBJdCBhcHBsaWVzIGVxdWFsbHkNCj4gdG8gKmFs bCogUENJIGRldmljZXMsIG5vdCBqdXN0IHRvIFRodW5kZXJib2x0Lg0KPiANCj4gUHJlc3VtYWJs eSB0aGlzIHBhdGNoIGZpeGVzIGEgcHJvYmxlbS4gIFRoZSBjaGFuZ2Vsb2cgc2F5czoNCj4gDQo+ ICAgICBLZXJuZWwgc2hvdWxkbid0IGFsbG9jYXRlIHRoZSBQQ0kgSS9PIHJlc291cmNlcw0KPiAg ICAgYXMgaXQgaW50ZXJmZXJlcyB3aXRoIEJJT1Mgb3BlcmF0aW9uLg0KPiAgICAgRG9pbmcgdGhp cyBtYXkgY2F1c2UgdGhlIGRldmljZXMgaW4gdGhlIFRodW5kZXJib2x0IGNoYWluDQo+ICAgICBu b3QgYmVpbmcgZGV0ZWN0ZWQgb3IgYWRkZWQsIG9yIHdvcnNlIHRvIHN0dWNrIHRoZQ0KPiAgICAg VGh1bmRlcmJvbHQgSG9zdCBjb250cm9sbGVyLg0KPiANCj4gVGhlIHByb2JsZW0gb2YgZGV2aWNl cyBub3QgYmVpbmcgZGV0ZWN0ZWQgc291bmRzIGxpa2UgYSBnZW5lcmFsIHByb2JsZW0gKEkNCj4g YXNzdW1lIHRoZSBwcm9ibGVtIGlzIGFjdHVhbGx5IHRoYXQgd2UgZG8gZW51bWVyYXRlIHRoZSBk ZXZpY2UsIGJ1dCB3ZSBtYXkNCj4gbm90IGJlIGFibGUgdG8gYXNzaWduIEkvTyBwb3J0IHNwYWNl IHRvIGl0LCB3aGljaCBtZWFucyB3ZSBtYXkgbm90IGJlIGFibGUgdG8NCj4gb3BlcmF0ZSBpdCku ICBUaGlzIGNvdWxkIGhhcHBlbiB3aXRoIGFueSBkZXZpY2UuICBJZiB5b3UgY2FuIGNvbWUgdXAg d2l0aCBhDQo+IGdlbmVyaWMgd2F5IHRvIGRlYWwgd2l0aCBpdCwgdGhhdCBtaWdodCB3b3JrLiAg Tm90ZSB0aGF0IHdlIGRvIGFscmVhZHkgaGF2ZQ0KPiBwY2lfZW5hYmxlX2RldmljZV9tZW0oKSBm b3IgZHJpdmVycyB0aGF0IGRvbid0IG5lZWQgSS9PIHNwYWNlIHRvIG9wZXJhdGUNCj4gdGhlaXIg ZGV2aWNlLg0KPiANCj4gSWYgYXNzaWduaW5nIEkvTyBwb3J0IHNwYWNlIHRvIGEgZGV2aWNlIGNh biBoYW5nIHRoZSBUaHVuZGVyYm9sdCBjb250cm9sbGVyLCB0aGF0DQo+IHNvdW5kcyBsaWtlIGEg Y29udHJvbGxlciBkZWZlY3QsIGFuZCBtYXliZSB5b3UgY291bGQgd3JpdGUgYSBxdWlyayB0byB3 b3JrDQo+IGFyb3VuZCBpdC4gIEknbSBub3Qgb3Bwb3NlZCB0byBhZGRpbmcgZGV2aWNlLXNwZWNp ZmljIHdvcmthcm91bmRzIGZvciB0aGluZ3MNCj4gbGlrZSB0aGF0LiAgSSBqdXN0IGhhdmUgdHJv dWJsZSB3aXRoIHB1dHRpbmcgdW5kb2N1bWVudGVkIHdvcmthcm91bmRzIGluIHRoZQ0KPiBjb21t b24gcGF0aCB0aGF0IGV2ZXJ5Ym9keSB1c2VzLg0KPiANCj4gQmpvcm4NCg0KVGhlIGFjdHVhbCBw cm9ibGVtIHRoZSBwYXRjaCBpcyBtZWFudCB0byBhZGRyZXNzIGlzIHJlbGF0ZWQgdG8gdGhlIFBD SSANCmxpbWl0YXRpb24gb2YgNjRLQiBJL08gc3BhY2UgYW5kIHRoZSBmYWN0IHRoYXQgYWxsb2Nh dGlvbnMgYXJlIA0KcGVyZm9ybWVkIGluIGNodW5rcyBvZiA0S0IgYmVoaW5kIFBDSS10by1QQ0kg YnJpZGdlcy4NCkFmdGVyIGEgY2VydGFpbiBhbW91bnQgb2YgZGV2aWNlcyB0aGUgSS9PIHJlc291 cmNlcyBtYXkgYmUgZXhoYXVzdGVkLiANClRoaXMgaXMgaW5kZWVkIGEgZ2VuZXJhbCBpc3N1ZSwg bm90IG9ubHkgcmVsYXRlZCB0byBUaHVuZGVyYm9sdCwgYW5kIA0KYXMgQmpvcm4gc3VnZ2VzdGVk IGEgZ2VuZXJhbCBmaXggaXMgYWR2aXNlZC4NCiANCkZvbGxvd2luZyBmdXJ0aGVyIGludmVzdGln YXRpb25zIGl0IHNlZW1zIHRoYXQgd2UgbWF5IG5vdCByZWFsbHkgbmVlZCANCnRoaXMgcGF0Y2gg Zm9yIHRoZSBmb2xsb3dpbmcgcmVhc29uczoNCg0KMS4gSXQgc2VlbXMgdGhhdCB0aGUgY29udHJv bGxlciBpc3N1ZXMgKHdyaXR0ZW4gb24gdGhlIGNoYW5nZWxvZykgd2VyZSANCnJlbGF0ZWQgdG8g dGhlIGxhY2sgb2Ygc3VwcG9ydCBvZiBob3RwbHVnIHRvcG9sb2d5IGNoYW5nZXMgaW4gdGhlIG9s ZCANCnRlc3RlZCBrZXJuZWwgKDMuMTUpIHJhdGhlciB0aGFuIHRoZSBJL08gc3BhY2UgaXNzdWUu DQoNCjIuIEtlcm5lbCBQQ0kgZHJpdmVyIGhhbmRsZXMgcHJvcGVybHkgYWxsb2NhdGlvbnMgb2Yg SS9PIHJlc291cmNlcyBhbmQgDQpwcmV2ZW50cyBJL08gcmVzb3VyY2VzIGV4aGF1c3Rpb24gd2hp Y2ggbWF5IGhhdmUgY2F1c2VkIGlzc3VlcyAoaWYgbm90IA0KaGFuZGxlZCBwcm9wZXJseSkgdG8g YW55IFBDSSBvciBUaHVuZGVyYm9sdCBjb250cm9sbGVyLiBMb29rcyBsaWtlIA0KZmlyc3QgZGV2 aWNlIGFza2luZyBJL08gaXMgdGhlIGZpcnN0IHNlcnZlZCBhbmQgdGhpcyBtZWNoYW5pc20gd29y a3MgZmluZS4NCg0KMy4gRm9yIG1vc3QgZGV2aWNlcyBzdXBwb3J0ZWQgdG9kYXkgb24gVGh1bmRl cmJvbHQgc3VjaCBhcyBBSENJIG9yICANCmlHQiAoRTEwMDBFKSwgaWYgbm8gSS9PIHJlc291cmNl cyBpcyBhbGxvY2F0ZWQsIHRoZSBkZXZpY2Ugc2VlbXMgdG8gDQpjb250aW51ZSB0byBvcGVyYXRl IHRocm91Z2ggdGhlIG1lbW9yeSBCQVJzLg0KIA0KL01pY2hhZWwNCi0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpJbnRl bCBJc3JhZWwgKDc0KSBMaW1pdGVkCgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRzIG1h eSBjb250YWluIGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUgdXNlIG9mIHRoZSBp bnRlbmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCmJ5IG90aGVy cyBpcyBzdHJpY3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQKcmVj aXBpZW50LCBwbGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFuZCBkZWxldGUgYWxsIGNvcGllcy4K -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Andreas Noever [mailto:andreas.noever@gmail.com] > Sent: Tuesday, November 18, 2014 17:48 > To: Jamet, Michael > Cc: Andy Shevchenko; Bjorn Helgaas; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org; Levy, Amir (Jer); Alloun, Dan; Rafael Wysocki > Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. > > On Tue, Nov 18, 2014 at 9:20 AM, Jamet, Michael <michael.jamet@intel.com> > wrote: > >> -----Original Message----- > >> From: Andreas Noever [mailto:andreas.noever@gmail.com] > >> Sent: Wednesday, November 12, 2014 22:19 > >> To: Andy Shevchenko > >> Cc: Bjorn Helgaas; Jamet, Michael; linux-pci@vger.kernel.org; linux- > >> kernel@vger.kernel.org; Levy, Amir (Jer); Alloun, Dan; Rafael Wysocki > >> Subject: Re: [PATCH] pci: support Thunderbolt requirements for I/O > >> resources. > >> > >> On Wed, Nov 12, 2014 at 7:30 PM, Andy Shevchenko > >> <andy.shevchenko@gmail.com> wrote: > >> > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas > >> > <bhelgaas@google.com> > >> wrote: > >> > > >> > [] > >> > > >> >>> To prevent this, we detect a chain contains a Thunderbolt device > >> >>> by checking the Thunderbolt PCI device id. > >> >> > >> >> I'm really not happy about checking a list of device IDs to > >> >> identify Thunderbolt devices. Surely there's a better way, > >> >> because a list like this has to be updated regularly. > >> > > >> > I recently proposed internally to use quirks (pci_fixup_early) for > >> > that, but apparently Michael didn't have time to answer. It might > >> > be he can just comment here since the patch already public. > >> > >> In any case: this will interfere with thunderbolt hotplug on Apple > >> systems, where we do not have BIOS support and have to handle hotplug > >> events and assign resources ourselves. So please add a DMI check for > >> Apple (the reverse of what we do in > >> http://lxr.free- > >> electrons.com/source/drivers/thunderbolt/nhi.c?v=3.17#L664 > >> ). > >> > >> Thanks, > >> Andreas > > > > This is correct that the hotplug handling mechanism and interaction with > BIOS is different. > > However, this patch also applies for any case, since the I/O space is > > limited and need to be preserved, so we must prevent allocation of I/O > > resources from the devices and avoiding exhaustion while chaining them. > > Now I am slightly confused. Does the BIOS (on non Apple hardware) leave I/O > resources always unassigned? Your first post seemed to imply that it does > assign some and that we should not interfere. In that case we would have to > assign them on Apple systems ourselves. On the other hand if no TB device uses > I/O resources and the BIOS never assigns them, then why do devices fail if we > exhaust them? > > Andreas Correct. The BIOS may play a role in allocation of memory and I/O resources but most of the case will leave the I/O resources unassigned. Some legacy Thunderbolt devices are designed with I/O BARs, so they need to be handled with care as written in the other email thread to Bjorn. /Michael --------------------------------------------------------------------- Intel Israel (74) Limited This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 625a4ac..41c2619 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -198,6 +198,62 @@ static int __pci_bus_find_cap_start(struct pci_bus *bus, } /** + * pci_is_thunderbolt_device - checks PCI device is Thunderbolt-based. + * The only existing way is to check the device id is allocated to Thunderbolt. + * @dev: the PCI device structure to check against + * + * Returns true if the PCI device is of the Thunderbolt type. + */ +bool pci_is_thunderbolt_device(struct pci_dev *dev) +{ + if ((dev->vendor == PCI_VENDOR_ID_INTEL) && + ((dev->device == 0xcaca) + || (dev->device == 0x1513) + || (dev->device == 0xcbca) + || (dev->device == 0x151A) + || (dev->device == 0x151B) + || (dev->device == 0x1549) + || (dev->device == 0x1547) + || (dev->device == 0x1548) + || (dev->device == 0x1566) + || (dev->device == 0x1567) + || (dev->device == 0x1569) + || (dev->device == 0x1568) + || (dev->device == 0x156A) + || (dev->device == 0x156B) + || (dev->device == 0x156C) + || (dev->device == 0x156D) + || (dev->device == 0x1579) + || (dev->device == 0x157A) + || (dev->device == 0x157D) + || (dev->device == 0x157E))) + return true; + + return false; +} +EXPORT_SYMBOL(pci_is_thunderbolt_device); + +/** + * pci_is_connected_to_thunderbolt - check if connected to a Thunderbolt chain. + * @dev: the PCI device structure to check against + * + * Returns true if the PCI device s connected is connected to a + * Thunderbolt-based in the chain. + */ +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev) +{ + struct pci_dev *bridge; + + if (pci_is_thunderbolt_device(dev)) + return true; + bridge = pci_upstream_bridge(dev); + if (bridge) + return pci_is_connected_to_thunderbolt(bridge); + return false; +} +EXPORT_SYMBOL(pci_is_connected_to_thunderbolt); + +/** * pci_find_capability - query for devices' capabilities * @dev: PCI device to query * @cap: capability code @@ -1285,6 +1341,14 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) if (atomic_inc_return(&dev->enable_cnt) > 1) return 0; /* already enabled */ + /* + * if IO resource have been requested, but the device is connected + * to Thunderbolt chain, don't allocate IO resource + */ + if ((flags & IORESOURCE_IO) + && pci_is_connected_to_thunderbolt(dev)) + flags &= ~IORESOURCE_IO; + bridge = pci_upstream_bridge(dev); if (bridge) pci_enable_bridge(bridge); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 0601890..fb9a3b1 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -4,6 +4,8 @@ #define PCI_CFG_SPACE_SIZE 256 #define PCI_CFG_SPACE_EXP_SIZE 4096 +bool pci_is_thunderbolt_device(struct pci_dev *dev); + extern const unsigned char pcie_link_speed[]; /* Functions internal to the PCI core code */ diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 5ed9930..d8171af 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -131,7 +131,8 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar) if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) { flags = bar & ~PCI_BASE_ADDRESS_IO_MASK; - flags |= IORESOURCE_IO; + if (!pci_is_connected_to_thunderbolt(dev)) + flags |= IORESOURCE_IO; return flags; } diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 0482235..79ac38f 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -663,12 +663,12 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) b_res[1].flags |= IORESOURCE_MEM; pci_read_config_word(bridge, PCI_IO_BASE, &io); - if (!io) { + if (!io || pci_is_thunderbolt_device(bridge)) { pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); pci_read_config_word(bridge, PCI_IO_BASE, &io); pci_write_config_word(bridge, PCI_IO_BASE, 0x0); } - if (io) + if (io && !pci_is_thunderbolt_device(bridge)) b_res[0].flags |= IORESOURCE_IO; /* DECchip 21050 pass 2 errata: the bridge may miss an address diff --git a/include/linux/pci.h b/include/linux/pci.h index 2a8c405..09ddc2c 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -931,6 +931,8 @@ int __must_check pci_enable_device_io(struct pci_dev *dev); int __must_check pci_enable_device_mem(struct pci_dev *dev); int __must_check pci_reenable_device(struct pci_dev *); int __must_check pcim_enable_device(struct pci_dev *pdev); + +bool pci_is_connected_to_thunderbolt(struct pci_dev *dev); void pcim_pin_device(struct pci_dev *pdev); static inline int pci_is_enabled(struct pci_dev *pdev)
Every Thunderbolt-based devices or Thunderbolt-connected devices should not allocate PCI I/O resources per Thunderbolt specs. On a Thunderbolt PC, BIOS is responsible to allocate IO resources. Kernel shouldn't allocate the PCI I/O resources as it interferes with BIOS operation. Doing this may cause the devices in the Thunderbolt chain not being detected or added, or worse to stuck the Thunderbolt Host controller. To prevent this, we detect a chain contains a Thunderbolt device by checking the Thunderbolt PCI device id. The validation is carried out at the pci_enable_device_flags() function that checks the PCI device or bridge is Thunderbolt chained and avoid IO resources allocation. In addition, decode_bar() and pci_bridge_check_ranges() function has been internally checked for Thunderbolt as well to ensure no IO resources are allocated. Signed-off-by: Michael Jamet <michael.jamet@intel.com> --- drivers/pci/pci.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci.h | 2 ++ drivers/pci/probe.c | 3 ++- drivers/pci/setup-bus.c | 4 ++-- include/linux/pci.h | 2 ++ 5 files changed, 72 insertions(+), 3 deletions(-)