Thunderbolt security levels and NVM firmware upgrade

These functions should not (and do not) modify the argument in any way so make it const.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

At least Falcon Ridge when in host mode does not have any kind of DROM available and reading DROM offset returns 0 for these. Do not try to read DROM any further in that case.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/eeprom.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/thunderbolt/eeprom.c +0 -1

        /* read uid */
        res = tb_eeprom_read_n(sw, drom_offset, data, 9);
        if (res)
--

2.11.0

Quoted 11 lines · Mika Westerberg

At least Falcon Ridge when in host mode does not have any kind of DROM available and reading DROM offset returns 0 for these. Do not try to read DROM any further in that case.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/eeprom.c | 3 +++
 1 file changed, 3 insertions(+)

Hi Mika,

nice work, it is nice to see Intel contribute to the Thunderbolt driver (I can second Lukas's 'jaw drop' comment)!

I will try to read through everything today, but maybe the last few patches will get pushed back to next weekend.

drivers/thunderbolt/eeprom.c int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid) · line 281

Quoted 11 lines

drivers/thunderbolt/eeprom.c +3 -0

index 6392990c984d..e4e64b130514 100644
@@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
        if (res)
                return res;
 
+       if (drom_offset == 0)
+               return -ENODEV;
+

I think that this will make tb_switch_resume bail out on the root switch, which is not good. Since the uid is only used to detect whether a different device was plugged in while the system was suspended I think that we can safely ignore the uid on the root switch:
- don't read it in tb_drom_read (route == 0 is already special cased anyways)
- add a special case for the root switch to tb_switch_resume and
don't read the uid - just assume that it did not change (should be impossible anyways)

What do you think?

Quoted 14 lines · Andreas Noever

Quoted 11 lines · Mika Westerberg

At least Falcon Ridge when in host mode does not have any kind of DROM available and reading DROM offset returns 0 for these. Do not try to read DROM any further in that case.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/eeprom.c | 3 +++
 1 file changed, 3 insertions(+)

Quoted 7 lines

Hi Mika,

nice work, it is nice to see Intel contribute to the Thunderbolt driver (I can second Lukas's 'jaw drop' comment)!

I will try to read through everything today, but maybe the last few patches will get pushed back to next weekend.

Thanks :)

Quoted 21 lines

drivers/thunderbolt/eeprom.c int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid) · line 281

Quoted 11 lines

drivers/thunderbolt/eeprom.c +3 -0

index 6392990c984d..e4e64b130514 100644
@@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
        if (res)
                return res;
 
+       if (drom_offset == 0)
+               return -ENODEV;
+

I think that this will make tb_switch_resume bail out on the root switch, which is not good. Since the uid is only used to detect whether a different device was plugged in while the system was suspended I think that we can safely ignore the uid on the root switch:
- don't read it in tb_drom_read (route == 0 is already special cased anyways)
- add a special case for the root switch to tb_switch_resume and
don't read the uid - just assume that it did not change (should be impossible anyways)

Quoted 1 line

What do you think?

I think there actually is such check already in tb_switch_resume() where we special case the root switch ignoring its UID. Unless I'm missing something.

I'm testing this on a Mac with Cactus Ridge and the root switch resume does not fail :)

Yes there is a check for the root switch, but also one that checks the return code of tb_drom_read_uid_only :)

err = tb_drom_read_uid_only(sw, &uid);

if (err) {
    tb_sw_warn(sw, "uid read failed\n");
    return err;
}

if (sw != sw->tb->root_switch && sw->uid != uid) {

The reason it works on the Mac is because drom_offset is not 0, so the new branch in tb_drom_read_uid_only is not taken. Probably this is the case for all Cactus Ridge models and Alpine Ridge doesn't go there since it uses the ICM?

Still it wouldn't hurt to only read the uid if
sw != root_switch, the value is not used if sw == root_switch.

Quoted 8 lines · Andreas Noever

Yes there is a check for the root switch, but also one that checks the return code of tb_drom_read_uid_only :)

err = tb_drom_read_uid_only(sw, &uid);

if (err) {
    tb_sw_warn(sw, "uid read failed\n");
    return err;
}

Quoted 7 lines

if (sw != sw->tb->root_switch && sw->uid != uid) {

The reason it works on the Mac is because drom_offset is not 0, so the new branch in tb_drom_read_uid_only is not taken. Probably this is the case for all Cactus Ridge models and Alpine Ridge doesn't go there since it uses the ICM?

Yes in case of ICM we don't call the function at all.

Quoted 2 lines

Still it wouldn't hurt to only read the uid if
sw != root_switch, the value is not used if sw == root_switch.

I agree. I'll update the code so that it will only read and check UID when we are not dealing with the root switch.

Intel Thunderbolt controllers support up to 16 MSI-X vectors. Using

MSI-X is preferred over MSI or legacy interrupt and may bring additional performance because there is no need to check the status registers which interrupt was triggered.

While there we convert comments in structs tb_ring and tb_nhi to follow kernel-doc format more closely.

This code is based on the work done by Amir Levy and Michael Jamet.

Signed-off-by Michael Jamet <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

---
 drivers/thunderbolt/ctl.c      |   4 +-
 drivers/thunderbolt/nhi.c      | 159 ++++++++++++++++++++++++++++++++++++-----
 drivers/thunderbolt/nhi.h      |  56 +++++++++++----
 drivers/thunderbolt/nhi_regs.h |   9 +++
 4 files changed, 196 insertions(+), 32 deletions(-)

drivers/thunderbolt/ctl.c +2 -2

index 1031d97407a8..889a32dd21e7 100644
@@ -488,11 +488,11 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data)
        if (!ctl->frame_pool)
                goto err;
 
-       ctl->tx = ring_alloc_tx(nhi, 0, 10);
+       ctl->tx = ring_alloc_tx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->tx)
                goto err;
 
-       ctl->rx = ring_alloc_rx(nhi, 0, 10);
+       ctl->rx = ring_alloc_rx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->rx)
                goto err;
 

drivers/thunderbolt/nhi.c +6 -0

index a8c20413dbda..17f3b1bdb7da 100644
@@ -21,6 +21,12 @@
 
 #define RING_TYPE(ring) ((ring)->is_tx ? "TX ring" : "RX ring")
 
+/*
+ * Minimal number of vectors when we use MSI-X. Two for control channel
+ * Rx/Tx and the rest four are for cross domain DMA paths.
+ */
+#define MSIX_MIN_VECS          6
+#define MSIX_MAX_VECS          16
 
 static int ring_interrupt_index(struct tb_ring *ring)
 {

@@ -239,8 +245,82 @@ int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)
        return ret;
 }
 

drivers/thunderbolt/nhi.c +78 -5

+       u32 step, shift, ivr, misc;
+       int index;
+
+       if (ring->irq <= 0)
+               return;
+
+       if (ring->is_tx)
+               index = ring->hop;
+       else
+               index = ring->hop + ring->nhi->hop_count;
+
+       /*
+        * Ask the hardware to clear interrupt status bits automatically
+        * since we already know which interrupt was triggered.
+        */
+       misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
+       if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
+               misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
+               iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
+       }
+
+       step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
+       shift = index % REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
+       ivr = ioread32(ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE + step);
+       ivr &= ~(REG_INT_VEC_ALLOC_MASK << shift);
+       if (map)
+               ivr |= ring->vector << shift;
+       iowrite32(ivr, ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE + step);
+}
+
+static int ring_request_msix(struct tb_ring *ring, bool no_suspend)
+{
+       struct tb_nhi *nhi = ring->nhi;
+       unsigned long irqflags;
+       int ret;
+
+       if (!nhi->pdev->msix_enabled)
+               return 0;
+
+       ret = ida_simple_get(&nhi->msix_ida, 0, MSIX_MAX_VECS, GFP_KERNEL);
+       if (ret < 0)
+               return ret;
+
+       ring->vector = ret;
+
+       ring->irq = pci_irq_vector(ring->nhi->pdev, ring->vector);
+       if (ring->irq < 0)
+               return ring->irq;
+
+       irqflags = no_suspend ? IRQF_NO_SUSPEND : 0;
+       return request_irq(ring->irq, ring_msix, irqflags, "thunderbolt", ring);
+}
+
+static void ring_release_msix(struct tb_ring *ring)
+{
+       if (ring->irq <= 0)
+               return;
+
+       free_irq(ring->irq, ring);
+       ida_simple_remove(&ring->nhi->msix_ida, ring->vector);
+       ring->vector = 0;
+       ring->irq = 0;
+}
+
 static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
-                                 bool transmit)
+                                 bool transmit, unsigned int flags)
 {
        struct tb_ring *ring = NULL;
        dev_info(&nhi->pdev->dev, "allocating %s ring %d of size %d\n",

@@ -271,9 +351,14 @@ static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
        ring->hop = hop;
        ring->is_tx = transmit;
        ring->size = size;
+       ring->flags = flags;
        ring->head = 0;
        ring->tail = 0;
        ring->running = false;
+
+       if (ring_request_msix(ring, flags & RING_FLAG_NO_SUSPEND))
+               goto err;
+
        ring->descriptors = dma_alloc_coherent(&ring->nhi->pdev->dev,
                        size * sizeof(*ring->descriptors),
                        &ring->descriptors_dma, GFP_KERNEL | __GFP_ZERO);

@@ -295,14 +380,16 @@ static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
        return NULL;
 }
 
-struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, true);
+       return ring_alloc(nhi, hop, size, true, flags);
 }
 
-struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, false);
+       return ring_alloc(nhi, hop, size, false, flags);
 }
 
 /**

@@ -334,6 +421,7 @@ void ring_start(struct tb_ring *ring)
                ring_iowrite32options(ring,
                                      RING_FLAG_ENABLE | RING_FLAG_RAW, 0);
        }
+       ring_map_unmap_msix(ring, true);
        ring_interrupt_active(ring, true);
        ring->running = true;
 err:

@@ -366,6 +454,7 @@ void ring_stop(struct tb_ring *ring)
                goto err;
        }
        ring_interrupt_active(ring, false);
+       ring_map_unmap_msix(ring, false);
 
        ring_iowrite32options(ring, 0, 0);
        ring_iowrite64desc(ring, 0, 0);

drivers/thunderbolt/nhi.c +19 -1

@@ -528,9 +619,51 @@ static void nhi_shutdown(struct tb_nhi *nhi)
         * We have to release the irq before calling flush_work. Otherwise an
         * already executing IRQ handler could call schedule_work again.
         */
-       devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
+       if (!nhi->pdev->msix_enabled)
+               devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
        flush_work(&nhi->interrupt_work);
        mutex_destroy(&nhi->lock);
+       ida_destroy(&nhi->msix_ida);
+}
+
+static int nhi_init_msi(struct tb_nhi *nhi)
+{
+       struct pci_dev *pdev = nhi->pdev;
+       int res, irq, nvec;
+
+       /* In case someone left them on. */
+       nhi_disable_interrupts(nhi);
+
+       ida_init(&nhi->msix_ida);
+
+       /*
+        * The NHI has 16 MSI-X vectors or a single MSI. We first try to
+        * get all MSI-X vectors and if we succeed, each ring will have
+        * one MSI-X. If for some reason that does not work out, we

drivers/thunderbolt/nhi.c +16 -12

+
+               irq = pci_irq_vector(nhi->pdev, 0);
+               if (irq < 0)
+                       return irq;
+
+               res = devm_request_irq(&pdev->dev, irq, nhi_msi,
+                                      IRQF_NO_SUSPEND, "thunderbolt", nhi);
+               if (res) {
+                       dev_err(&pdev->dev, "request_irq failed, aborting\n");
+                       return res;
+               }
+       }
+
+       return 0;
 }
 
 static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)

@@ -545,12 +678,6 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
                return res;
        }
 
-       res = pci_enable_msi(pdev);
-       if (res) {
-               dev_err(&pdev->dev, "cannot enable MSI, aborting\n");
-               return res;
-       }
-
        res = pcim_iomap_regions(pdev, 1 << 0, "thunderbolt");
        if (res) {
                dev_err(&pdev->dev, "cannot obtain PCI resources, aborting\n");

@@ -568,7 +695,6 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (nhi->hop_count != 12 && nhi->hop_count != 32)
                dev_warn(&pdev->dev, "unexpected hop count: %d\n",
                         nhi->hop_count);
-       INIT_WORK(&nhi->interrupt_work, nhi_interrupt_work);
 
        nhi->tx_rings = devm_kcalloc(&pdev->dev, nhi->hop_count,
                                     sizeof(*nhi->tx_rings), GFP_KERNEL);

@@ -577,12 +703,9 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
        if (!nhi->tx_rings || !nhi->rx_rings)
                return -ENOMEM;
 
-       nhi_disable_interrupts(nhi); /* In case someone left them on. */
-       res = devm_request_irq(&pdev->dev, pdev->irq, nhi_msi,
-                              IRQF_NO_SUSPEND, /* must work during _noirq */
-                              "thunderbolt", nhi);
+       res = nhi_init_msi(nhi);
        if (res) {
-               dev_err(&pdev->dev, "request_irq failed, aborting\n");
+               dev_err(&pdev->dev, "cannot enable MSI, aborting\n");
                return res;
        }
 

drivers/thunderbolt/nhi.h +44 -12

index 317242939b31..630f44140530 100644
@@ -7,45 +7,75 @@
 #ifndef DSL3510_H_
 #define DSL3510_H_
 
+#include <linux/idr.h>
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 
 /**
  * struct tb_nhi - thunderbolt native host interface
+ * @lock: Must be held during ring creation/destruction. Is acquired by
+ *       interrupt_work when dispatching interrupts to individual rings.
+ * @pdev: Pointer to the PCI device
+ * @iobase: MMIO space of the NHI
+ * @tx_rings: All Tx rings available on this host controller
+ * @rx_rings: All Rx rings available on this host controller
+ * @msix_ida: Used to allocate MSI-X vectors for rings
+ * @interrupt_work: Work scheduled to handle ring interrupt when no
+ *                 MSI-X is used.
+ * @hop_count: Number of rings (end point hops) supported by NHI.
  */
 struct tb_nhi {
-       struct mutex lock; /*
-                           * Must be held during ring creation/destruction.
-                           * Is acquired by interrupt_work when dispatching
-                           * interrupts to individual rings.
-                           **/
+       struct mutex lock;
        struct pci_dev *pdev;
        void __iomem *iobase;
        struct tb_ring **tx_rings;
        struct tb_ring **rx_rings;
+       struct ida msix_ida;
        struct work_struct interrupt_work;
-       u32 hop_count; /* Number of rings (end point hops) supported by NHI. */
+       u32 hop_count;
 };
 
 /**
  * struct tb_ring - thunderbolt TX or RX ring associated with a NHI
+ * @lock: Lock serializing actions to this ring. Must be acquired after
+ *       nhi->lock.
+ * @nhi: Pointer to the native host controller interface
+ * @size: Size of the ring
+ * @hop: Hop (DMA channel) associated with this ring
+ * @head: Head of the ring (write next descriptor here)
+ * @tail: Tail of the ring (complete next descriptor here)
+ * @descriptors: Allocated descriptors for this ring
+ * @queue: Queue holding frames to be transferred over this ring
+ * @in_flight: Queue holding frames that are currently in flight
+ * @work: Interrupt work structure
+ * @is_tx: Is the ring Tx or Rx
+ * @running: Is the ring running
+ * @irq: MSI-X irq number if the ring uses MSI-X. %0 otherwise.
+ * @vector: MSI-X vector number the ring uses (only set if @irq is > 0)
+ * @flags: Ring specific flags
  */
 struct tb_ring {
-       struct mutex lock; /* must be acquired after nhi->lock */
+       struct mutex lock;
        struct tb_nhi *nhi;
        int size;
        int hop;
-       int head; /* write next descriptor here */
-       int tail; /* complete next descriptor here */
+       int head;
+       int tail;
        struct ring_desc *descriptors;
        dma_addr_t descriptors_dma;
        struct list_head queue;
        struct list_head in_flight;
        struct work_struct work;
-       bool is_tx:1; /* rx otherwise */
+       bool is_tx:1;
        bool running:1;
+       int irq;
+       u8 vector;
+       unsigned int flags;
 };
 
+/* Leave ring interrupt enabled on suspend */
+#define RING_FLAG_NO_SUSPEND   BIT(0)
+
 struct ring_frame;
 typedef void (*ring_cb)(struct tb_ring*, struct ring_frame*, bool canceled);
 

@@ -64,8 +94,10 @@ struct ring_frame {
 
 #define TB_FRAME_SIZE 0x100    /* minimum size for ring_rx */
 
-struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size);
-struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size);
+struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags);
+struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags);
 void ring_start(struct tb_ring *ring);
 void ring_stop(struct tb_ring *ring);
 void ring_free(struct tb_ring *ring);

drivers/thunderbolt/nhi_regs.h +9 -1

index 75cf0691e6c5..48b98d3c7e6a 100644
@@ -95,7 +95,16 @@ struct ring_desc {
 #define REG_RING_INTERRUPT_BASE        0x38200
 #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
 
+/* Interrupt Vector Allocation */
+#define REG_INT_VEC_ALLOC_BASE 0x38c40
+#define REG_INT_VEC_ALLOC_BITS 4
+#define REG_INT_VEC_ALLOC_MASK GENMASK(3, 0)
+#define REG_INT_VEC_ALLOC_REGS (32 / REG_INT_VEC_ALLOC_BITS)
+
 /* The last 11 bits contain the number of hops supported by the NHI port. */
 #define REG_HOP_COUNT          0x39640
 
+#define REG_DMA_MISC                   0x39864
+#define REG_DMA_MISC_INT_AUTO_CLEAR     BIT(2)
+
 #endif
--

2.11.0

Quoted 1 line · Mika Westerberg

Intel Thunderbolt controllers support up to 16 MSI-X vectors. Using

Is that true for all generations? If so can we remove the legacy path?

Quoted 58 lines

MSI-X is preferred over MSI or legacy interrupt and may bring additional performance because there is no need to check the status registers which interrupt was triggered.

While there we convert comments in structs tb_ring and tb_nhi to follow kernel-doc format more closely.

This code is based on the work done by Amir Levy and Michael Jamet.

Signed-off-by Michael Jamet <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

---
 drivers/thunderbolt/ctl.c      |   4 +-
 drivers/thunderbolt/nhi.c      | 159 ++++++++++++++++++++++++++++++++++++-----
 drivers/thunderbolt/nhi.h      |  56 +++++++++++----
 drivers/thunderbolt/nhi_regs.h |   9 +++
 4 files changed, 196 insertions(+), 32 deletions(-)

drivers/thunderbolt/ctl.c +2 -2

index 1031d97407a8..889a32dd21e7 100644
@@ -488,11 +488,11 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi, hotplug_cb cb, void *cb_data)
        if (!ctl->frame_pool)
                goto err;
 
