linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 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

* [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-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-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 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  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

* 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

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).