* [PATCH] pci: support Thunderbolt requirements for I/O resources. @ 2014-11-13 2:52 Michael Jamet 2014-11-12 17:29 ` Bjorn Helgaas 0 siblings, 1 reply; 13+ messages in thread From: Michael Jamet @ 2014-11-13 2:52 UTC (permalink / raw) To: bhelgaas Cc: linux-pci, linux-kernel, amir.jer.levy, dan.alloun, michael.jamet 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(-) 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-13 2:52 [PATCH] pci: support Thunderbolt requirements for I/O resources Michael Jamet @ 2014-11-12 17:29 ` Bjorn Helgaas 2014-11-12 18:30 ` Andy Shevchenko 2014-11-18 7:57 ` Jamet, Michael 0 siblings, 2 replies; 13+ messages in thread From: Bjorn Helgaas @ 2014-11-12 17:29 UTC (permalink / raw) To: Michael Jamet Cc: linux-pci, linux-kernel, amir.jer.levy, dan.alloun, Rafael Wysocki, Andreas Noever [+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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-12 17:29 ` Bjorn Helgaas @ 2014-11-12 18:30 ` Andy Shevchenko 2014-11-12 20:19 ` Andreas Noever 2014-11-18 8:15 ` Jamet, Michael 2014-11-18 7:57 ` Jamet, Michael 1 sibling, 2 replies; 13+ messages in thread From: Andy Shevchenko @ 2014-11-12 18:30 UTC (permalink / raw) To: Bjorn Helgaas Cc: Michael Jamet, linux-pci, linux-kernel, amir.jer.levy, dan.alloun, Rafael Wysocki, Andreas Noever 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-12 18:30 ` Andy Shevchenko @ 2014-11-12 20:19 ` Andreas Noever 2014-11-18 8:20 ` Jamet, Michael 2014-11-18 8:15 ` Jamet, Michael 1 sibling, 1 reply; 13+ messages in thread From: Andreas Noever @ 2014-11-12 20:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Bjorn Helgaas, Michael Jamet, linux-pci, linux-kernel, amir.jer.levy, dan.alloun, Rafael Wysocki 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-12 20:19 ` Andreas Noever @ 2014-11-18 8:20 ` Jamet, Michael 2014-11-18 15:47 ` Andreas Noever 0 siblings, 1 reply; 13+ messages in thread From: Jamet, Michael @ 2014-11-18 8:20 UTC (permalink / raw) To: Andreas Noever, Andy Shevchenko Cc: Bjorn Helgaas, linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2288 bytes --] > -----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. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-18 8:20 ` Jamet, Michael @ 2014-11-18 15:47 ` Andreas Noever 2015-01-18 14:16 ` Jamet, Michael 0 siblings, 1 reply; 13+ messages in thread From: Andreas Noever @ 2014-11-18 15:47 UTC (permalink / raw) To: Jamet, Michael Cc: Andy Shevchenko, Bjorn Helgaas, linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-18 15:47 ` Andreas Noever @ 2015-01-18 14:16 ` Jamet, Michael 0 siblings, 0 replies; 13+ messages in thread From: Jamet, Michael @ 2015-01-18 14:16 UTC (permalink / raw) To: Andreas Noever Cc: Andy Shevchenko, Bjorn Helgaas, linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Mika Westerberg [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3590 bytes --] > -----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. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-12 18:30 ` Andy Shevchenko 2014-11-12 20:19 ` Andreas Noever @ 2014-11-18 8:15 ` Jamet, Michael 1 sibling, 0 replies; 13+ messages in thread From: Jamet, Michael @ 2014-11-18 8:15 UTC (permalink / raw) To: Andy Shevchenko, Bjorn Helgaas Cc: linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2219 bytes --] > -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Wednesday, November 12, 2014 20:31 > To: Bjorn Helgaas > Cc: Jamet, Michael; 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. > > 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. > > -- > With Best Regards, > Andy Shevchenko Indeed, I've explored the quirks option. Unfortunately the fixup hook seems to be called too late in the code since pci_enable_device()calls pcibios_enable_device() which actually send a PCI command with CMD |=PCI_COMMAND_IO and configure the PCI HW. The IORESOURCE_MEM and IORESOURCE_IO flags are set at the time the pci_enable_device(), pci_enable_device_mem() or pci_enable_device_io() is called during device initializations, so an early fixup call won't help either. At this stage, the PCI HW is configured and a fixup call won't resolve the issue unless another PCI command is sent to revert it (generally not advised to execute again on hardware). 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. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-12 17:29 ` Bjorn Helgaas 2014-11-12 18:30 ` Andy Shevchenko @ 2014-11-18 7:57 ` Jamet, Michael 2014-11-18 15:58 ` Bjorn Helgaas 1 sibling, 1 reply; 13+ messages in thread From: Jamet, Michael @ 2014-11-18 7:57 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 9354 bytes --] > -----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. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-18 7:57 ` Jamet, Michael @ 2014-11-18 15:58 ` Bjorn Helgaas 2014-11-24 9:17 ` One Thousand Gnomes 0 siblings, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2014-11-18 15:58 UTC (permalink / raw) To: Jamet, Michael Cc: linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-18 15:58 ` Bjorn Helgaas @ 2014-11-24 9:17 ` One Thousand Gnomes 2014-12-03 0:19 ` Bjorn Helgaas 0 siblings, 1 reply; 13+ messages in thread From: One Thousand Gnomes @ 2014-11-24 9:17 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jamet, Michael, linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever > > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-11-24 9:17 ` One Thousand Gnomes @ 2014-12-03 0:19 ` Bjorn Helgaas 2015-01-18 14:15 ` Jamet, Michael 0 siblings, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2014-12-03 0:19 UTC (permalink / raw) To: One Thousand Gnomes Cc: Jamet, Michael, linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] pci: support Thunderbolt requirements for I/O resources. 2014-12-03 0:19 ` Bjorn Helgaas @ 2015-01-18 14:15 ` Jamet, Michael 0 siblings, 0 replies; 13+ messages in thread From: Jamet, Michael @ 2015-01-18 14:15 UTC (permalink / raw) To: Bjorn Helgaas, One Thousand Gnomes Cc: linux-pci, linux-kernel, Levy, Amir (Jer), Alloun, Dan, Rafael Wysocki, Andreas Noever, Mika Westerberg [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4241 bytes --] > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Wednesday, December 03, 2014 02:19 > To: One Thousand Gnomes > Cc: Jamet, Michael; 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. > > 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 The actual problem the patch is meant to address is related to the PCI limitation of 64KB I/O space and the fact that allocations are performed in chunks of 4KB behind PCI-to-PCI bridges. After a certain amount of devices the I/O resources may be exhausted. This is indeed a general issue, not only related to Thunderbolt, and as Bjorn suggested a general fix is advised. Following further investigations it seems that we may not really need this patch for the following reasons: 1. It seems that the controller issues (written on the changelog) were related to the lack of support of hotplug topology changes in the old tested kernel (3.15) rather than the I/O space issue. 2. Kernel PCI driver handles properly allocations of I/O resources and prevents I/O resources exhaustion which may have caused issues (if not handled properly) to any PCI or Thunderbolt controller. Looks like first device asking I/O is the first served and this mechanism works fine. 3. For most devices supported today on Thunderbolt such as AHCI or iGB (E1000E), if no I/O resources is allocated, the device seems to continue to operate through the memory BARs. /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. ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-01-18 14:16 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-13 2:52 [PATCH] pci: support Thunderbolt requirements for I/O resources Michael Jamet 2014-11-12 17:29 ` Bjorn Helgaas 2014-11-12 18:30 ` Andy Shevchenko 2014-11-12 20:19 ` Andreas Noever 2014-11-18 8:20 ` Jamet, Michael 2014-11-18 15:47 ` Andreas Noever 2015-01-18 14:16 ` Jamet, Michael 2014-11-18 8:15 ` Jamet, Michael 2014-11-18 7:57 ` Jamet, Michael 2014-11-18 15:58 ` Bjorn Helgaas 2014-11-24 9:17 ` One Thousand Gnomes 2014-12-03 0:19 ` Bjorn Helgaas 2015-01-18 14:15 ` Jamet, Michael
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).