-       ctl->tx = ring_alloc_tx(nhi, 0, 10);
+       ctl->tx = ring_alloc_tx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->tx)
                goto err;
 
-       ctl->rx = ring_alloc_rx(nhi, 0, 10);
+       ctl->rx = ring_alloc_rx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->rx)
                goto err;
 

drivers/thunderbolt/nhi.c +6 -0

index a8c20413dbda..17f3b1bdb7da 100644
@@ -21,6 +21,12 @@
 
 #define RING_TYPE(ring) ((ring)->is_tx ? "TX ring" : "RX ring")
 
+/*
+ * Minimal number of vectors when we use MSI-X. Two for control channel
+ * Rx/Tx and the rest four are for cross domain DMA paths.
+ */
+#define MSIX_MIN_VECS          6
+#define MSIX_MAX_VECS          16
 
 static int ring_interrupt_index(struct tb_ring *ring)
 {

@@ -239,8 +245,82 @@ int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)
        return ret;
 }
 

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame) · line 257

Quoted 10 lines

drivers/thunderbolt/nhi.c +10 -0

+static irqreturn_t ring_msix(int irq, void *data)
+{
+       struct tb_ring *ring = data;
+
+       schedule_work(&ring->work);
+       return IRQ_HANDLED;
+}
+
+static void ring_map_unmap_msix(struct tb_ring *ring, bool map)
+{

This function does the same as ring_interrupt_active just for msix, right? Can we rename one (or both) to express that?
For example rename ring_interrput_active -> ring_map_unmap_msi. Or just roll this one into ring_interrupt_active and do the right thing internally.

Quoted 126 lines

drivers/thunderbolt/nhi.c +78 -5

+       u32 step, shift, ivr, misc;
+       int index;
+
+       if (ring->irq <= 0)
+               return;
+
+       if (ring->is_tx)
+               index = ring->hop;
+       else
+               index = ring->hop + ring->nhi->hop_count;
+
+       /*
+        * Ask the hardware to clear interrupt status bits automatically
+        * since we already know which interrupt was triggered.
+        */
+       misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
+       if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
+               misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
+               iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
+       }
+
+       step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
+       shift = index % REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
+       ivr = ioread32(ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE + step);
+       ivr &= ~(REG_INT_VEC_ALLOC_MASK << shift);
+       if (map)
+               ivr |= ring->vector << shift;
+       iowrite32(ivr, ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE + step);
+}
+
+static int ring_request_msix(struct tb_ring *ring, bool no_suspend)
+{
+       struct tb_nhi *nhi = ring->nhi;
+       unsigned long irqflags;
+       int ret;
+
+       if (!nhi->pdev->msix_enabled)
+               return 0;
+
+       ret = ida_simple_get(&nhi->msix_ida, 0, MSIX_MAX_VECS, GFP_KERNEL);
+       if (ret < 0)
+               return ret;
+
+       ring->vector = ret;
+
+       ring->irq = pci_irq_vector(ring->nhi->pdev, ring->vector);
+       if (ring->irq < 0)
+               return ring->irq;
+
+       irqflags = no_suspend ? IRQF_NO_SUSPEND : 0;
+       return request_irq(ring->irq, ring_msix, irqflags, "thunderbolt", ring);
+}
+
+static void ring_release_msix(struct tb_ring *ring)
+{
+       if (ring->irq <= 0)
+               return;
+
+       free_irq(ring->irq, ring);
+       ida_simple_remove(&ring->nhi->msix_ida, ring->vector);
+       ring->vector = 0;
+       ring->irq = 0;
+}
+
 static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
-                                 bool transmit)
+                                 bool transmit, unsigned int flags)
 {
        struct tb_ring *ring = NULL;
        dev_info(&nhi->pdev->dev, "allocating %s ring %d of size %d\n",

@@ -271,9 +351,14 @@ static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
        ring->hop = hop;
        ring->is_tx = transmit;
        ring->size = size;
+       ring->flags = flags;
        ring->head = 0;
        ring->tail = 0;
        ring->running = false;
+
+       if (ring_request_msix(ring, flags & RING_FLAG_NO_SUSPEND))
+               goto err;
+
        ring->descriptors = dma_alloc_coherent(&ring->nhi->pdev->dev,
                        size * sizeof(*ring->descriptors),
                        &ring->descriptors_dma, GFP_KERNEL | __GFP_ZERO);

@@ -295,14 +380,16 @@ static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
        return NULL;
 }
 
-struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, true);
+       return ring_alloc(nhi, hop, size, true, flags);
 }
 
-struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, false);
+       return ring_alloc(nhi, hop, size, false, flags);
 }
 
 /**

@@ -334,6 +421,7 @@ void ring_start(struct tb_ring *ring)
                ring_iowrite32options(ring,
                                      RING_FLAG_ENABLE | RING_FLAG_RAW, 0);
        }
+       ring_map_unmap_msix(ring, true);
        ring_interrupt_active(ring, true);
        ring->running = true;
 err:

@@ -366,6 +454,7 @@ void ring_stop(struct tb_ring *ring)
                goto err;
        }
        ring_interrupt_active(ring, false);
+       ring_map_unmap_msix(ring, false);
 
        ring_iowrite32options(ring, 0, 0);
        ring_iowrite64desc(ring, 0, 0);

@@ -413,6 +502,8 @@ void ring_free(struct tb_ring *ring)
                         RING_TYPE(ring), ring->hop);
        }
 

drivers/thunderbolt/nhi.c void ring_free(struct tb_ring *ring) · line 509

Quoted 5 lines

drivers/thunderbolt/nhi.c +2 -0

+       ring_release_msix(ring);
+
        dma_free_coherent(&ring->nhi->pdev->dev,
                          ring->size * sizeof(*ring->descriptors),
                          ring->descriptors, ring->descriptors_dma);

There is a comment a couple lines below that states that ring->work is only scheduled by nhi_interrupt_work and ring_stop. Please add ring_msix to the list.

Quoted 30 lines

drivers/thunderbolt/nhi.c +23 -1

@@ -528,9 +619,51 @@ static void nhi_shutdown(struct tb_nhi *nhi)
         * We have to release the irq before calling flush_work. Otherwise an
         * already executing IRQ handler could call schedule_work again.
         */
-       devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
+       if (!nhi->pdev->msix_enabled)
+               devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
        flush_work(&nhi->interrupt_work);
        mutex_destroy(&nhi->lock);
+       ida_destroy(&nhi->msix_ida);
+}
+
+static int nhi_init_msi(struct tb_nhi *nhi)
+{
+       struct pci_dev *pdev = nhi->pdev;
+       int res, irq, nvec;
+
+       /* In case someone left them on. */
+       nhi_disable_interrupts(nhi);
+
+       ida_init(&nhi->msix_ida);
+
+       /*
+        * The NHI has 16 MSI-X vectors or a single MSI. We first try to
+        * get all MSI-X vectors and if we succeed, each ring will have
+        * one MSI-X. If for some reason that does not work out, we
+        * fallback to a single MSI.
+        */
+       nvec = pci_alloc_irq_vectors(pdev, MSIX_MIN_VECS, MSIX_MAX_VECS,
+                                    PCI_IRQ_MSIX);

drivers/thunderbolt/nhi.c static void nhi_shutdown(struct tb_nhi *nhi) · line 652

Quoted 6 lines

drivers/thunderbolt/nhi.c +6 -0

+       if (nvec < 0) {
+               nvec = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+               if (nvec < 0)
+                       return nvec;
+
+               INIT_WORK(&nhi->interrupt_work, nhi_interrupt_work);

As far as I can see INIT_WORK is only called here in the fallback case. But flush_work is still called unconditionally in nhi_shutdown.

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Monday, May 22, 2017 11:52
To: Andreas Noever <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Jamet, Michael <[email protected]>; Bernat, Yehezkel <[email protected]>; Lukas Wunner <[email protected]>; Levy, Amir (Jer) <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]; Andy Shevchenko <[email protected]>; [email protected]
Subject: Re: [PATCH 04/24] thunderbolt: Add MSI-X support

Quoted 2 lines · Andreas Noever

On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg
<[email protected]> wrote:

Quoted 2 lines

Quoted 1 line

Intel Thunderbolt controllers support up to 16 MSI-X vectors. Using

Is that true for all generations? If so can we remove the legacy path?

Yes, I think it has been the case from the beginning. I have here one Mac Mini with Light Ridge and even it has working MSI-X.

Yehezkel, Michael, Amir, do you know if that's the case for all other controller as well?

Quoted 3 lines

Quoted 3 lines

MSI-X is preferred over MSI or legacy interrupt and may bring additional performance because there is no need to check the status registers

which

Quoted 5 lines

Quoted 5 lines

interrupt was triggered.

While there we convert comments in structs tb_ring and tb_nhi to follow kernel-doc format more closely.

Quoted 8 lines

Quoted 8 lines

This code is based on the work done by Amir Levy and Michael Jamet.

Signed-off-by Michael Jamet <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

---
 drivers/thunderbolt/ctl.c      |   4 +-
 drivers/thunderbolt/nhi.c      | 159

++++++++++++++++++++++++++++++++++++-----

Quoted 9 lines

drivers/thunderbolt/ctl.c struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi,

Quoted 9 lines

 drivers/thunderbolt/nhi.h      |  56 +++++++++++----
 drivers/thunderbolt/nhi_regs.h |   9 +++
 4 files changed, 196 insertions(+), 32 deletions(-)

drivers/thunderbolt/ctl.c +0 -0

index 1031d97407a8..889a32dd21e7 100644
@@ -488,11 +488,11 @@ struct tb_ctl *tb_ctl_alloc(struct tb_nhi *nhi,

hotplug_cb cb, void *cb_data)

Quoted 19 lines

Quoted 19 lines

drivers/thunderbolt/ctl.c +2 -2

        if (!ctl->frame_pool)
                goto err;
 
-       ctl->tx = ring_alloc_tx(nhi, 0, 10);
+       ctl->tx = ring_alloc_tx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->tx)
                goto err;
 
