> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com] > Sent: Monday, May 22, 2017 11:52 > To: Andreas Noever <andreas.noe...@gmail.com> > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; Jamet, Michael > <michael.ja...@intel.com>; Bernat, Yehezkel > <yehezkel.ber...@intel.com>; Lukas Wunner <lu...@wunner.de>; Levy, > Amir (Jer) <amir.jer.l...@intel.com>; Andy Lutomirski <l...@kernel.org>; > mario.limoncie...@dell.com; jared.doming...@dell.com; Andy Shevchenko > <andriy.shevche...@linux.intel.com>; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 04/24] thunderbolt: Add MSI-X support > > On Sun, May 21, 2017 at 07:51:09PM +0200, Andreas Noever wrote: > > On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg > > <mika.westerb...@linux.intel.com> wrote: > > > 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?
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? > > > > 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 <michael.ja...@intel.com> > > > Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com> > > > Reviewed-by: Yehezkel Bernat <yehezkel.ber...@intel.com> > > > --- > > > 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(-) > > > > > > diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c > > > index 1031d97407a8..889a32dd21e7 100644 > > > --- a/drivers/thunderbolt/ctl.c > > > +++ b/drivers/thunderbolt/ctl.c > > > @@ -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; > > > > > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c > > > index a8c20413dbda..17f3b1bdb7da 100644 > > > --- a/drivers/thunderbolt/nhi.c > > > +++ b/drivers/thunderbolt/nhi.c > > > @@ -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; > > > } > > > > > > +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. > > > > + 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); > > > } > > > > > > + 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. > > > > @@ -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); > > > + 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).