RE: [PATCH] pci: support Thunderbolt requirements for I/O resources.

[Jamet, Michael]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx]
> Sent: Wednesday, November 12, 2014 19:29
> To: Jamet, Michael
> Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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.