-       ctl->rx = ring_alloc_rx(nhi, 0, 10);
+       ctl->rx = ring_alloc_rx(nhi, 0, 10, RING_FLAG_NO_SUSPEND);
        if (!ctl->rx)
                goto err;
 

drivers/thunderbolt/nhi.c +0 -0

index a8c20413dbda..17f3b1bdb7da 100644
@@ -21,6 +21,12 @@
 

Quoted 4 lines

drivers/thunderbolt/nhi.c

Quoted 4 lines

drivers/thunderbolt/nhi.c +2 -0

 #define RING_TYPE(ring) ((ring)->is_tx ? "TX ring" : "RX ring")
 
+/*
+ * Minimal number of vectors when we use MSI-X. Two for control

channel

Quoted 8 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct

Quoted 8 lines

drivers/thunderbolt/nhi.c +4 -0

+ * Rx/Tx and the rest four are for cross domain DMA paths.
+ */
+#define MSIX_MIN_VECS          6
+#define MSIX_MAX_VECS          16
 
 static int ring_interrupt_index(struct tb_ring *ring)
 {

@@ -239,8 +245,82 @@ int __ring_enqueue(struct tb_ring *ring, struct

ring_frame *frame)

Quoted 18 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame) · line 257

Quoted 13 lines

drivers/thunderbolt/nhi.c +10 -0

        return ret;
 }
 
+static irqreturn_t ring_msix(int irq, void *data)
+{
+       struct tb_ring *ring = data;
+
+       schedule_work(&ring->work);
+       return IRQ_HANDLED;
+}
+
+static void ring_map_unmap_msix(struct tb_ring *ring, bool map)
+{

This function does the same as ring_interrupt_active just for msix, right? Can we rename one (or both) to express that?
For example rename ring_interrput_active -> ring_map_unmap_msi. Or just roll this one into ring_interrupt_active and do the right thing internally.

Yes, I think we can do that.

Quoted 14 lines

Quoted 14 lines

drivers/thunderbolt/nhi.c +14 -0

+       u32 step, shift, ivr, misc;
+       int index;
+
+       if (ring->irq <= 0)
+               return;
+
+       if (ring->is_tx)
+               index = ring->hop;
+       else
+               index = ring->hop + ring->nhi->hop_count;
+
+       /*
+        * Ask the hardware to clear interrupt status bits automatically
+        * since we already know which interrupt was triggered.

Quoted 8 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)

Quoted 8 lines

drivers/thunderbolt/nhi.c +8 -0

+        */
+       misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
+       if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
+               misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
+               iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
+       }
+
+       step = index / REG_INT_VEC_ALLOC_REGS *

REG_INT_VEC_ALLOC_BITS;

Quoted 1 line

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct · line 280

Quoted 1 line

drivers/thunderbolt/nhi.c +1 -0

+       shift = index % REG_INT_VEC_ALLOC_REGS *

REG_INT_VEC_ALLOC_BITS;

Quoted 1 line

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct · line 281

Quoted 1 line

drivers/thunderbolt/nhi.c +1 -0

+       ivr = ioread32(ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE +

step);

Quoted 4 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)

Quoted 4 lines

drivers/thunderbolt/nhi.c +4 -0

+       ivr &= ~(REG_INT_VEC_ALLOC_MASK << shift);
+       if (map)
+               ivr |= ring->vector << shift;
+       iowrite32(ivr, ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE +

step);

Quoted 4 lines

Quoted 4 lines

drivers/thunderbolt/nhi.c +4 -0

+}
+
+static int ring_request_msix(struct tb_ring *ring, bool no_suspend)
+{

Quoted 8 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)

Quoted 8 lines

drivers/thunderbolt/nhi.c +8 -0

+       struct tb_nhi *nhi = ring->nhi;
+       unsigned long irqflags;
+       int ret;
+
+       if (!nhi->pdev->msix_enabled)
+               return 0;
+
+       ret = ida_simple_get(&nhi->msix_ida, 0, MSIX_MAX_VECS,

GFP_KERNEL);

Quoted 11 lines

drivers/thunderbolt/nhi.c int __ring_enqueue(struct tb_ring *ring, struct ring_frame *frame)

Quoted 11 lines

drivers/thunderbolt/nhi.c +11 -0

+       if (ret < 0)
+               return ret;
+
+       ring->vector = ret;
+
+       ring->irq = pci_irq_vector(ring->nhi->pdev, ring->vector);
+       if (ring->irq < 0)
+               return ring->irq;
+
+       irqflags = no_suspend ? IRQF_NO_SUSPEND : 0;
+       return request_irq(ring->irq, ring_msix, irqflags, "thunderbolt",

ring);

Quoted 12 lines

Quoted 12 lines

drivers/thunderbolt/nhi.c +12 -0

+}
+
+static void ring_release_msix(struct tb_ring *ring)
+{
+       if (ring->irq <= 0)
+               return;
+
+       free_irq(ring->irq, ring);
+       ida_simple_remove(&ring->nhi->msix_ida, ring->vector);
+       ring->vector = 0;
+       ring->irq = 0;
+}

Quoted 8 lines

