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 <[email protected]>
---
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.
[+cc Rafael, Andreas]
On Wed, Nov 12, 2014 at 7:52 PM, Michael Jamet <[email protected]> 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 <[email protected]>
> ---
> 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.
>
On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <[email protected]> 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
On Wed, Nov 12, 2014 at 7:30 PM, Andy Shevchenko
<[email protected]> wrote:
> On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <[email protected]> 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
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Wednesday, November 12, 2014 19:29
> To: Jamet, Michael
> Cc: [email protected]; [email protected]; 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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Andy Shevchenko [mailto:[email protected]]
> Sent: Wednesday, November 12, 2014 20:31
> To: Bjorn Helgaas
> Cc: Jamet, Michael; [email protected]; [email protected];
> 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 <[email protected]>
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Andreas Noever [mailto:[email protected]]
> Sent: Wednesday, November 12, 2014 22:19
> To: Andy Shevchenko
> Cc: Bjorn Helgaas; Jamet, Michael; [email protected]; linux-
> [email protected]; 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
> <[email protected]> wrote:
> > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <[email protected]>
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Nov 18, 2014 at 9:20 AM, Jamet, Michael <[email protected]> wrote:
>> -----Original Message-----
>> From: Andreas Noever [mailto:[email protected]]
>> Sent: Wednesday, November 12, 2014 22:19
>> To: Andy Shevchenko
>> Cc: Bjorn Helgaas; Jamet, Michael; [email protected]; linux-
>> [email protected]; 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
>> <[email protected]> wrote:
>> > On Wed, Nov 12, 2014 at 7:29 PM, Bjorn Helgaas <[email protected]>
>> 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.
On Tue, Nov 18, 2014 at 12:57 AM, Jamet, Michael
<[email protected]> wrote:
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[email protected]]
>> Sent: Wednesday, November 12, 2014 19:29
>> To: Jamet, Michael
>> Cc: [email protected]; [email protected]; 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
>> <[email protected]> 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
> > 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.
On Mon, Nov 24, 2014 at 2:17 AM, One Thousand Gnomes
<[email protected]> 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