drivers/thunderbolt/nhi.c static struct tb_ring *ring_alloc(struct tb_nhi

Quoted 8 lines

drivers/thunderbolt/nhi.c +2 -1

+
 static struct tb_ring *ring_alloc(struct tb_nhi *nhi, u32 hop, int size,
-                                 bool transmit)
+                                 bool transmit, unsigned int flags)
 {
        struct tb_ring *ring = NULL;
        dev_info(&nhi->pdev->dev, "allocating %s ring %d of size %d\n",

@@ -271,9 +351,14 @@ static struct tb_ring *ring_alloc(struct tb_nhi

*nhi, u32 hop, int size,

Quoted 7 lines

Quoted 7 lines

drivers/thunderbolt/nhi.c +1 -0

        ring->hop = hop;
        ring->is_tx = transmit;
        ring->size = size;
+       ring->flags = flags;
        ring->head = 0;
        ring->tail = 0;
        ring->running = false;

Quoted 8 lines

drivers/thunderbolt/nhi.c static struct tb_ring *ring_alloc(struct tb_nhi

Quoted 8 lines

drivers/thunderbolt/nhi.c +4 -0

+
+       if (ring_request_msix(ring, flags & RING_FLAG_NO_SUSPEND))
+               goto err;
+
        ring->descriptors = dma_alloc_coherent(&ring->nhi->pdev->dev,
                        size * sizeof(*ring->descriptors),
                        &ring->descriptors_dma, GFP_KERNEL | __GFP_ZERO);

@@ -295,14 +380,16 @@ static struct tb_ring *ring_alloc(struct tb_nhi

*nhi, u32 hop, int size,

Quoted 40 lines

Quoted 40 lines

drivers/thunderbolt/nhi.c +8 -4

        return NULL;
 }
 
-struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_tx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, true);
+       return ring_alloc(nhi, hop, size, true, flags);
 }
 
-struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size)
+struct tb_ring *ring_alloc_rx(struct tb_nhi *nhi, int hop, int size,
+                             unsigned int flags)
 {
-       return ring_alloc(nhi, hop, size, false);
+       return ring_alloc(nhi, hop, size, false, flags);
 }
 
 /**

@@ -334,6 +421,7 @@ void ring_start(struct tb_ring *ring)
                ring_iowrite32options(ring,
                                      RING_FLAG_ENABLE | RING_FLAG_RAW, 0);
        }
+       ring_map_unmap_msix(ring, true);
        ring_interrupt_active(ring, true);
        ring->running = true;
 err:

@@ -366,6 +454,7 @@ void ring_stop(struct tb_ring *ring)
                goto err;
        }
        ring_interrupt_active(ring, false);
+       ring_map_unmap_msix(ring, false);
 
        ring_iowrite32options(ring, 0, 0);
        ring_iowrite64desc(ring, 0, 0);

@@ -413,6 +502,8 @@ void ring_free(struct tb_ring *ring)
                         RING_TYPE(ring), ring->hop);
        }
 

Quoted 8 lines

drivers/thunderbolt/nhi.c void ring_free(struct tb_ring *ring) · line 509

Quoted 5 lines

drivers/thunderbolt/nhi.c +2 -0

+       ring_release_msix(ring);
+
        dma_free_coherent(&ring->nhi->pdev->dev,
                          ring->size * sizeof(*ring->descriptors),
                          ring->descriptors, ring->descriptors_dma);

There is a comment a couple lines below that states that ring->work is only scheduled by nhi_interrupt_work and ring_stop. Please add ring_msix to the list.

OK.

Quoted 21 lines

Quoted 21 lines

drivers/thunderbolt/nhi.c +14 -1

@@ -528,9 +619,51 @@ static void nhi_shutdown(struct tb_nhi *nhi)
         * We have to release the irq before calling flush_work. Otherwise an
         * already executing IRQ handler could call schedule_work again.
         */
-       devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
+       if (!nhi->pdev->msix_enabled)
+               devm_free_irq(&nhi->pdev->dev, nhi->pdev->irq, nhi);
        flush_work(&nhi->interrupt_work);
        mutex_destroy(&nhi->lock);
+       ida_destroy(&nhi->msix_ida);
+}
+
+static int nhi_init_msi(struct tb_nhi *nhi)
+{
+       struct pci_dev *pdev = nhi->pdev;
+       int res, irq, nvec;
+
+       /* In case someone left them on. */
+       nhi_disable_interrupts(nhi);
+
+       ida_init(&nhi->msix_ida);

Quoted 8 lines

drivers/thunderbolt/nhi.c static void nhi_shutdown(struct tb_nhi *nhi)

Quoted 8 lines

drivers/thunderbolt/nhi.c +8 -0

+
+       /*
+        * The NHI has 16 MSI-X vectors or a single MSI. We first try to
+        * get all MSI-X vectors and if we succeed, each ring will have
+        * one MSI-X. If for some reason that does not work out, we
+        * fallback to a single MSI.
+        */
+       nvec = pci_alloc_irq_vectors(pdev, MSIX_MIN_VECS,

MSIX_MAX_VECS,

Quoted 9 lines

drivers/thunderbolt/nhi.c static void nhi_shutdown(struct tb_nhi *nhi) · line 652

Quoted 7 lines

drivers/thunderbolt/nhi.c +7 -0

+                                    PCI_IRQ_MSIX);
+       if (nvec < 0) {
+               nvec = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+               if (nvec < 0)
+                       return nvec;
+
+               INIT_WORK(&nhi->interrupt_work, nhi_interrupt_work);

As far as I can see INIT_WORK is only called here in the fallback case. But flush_work is still called unconditionally in nhi_shutdown.

Good point. I should make it conditional as well (or INIT_WORK() unconditional).

Quoted 23 lines

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Monday, May 22, 2017 11:52
To: Andreas Noever <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Jamet, Michael <[email protected]>; Bernat, Yehezkel <[email protected]>; Lukas Wunner <[email protected]>; Levy, Amir (Jer) <[email protected]>; Andy Lutomirski <[email protected]>; [email protected]; [email protected]; Andy Shevchenko <[email protected]>; [email protected]
Subject: Re: [PATCH 04/24] thunderbolt: Add MSI-X support

Quoted 4 lines · Andreas Noever

Quoted 1 line · Mika Westerberg

Intel Thunderbolt controllers support up to 16 MSI-X vectors. Using

Is that true for all generations? If so can we remove the legacy path?

Yes, I think it has been the case from the beginning. I have here one Mac Mini with Light Ridge and even it has working MSI-X.

Quoted 2 lines

Yehezkel, Michael, Amir, do you know if that's the case for all other controller as well?

We are checking it, but what if we don't get the MSI-X we need? Don't we have to support MSI as a fallback option or maybe we are sure this will never happen in modern kernels?

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to extract capabilities for ports and switches. Then convert the current users over these.

drivers/thunderbolt/cap.c +36 -0

+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
 {
 
+       u32 offset;
 
 
+       /*
+        * DP out adapters claim to implement TMU capability but in
+        * reality they do not so we hard code the adapter specific
+        * capability offset here.
+        */
+       if (port->config.type == TB_TYPE_DP_HDMI_OUT)
+               offset = 0x39;
        else
 
+               offset = 0x1;
+
+       do {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
+       } while (offset);
+
+       return -ENOENT;
 }
 
+static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
 {
 
+       int offset = sw->config.first_cap_offset;
+
+       while (offset > 0 && offset < CAP_OFFSET_MAX) {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
+               if (ret)
+                       return ret;

Quoted 6 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to extract capabilities for ports and switches. Then convert the current users over these.

One nit here.

Quoted 52 lines

drivers/thunderbolt/cap.c +41 -0

+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
 {
 
+       u32 offset;
 
 
+       /*
+        * DP out adapters claim to implement TMU capability but in
+        * reality they do not so we hard code the adapter specific
+        * capability offset here.
+        */
+       if (port->config.type == TB_TYPE_DP_HDMI_OUT)
+               offset = 0x39;
        else
 
+               offset = 0x1;
+
+       do {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
+       } while (offset);
+
+       return -ENOENT;
 }
 
+static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
 {
 
+       int offset = sw->config.first_cap_offset;
+
+       while (offset > 0 && offset < CAP_OFFSET_MAX) {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
        }

drivers/thunderbolt/cap.c struct tb_cap_any { · line 61

Quoted 3 lines

drivers/thunderbolt/cap.c +2 -0

+
+       return -ENOENT;
 }

Both has quite similar bodies.
Wouldn't be nice to split out a helper which takes initial offset and type as parameters?

Quoted 8 lines · Andy Shevchenko

On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
<[email protected]> wrote:

One nit here.

Both has quite similar bodies.
Wouldn't be nice to split out a helper which takes initial offset and type as parameters?

The whole point of this rework was to separate port vs. switch capability to follow how the hardware is organized instead of having one more complex function handling everything :)

Sure, I can merge them back together but IMHO it will be not that readable anymore.

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to extract capabilities for ports and switches. Then convert the current users over these.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/cap.c        | 169 +++++++++++++++++++++------------------
 drivers/thunderbolt/switch.c     |   6 +-
 drivers/thunderbolt/tb.c         |   8 +-
 drivers/thunderbolt/tb.h         |   3 +-
 drivers/thunderbolt/tb_regs.h    |  31 ++++---
 drivers/thunderbolt/tunnel_pci.c |   8 +-
 6 files changed, 124 insertions(+), 101 deletions(-)

drivers/thunderbolt/cap.c +14 -15

index a7b47e7cddbd..0dd852c3df8d 100644
@@ -9,6 +9,8 @@
 
 #include "tb.h"
 
+#define CAP_OFFSET_MAX         0xff
+#define VSEC_CAP_OFFSET_MAX    0xffff
 
 struct tb_cap_any {
        union {

@@ -18,99 +20,110 @@ struct tb_cap_any {
        };
 } __packed;
 
-static bool tb_cap_is_basic(struct tb_cap_any *cap)
-{
-       /* basic.cap is u8. This checks only the lower 8 bit of cap. */
-       return cap->basic.cap != 5;
-}
-
-static bool tb_cap_is_long(struct tb_cap_any *cap)
+/**
+ * tb_port_find_cap() - Find port capability
+ * @port: Port to find the capability for
+ * @cap: Capability to look
+ *
+ * Returns offset to start of capability or %-ENOENT if no such
+ * capability was found. Negative errno is returned if there was an
+ * error.
+ */
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
 {
-       return !tb_cap_is_basic(cap)
-              && cap->extended_short.next == 0
-              && cap->extended_short.length == 0;
-}
+       u32 offset;
 
-static enum tb_cap tb_cap(struct tb_cap_any *cap)
-{
-       if (tb_cap_is_basic(cap))
-               return cap->basic.cap;
+       /*

drivers/thunderbolt/cap.c +70 -61

+
+       do {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
+       } while (offset);
+
+       return -ENOENT;
 }
 
-static u32 tb_cap_next(struct tb_cap_any *cap, u32 offset)
+static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
 {
-       int next;
-       if (offset == 1) {
-               /*
-                * The first pointer is part of the switch header and always
-                * a simple pointer.
-                */
-               next = cap->basic.next;
-       } else {
-               /*
-                * Somehow Intel decided to use 3 different types of capability
-                * headers. It is not like anyone could have predicted that
-                * single byte offsets are not enough...
-                */
-               if (tb_cap_is_basic(cap))
-                       next = cap->basic.next;
-               else if (!tb_cap_is_long(cap))
-                       next = cap->extended_short.next;
-               else
-                       next = cap->extended_long.next;
+       int offset = sw->config.first_cap_offset;
+
+       while (offset > 0 && offset < CAP_OFFSET_MAX) {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
        }
-       /*
-        * "Hey, we could terminate some capability lists with a null offset
-        *  and others with a pointer to the last element." - "Great idea!"
-        */
-       if (next == offset)
-               return 0;
-       return next;
+
+       return -ENOENT;
 }
 
 /**
- * tb_find_cap() - find a capability
+ * tb_switch_find_vsec_cap() - Find switch vendor specific capability
+ * @sw: Switch to find the capability for
+ * @vsec: Vendor specific capability to look
  *
- * Return: Returns a positive offset if the capability was found and 0 if not.
- * Returns an error code on failure.
+ * Functions enumerates vendor specific (VSEC) capabilities of a switch
+ * and returns offset when capability matching @vsed is found. If no
+ * such capability is found returns %-ENOENT. In case of error returns
+ * negative errno.
  */
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap)
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec)
 {
-       u32 offset = 1;
        struct tb_cap_any header;
-       int res;
-       int retries = 10;
-       while (retries--) {
-               res = tb_port_read(port, &header, space, offset, 1);
-               if (res) {
-                       /* Intel needs some help with linked lists. */
-                       if (space == TB_CFG_PORT && offset == 0xa
-                           && port->config.type == TB_TYPE_DP_HDMI_OUT) {
-                               offset = 0x39;
-                               continue;
-                       }
-                       return res;
-               }
-               if (offset != 1) {
-                       if (tb_cap(&header) == cap)
+       int offset;
+
+       offset = tb_switch_find_cap(sw, TB_SWITCH_CAP_VSEC);
+       if (offset < 0)
+               return offset;
+
+       while (offset > 0 && offset < VSEC_CAP_OFFSET_MAX) {
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 2);
+               if (ret)
+                       return ret;
+
+               /*
+                * Extended vendor specific capabilities come in two
+                * flavors: short and long. The latter is used when
+                * offset is over 0xff.
+                */
+               if (offset >= CAP_OFFSET_MAX) {
+                       if (header.extended_long.vsec_id == vsec)
                                return offset;
-                       if (tb_cap_is_long(&header)) {
-                               /* tb_cap_extended_long is 2 dwords */
-                               res = tb_port_read(port, &header, space,
-                                                  offset, 2);
-                               if (res)
-                                       return res;
-                       }
+                       offset = header.extended_long.next;
+               } else {
+                       if (header.extended_short.vsec_id == vsec)
+                               return offset;
+                       if (!header.extended_short.length)
+                               return -ENOENT;
+                       offset = header.extended_short.next;
                }
-               offset = tb_cap_next(&header, offset);
-               if (!offset)
-                       return 0;
        }
-       tb_port_WARN(port,
-                    "run out of retries while looking for cap %#x in config space %d, last offset: %#x\n",
-                    cap, space, offset);
-       return -EIO;
+
+       return -ENOENT;
 }

drivers/thunderbolt/switch.c +3 -3

index c6f30b1695a9..5bae49d72d2c 100644
@@ -192,7 +192,7 @@ static int tb_init_port(struct tb_port *port)
 
        /* Port 0 is the switch itself and has no PHY. */
        if (port->config.type == TB_TYPE_PORT && port->port != 0) {
-               cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PHY);
+               cap = tb_port_find_cap(port, TB_PORT_CAP_PHY);
 
                if (cap > 0)
                        port->cap_phy = cap;

@@ -394,9 +394,9 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
                sw->ports[i].port = i;
        }
 
-       cap = tb_find_cap(&sw->ports[0], TB_CFG_SWITCH, TB_CAP_PLUG_EVENTS);
+       cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_PLUG_EVENTS);
        if (cap < 0) {
-               tb_sw_warn(sw, "cannot find TB_CAP_PLUG_EVENTS aborting\n");
+               tb_sw_warn(sw, "cannot find TB_VSEC_CAP_PLUG_EVENTS aborting\n");
                goto err;
        }
        sw->cap_plug_events = cap;

drivers/thunderbolt/tb.c +4 -4

index 24b6d30c3c86..6b44076e1380 100644
@@ -121,8 +121,8 @@ static struct tb_port *tb_find_unused_down_port(struct tb_switch *sw)
                        continue;
                if (sw->ports[i].config.type != TB_TYPE_PCIE_DOWN)
                        continue;
-               cap = tb_find_cap(&sw->ports[i], TB_CFG_PORT, TB_CAP_PCIE);
-               if (cap <= 0)
+               cap = tb_port_find_cap(&sw->ports[i], TB_PORT_CAP_ADAP);
+               if (cap < 0)
                        continue;
                res = tb_port_read(&sw->ports[i], &data, TB_CFG_PORT, cap, 1);
                if (res < 0)

@@ -165,8 +165,8 @@ static void tb_activate_pcie_devices(struct tb *tb)
                }
 
                /* check whether port is already activated */
-               cap = tb_find_cap(up_port, TB_CFG_PORT, TB_CAP_PCIE);
-               if (cap <= 0)
+               cap = tb_port_find_cap(up_port, TB_PORT_CAP_ADAP);
+               if (cap < 0)
                        continue;
                if (tb_port_read(up_port, &data, TB_CFG_PORT, cap, 1))
                        continue;

drivers/thunderbolt/tb.h +2 -1

index ba2b85750335..9b60e71562dc 100644
@@ -233,7 +233,8 @@ int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged);
 int tb_port_add_nfc_credits(struct tb_port *port, int credits);
 int tb_port_clear_counter(struct tb_port *port, int counter);
 
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap);
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec);
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap);
 
 struct tb_path *tb_path_alloc(struct tb *tb, int num_hops);
 void tb_path_free(struct tb_path *path);

drivers/thunderbolt/tb_regs.h +16 -9

index 1e2a4a8046be..57fdfa61e136 100644
@@ -23,15 +23,22 @@
  */
 #define TB_MAX_CONFIG_RW_LENGTH 60
 
-enum tb_cap {
-       TB_CAP_PHY              = 0x0001,
-       TB_CAP_TIME1            = 0x0003,
-       TB_CAP_PCIE             = 0x0004,
-       TB_CAP_I2C              = 0x0005,
-       TB_CAP_PLUG_EVENTS      = 0x0105, /* also EEPROM */
-       TB_CAP_TIME2            = 0x0305,
-       TB_CAP_IECS             = 0x0405,
-       TB_CAP_LINK_CONTROLLER  = 0x0605, /* also IECS */
+enum tb_switch_cap {
+       TB_SWITCH_CAP_VSEC              = 0x05,
+};
+
+enum tb_switch_vsec_cap {
+       TB_VSEC_CAP_PLUG_EVENTS         = 0x01, /* also EEPROM */
+       TB_VSEC_CAP_TIME2               = 0x03,
+       TB_VSEC_CAP_IECS                = 0x04,
+       TB_VSEC_CAP_LINK_CONTROLLER     = 0x06, /* also IECS */
+};
+
+enum tb_port_cap {
+       TB_PORT_CAP_PHY                 = 0x01,
+       TB_PORT_CAP_TIME1               = 0x03,
+       TB_PORT_CAP_ADAP                = 0x04,
+       TB_PORT_CAP_VSEC                = 0x05,
 };
 
 enum tb_port_state {

drivers/thunderbolt/tb_regs.h +2 -1

        u8 length;
 } __packed;
 
 struct tb_cap_extended_long {
        u8 zero1;
-       enum tb_cap cap:16;
+       u8 cap;
+       u8 vsec_id;
        u8 zero2;
        u16 next;
        u16 length;

drivers/thunderbolt/tunnel_pci.c +4 -5

index baf1cd370446..f4ce9845e42a 100644
@@ -147,10 +147,10 @@ bool tb_pci_is_invalid(struct tb_pci_tunnel *tunnel)
 static int tb_pci_port_active(struct tb_port *port, bool active)
 {
        u32 word = active ? 0x80000000 : 0x0;
-       int cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PCIE);
-       if (cap <= 0) {
-               tb_port_warn(port, "TB_CAP_PCIE not found: %d\n", cap);
-               return cap ? cap : -ENXIO;
+       int cap = tb_port_find_cap(port, TB_PORT_CAP_ADAP);
+       if (cap < 0) {
+               tb_port_warn(port, "TB_PORT_CAP_ADAP not found: %d\n", cap);
+               return cap;
        }
        return tb_port_write(port, &word, TB_CFG_PORT, cap, 1);
 }
--

2.11.0

Quoted 6 lines · Mika Westerberg

Organization of the capabilities in switches and ports is not so random after all. Rework the capability handling functionality so that it follows how capabilities are organized and provide two new functions (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to extract capabilities for ports and switches. Then convert the current users over these.

Hm, could you explain the "official" logic a bit? What I could gather from reading the code:
- Port capabilities are always "basic" and the list is terminated
with next=0x00

- Switch capabilities start "basic" then if cap==VSEC they turn into
"extendend_short" and after offset=0xff they turn into "extended_long". The list is terminated by either next=0x00 or next=0xffff.

- There is always an extended cap whose offset is < 0xff (otherwise
tb_switch_find_cap won't find the first vsec cap).
- If there are "long" caps then there is always one at exactly 0xff
(because short caps have single byte next pointers).

Are these observations correct? I must say I found the old logic to detect long capabilities (short.next == short.length == 0) more elegant (and I'm quite sure that is the logic that the Mac driver implemented at some point), but we can of course change it to match the way it is written in the spec (and of course I have to request this at least once: you should definitely release the spec - I highly doubt that Intel's competitive advantage depends on keeping this linked list technology secret ;) ).

Two more inline comments:

Quoted 64 lines

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/cap.c        | 169 +++++++++++++++++++++------------------
 drivers/thunderbolt/switch.c     |   6 +-
 drivers/thunderbolt/tb.c         |   8 +-
 drivers/thunderbolt/tb.h         |   3 +-
 drivers/thunderbolt/tb_regs.h    |  31 ++++---
 drivers/thunderbolt/tunnel_pci.c |   8 +-
 6 files changed, 124 insertions(+), 101 deletions(-)

drivers/thunderbolt/cap.c +20 -15

index a7b47e7cddbd..0dd852c3df8d 100644
@@ -9,6 +9,8 @@
 
 #include "tb.h"
 
+#define CAP_OFFSET_MAX         0xff
+#define VSEC_CAP_OFFSET_MAX    0xffff
 
 struct tb_cap_any {
        union {

@@ -18,99 +20,110 @@ struct tb_cap_any {
        };
 } __packed;
 
-static bool tb_cap_is_basic(struct tb_cap_any *cap)
-{
-       /* basic.cap is u8. This checks only the lower 8 bit of cap. */
-       return cap->basic.cap != 5;
-}
-
-static bool tb_cap_is_long(struct tb_cap_any *cap)
+/**
+ * tb_port_find_cap() - Find port capability
+ * @port: Port to find the capability for
+ * @cap: Capability to look
+ *
+ * Returns offset to start of capability or %-ENOENT if no such
+ * capability was found. Negative errno is returned if there was an
+ * error.
+ */
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
 {
-       return !tb_cap_is_basic(cap)
-              && cap->extended_short.next == 0
-              && cap->extended_short.length == 0;
-}
+       u32 offset;
 
-static enum tb_cap tb_cap(struct tb_cap_any *cap)
-{
-       if (tb_cap_is_basic(cap))
-               return cap->basic.cap;
+       /*
+        * DP out adapters claim to implement TMU capability but in
+        * reality they do not so we hard code the adapter specific
+        * capability offset here.
+        */
+       if (port->config.type == TB_TYPE_DP_HDMI_OUT)
+               offset = 0x39;

drivers/thunderbolt/cap.c struct tb_cap_any { · line 44

Quoted 4 lines

drivers/thunderbolt/cap.c +1 -2

        else
-               /* extended_short/long have cap at the same offset. */
-               return cap->extended_short.cap;
+               offset = 0x1;

When I was reverse engineering this the problem was not that some capability was advertised (what does "TMU" stand for?), but that a capabilities's next pointer (the one at 0xa) was pointing .. *somewhere*.

The fix was to redirect it to 0x39 (which looked like it was the next valid capability). Your code makes the iteration start at 0x39. Which I guess is equivalent if the first capability is always the faulty one?

Also can we make this quirk specific to some rom version or is it expected that HDMI_OUT ports will forever have their first capability at 0x39?

Quoted 249 lines

drivers/thunderbolt/cap.c +70 -61

+
+       do {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
+       } while (offset);
+
+       return -ENOENT;
 }
 
-static u32 tb_cap_next(struct tb_cap_any *cap, u32 offset)
+static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
 {
-       int next;
-       if (offset == 1) {
-               /*
-                * The first pointer is part of the switch header and always
-                * a simple pointer.
-                */
-               next = cap->basic.next;
-       } else {
-               /*
-                * Somehow Intel decided to use 3 different types of capability
-                * headers. It is not like anyone could have predicted that
-                * single byte offsets are not enough...
-                */
-               if (tb_cap_is_basic(cap))
-                       next = cap->basic.next;
-               else if (!tb_cap_is_long(cap))
-                       next = cap->extended_short.next;
-               else
-                       next = cap->extended_long.next;
+       int offset = sw->config.first_cap_offset;
+
+       while (offset > 0 && offset < CAP_OFFSET_MAX) {
+               struct tb_cap_any header;
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
+               if (ret)
+                       return ret;
+
+               if (header.basic.cap == cap)
+                       return offset;
+
+               offset = header.basic.next;
        }
-       /*
-        * "Hey, we could terminate some capability lists with a null offset
-        *  and others with a pointer to the last element." - "Great idea!"
-        */
-       if (next == offset)
-               return 0;
-       return next;
+
+       return -ENOENT;
 }
 
 /**
- * tb_find_cap() - find a capability
+ * tb_switch_find_vsec_cap() - Find switch vendor specific capability
+ * @sw: Switch to find the capability for
+ * @vsec: Vendor specific capability to look
  *
- * Return: Returns a positive offset if the capability was found and 0 if not.
- * Returns an error code on failure.
+ * Functions enumerates vendor specific (VSEC) capabilities of a switch
+ * and returns offset when capability matching @vsed is found. If no
+ * such capability is found returns %-ENOENT. In case of error returns
+ * negative errno.
  */
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap)
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec)
 {
-       u32 offset = 1;
        struct tb_cap_any header;
-       int res;
-       int retries = 10;
-       while (retries--) {
-               res = tb_port_read(port, &header, space, offset, 1);
-               if (res) {
-                       /* Intel needs some help with linked lists. */
-                       if (space == TB_CFG_PORT && offset == 0xa
-                           && port->config.type == TB_TYPE_DP_HDMI_OUT) {
-                               offset = 0x39;
-                               continue;
-                       }
-                       return res;
-               }
-               if (offset != 1) {
-                       if (tb_cap(&header) == cap)
+       int offset;
+
+       offset = tb_switch_find_cap(sw, TB_SWITCH_CAP_VSEC);
+       if (offset < 0)
+               return offset;
+
+       while (offset > 0 && offset < VSEC_CAP_OFFSET_MAX) {
+               int ret;
+
+               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 2);
+               if (ret)
+                       return ret;
+
+               /*
+                * Extended vendor specific capabilities come in two
+                * flavors: short and long. The latter is used when
+                * offset is over 0xff.
+                */
+               if (offset >= CAP_OFFSET_MAX) {
+                       if (header.extended_long.vsec_id == vsec)
                                return offset;
-                       if (tb_cap_is_long(&header)) {
-                               /* tb_cap_extended_long is 2 dwords */
-                               res = tb_port_read(port, &header, space,
-                                                  offset, 2);
-                               if (res)
-                                       return res;
-                       }
+                       offset = header.extended_long.next;
+               } else {
+                       if (header.extended_short.vsec_id == vsec)
+                               return offset;
+                       if (!header.extended_short.length)
+                               return -ENOENT;
+                       offset = header.extended_short.next;
                }
-               offset = tb_cap_next(&header, offset);
-               if (!offset)
-                       return 0;
        }
-       tb_port_WARN(port,
-                    "run out of retries while looking for cap %#x in config space %d, last offset: %#x\n",
-                    cap, space, offset);
-       return -EIO;
+
+       return -ENOENT;
 }

drivers/thunderbolt/switch.c +3 -3

index c6f30b1695a9..5bae49d72d2c 100644
@@ -192,7 +192,7 @@ static int tb_init_port(struct tb_port *port)
 
        /* Port 0 is the switch itself and has no PHY. */
        if (port->config.type == TB_TYPE_PORT && port->port != 0) {
-               cap = tb_find_cap(port, TB_CFG_PORT, TB_CAP_PHY);
+               cap = tb_port_find_cap(port, TB_PORT_CAP_PHY);
 
                if (cap > 0)
                        port->cap_phy = cap;

@@ -394,9 +394,9 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, u64 route)
                sw->ports[i].port = i;
        }
 
-       cap = tb_find_cap(&sw->ports[0], TB_CFG_SWITCH, TB_CAP_PLUG_EVENTS);
+       cap = tb_switch_find_vsec_cap(sw, TB_VSEC_CAP_PLUG_EVENTS);
        if (cap < 0) {
-               tb_sw_warn(sw, "cannot find TB_CAP_PLUG_EVENTS aborting\n");
+               tb_sw_warn(sw, "cannot find TB_VSEC_CAP_PLUG_EVENTS aborting\n");
                goto err;
        }
        sw->cap_plug_events = cap;

drivers/thunderbolt/tb.c +4 -4

index 24b6d30c3c86..6b44076e1380 100644
@@ -121,8 +121,8 @@ static struct tb_port *tb_find_unused_down_port(struct tb_switch *sw)
                        continue;
                if (sw->ports[i].config.type != TB_TYPE_PCIE_DOWN)
                        continue;
-               cap = tb_find_cap(&sw->ports[i], TB_CFG_PORT, TB_CAP_PCIE);
-               if (cap <= 0)
+               cap = tb_port_find_cap(&sw->ports[i], TB_PORT_CAP_ADAP);
+               if (cap < 0)
                        continue;
                res = tb_port_read(&sw->ports[i], &data, TB_CFG_PORT, cap, 1);
                if (res < 0)

@@ -165,8 +165,8 @@ static void tb_activate_pcie_devices(struct tb *tb)
                }
 
                /* check whether port is already activated */
-               cap = tb_find_cap(up_port, TB_CFG_PORT, TB_CAP_PCIE);
-               if (cap <= 0)
+               cap = tb_port_find_cap(up_port, TB_PORT_CAP_ADAP);
+               if (cap < 0)
                        continue;
                if (tb_port_read(up_port, &data, TB_CFG_PORT, cap, 1))
                        continue;

drivers/thunderbolt/tb.h +2 -1

index ba2b85750335..9b60e71562dc 100644
@@ -233,7 +233,8 @@ int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged);
 int tb_port_add_nfc_credits(struct tb_port *port, int credits);
 int tb_port_clear_counter(struct tb_port *port, int counter);
 
-int tb_find_cap(struct tb_port *port, enum tb_cfg_space space, enum tb_cap cap);
+int tb_switch_find_vsec_cap(struct tb_switch *sw, enum tb_switch_vsec_cap vsec);
+int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap);
 
 struct tb_path *tb_path_alloc(struct tb *tb, int num_hops);
 void tb_path_free(struct tb_path *path);

drivers/thunderbolt/tb_regs.h +16 -9

index 1e2a4a8046be..57fdfa61e136 100644
@@ -23,15 +23,22 @@
  */
 #define TB_MAX_CONFIG_RW_LENGTH 60
 
-enum tb_cap {
-       TB_CAP_PHY              = 0x0001,
-       TB_CAP_TIME1            = 0x0003,
-       TB_CAP_PCIE             = 0x0004,
-       TB_CAP_I2C              = 0x0005,
-       TB_CAP_PLUG_EVENTS      = 0x0105, /* also EEPROM */
-       TB_CAP_TIME2            = 0x0305,
-       TB_CAP_IECS             = 0x0405,
-       TB_CAP_LINK_CONTROLLER  = 0x0605, /* also IECS */
+enum tb_switch_cap {
+       TB_SWITCH_CAP_VSEC              = 0x05,
+};
+
+enum tb_switch_vsec_cap {
+       TB_VSEC_CAP_PLUG_EVENTS         = 0x01, /* also EEPROM */
+       TB_VSEC_CAP_TIME2               = 0x03,
+       TB_VSEC_CAP_IECS                = 0x04,
+       TB_VSEC_CAP_LINK_CONTROLLER     = 0x06, /* also IECS */
+};
+
+enum tb_port_cap {
+       TB_PORT_CAP_PHY                 = 0x01,
+       TB_PORT_CAP_TIME1               = 0x03,
+       TB_PORT_CAP_ADAP                = 0x04,
+       TB_PORT_CAP_VSEC                = 0x05,
 };
 
 enum tb_port_state {

@@ -51,13 +58,15 @@ struct tb_cap_basic {
 

drivers/thunderbolt/tb_regs.h struct tb_cap_basic { · line 62

Quoted 5 lines

drivers/thunderbolt/tb_regs.h +2 -1

 struct tb_cap_extended_short {
        u8 next; /* if next and length are zero then we have a long cap */
-       enum tb_cap cap:16;
+       u8 cap;
+       u8 vsec_id;

Nit: can we make this enum tb_switch_cap:8; enum tb_switch_vsec_cap:8? (also below) Or at least add a comment linking these fields to the enums.

I'm not sure it is good idea to use bit fields here at all. I'm not an expert in C but I remember reading somewhere that they are not suitable for representing fields inside hardware registers.

(and of course I have to request
this at least once: you should definitely release the spec - I highly doubt that Intel's competitive advantage depends on keeping this linked list technology secret ;) ).

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

drivers/thunderbolt/domain.c new file +5 -0

+
+       destroy_workqueue(tb->wq);
+       ida_simple_remove(&tb_domain_ida, tb->index);
+       kfree(tb);
+}

Quoted 11 lines · Mika Westerberg

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

Quoted 4 lines

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

One nit below.

drivers/thunderbolt/domain.c line 30

Quoted 6 lines

drivers/thunderbolt/domain.c new file +6 -0

+static void tb_domain_release(struct device *dev)
+{
+       struct tb *tb = container_of(dev, struct tb, dev);
+
+       if (tb->ctl)
+               tb_ctl_free(tb->ctl);

The usual pattern is to put such checks inside *_free() type of functions.

Quoted 5 lines

drivers/thunderbolt/domain.c new file +5 -0

+
+       destroy_workqueue(tb->wq);
+       ida_simple_remove(&tb_domain_ida, tb->index);
+       kfree(tb);
+}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 5 lines · Andy Shevchenko

On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
<[email protected]> wrote:

One nit below.

Quoted 8 lines

drivers/thunderbolt/domain.c line 30

Quoted 6 lines

drivers/thunderbolt/domain.c new file +6 -0

+static void tb_domain_release(struct device *dev)
+{
+       struct tb *tb = container_of(dev, struct tb, dev);
+
+       if (tb->ctl)
+               tb_ctl_free(tb->ctl);

The usual pattern is to put such checks inside *_free() type of functions.

tb_ctl_free() and these patterns are from the original code. I just moved the calls from tb.c intto here. No problem doing that change but I suppose that should be done first in a separate patch before this patch.

Quoted 5 lines

Quoted 5 lines

drivers/thunderbolt/domain.c new file +5 -0

+
+       destroy_workqueue(tb->wq);
+       ida_simple_remove(&tb_domain_ida, tb->index);
+       kfree(tb);
+}

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

Quoted 6 lines · Mika Westerberg

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

Quoted 4 lines

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

What is the rationale for using a bus? Do you intend to attach a driver to each domain and/or switch? Unless you do, a class might be more appropriate.

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/Makefile     |   2 +-
 drivers/thunderbolt/domain.c     | 230 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c        |  31 ++++--
 drivers/thunderbolt/tb.c         | 156 ++++++++++++--------------
 drivers/thunderbolt/tb.h         |  70 +++++++++---
 drivers/thunderbolt/tunnel_pci.c |   9 +-
 6 files changed, 377 insertions(+), 121 deletions(-)
 create mode 100644 drivers/thunderbolt/domain.c

drivers/thunderbolt/Makefile +1 -1

index 5d1053cdfa54..e276a9a62261 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-
+thunderbolt-objs += domain.o

drivers/thunderbolt/domain.c new file +9 -0

new file mode 100644
index 000000000000..b5bfca95415d
@@ -0,0 +1,230 @@
+/*
+ * Thunderbolt bus support
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Author:  Mika Westerberg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.

Quoted 58 lines · Mika Westerberg

Thunderbolt fabric consists of one or more switches. This fabric is called domain and it is controlled by an entity called connection manager. The connection manager can be either internal (driven by a firmware running on the host controller) or external (software driver). This driver currently implements support for the latter.

In order to manage switches and their properties more easily we model this domain structure as a Linux bus. Each host controller adds a domain device to this bus, and these devices are named as domainN where N stands for index or id of the current domain.

We then abstract connection manager specific operations into a new structure tb_cm_ops and convert the existing tb.c to fill those accordingly. This makes it easier to add support for the internal connection manager in subsequent patches.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Yehezkel Bernat <[email protected]>

Reviewed-by Michael Jamet <[email protected]>

---
 drivers/thunderbolt/Makefile     |   2 +-
 drivers/thunderbolt/domain.c     | 230 +++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c        |  31 ++++--
 drivers/thunderbolt/tb.c         | 156 ++++++++++++--------------
 drivers/thunderbolt/tb.h         |  70 +++++++++---
 drivers/thunderbolt/tunnel_pci.c |   9 +-
 6 files changed, 377 insertions(+), 121 deletions(-)
 create mode 100644 drivers/thunderbolt/domain.c

drivers/thunderbolt/Makefile +1 -1

index 5d1053cdfa54..e276a9a62261 100644
@@ -1,3 +1,3 @@
 obj-${CONFIG_THUNDERBOLT} := thunderbolt.o
 thunderbolt-objs := nhi.o ctl.o tb.o switch.o cap.o path.o tunnel_pci.o eeprom.o
-
+thunderbolt-objs += domain.o

drivers/thunderbolt/domain.c new file +14 -0

new file mode 100644
index 000000000000..b5bfca95415d
@@ -0,0 +1,230 @@
+/*
+ * Thunderbolt bus support
+ *
+ * Copyright (C) 2017, Intel Corporation
+ * Author:  Mika Westerberg <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/idr.h>
+#include <linux/module.h>

drivers/thunderbolt/domain.c line 19

Quoted 5 lines

drivers/thunderbolt/domain.c new file +5 -0

+#include <linux/slab.h>
+
+#include "tb.h"
+
+static DEFINE_IDA(tb_domain_ida);

You forgot to clean this up in your module exit code with a call to ida_destroy(&tb_domain_ida);

Yeah, it's not obvious, I've made the same mistake before :)

Other than that minor thing, the bus logic looks good, nice job.