| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BN4PR11MB0996AFB1FB6B9902C20FF6A8E6F80@BN4PR11MB0996.namprd11.prod.outlook.com>
Date: Mon, 22 May 2017 10:35:08 +0000
From: "Bernat, Yehezkel" <yehezkel.bernat@...el.com>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>,
Andreas Noever <andreas.noever@...il.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Jamet, Michael" <michael.jamet@...el.com>,
Lukas Wunner <lukas@...ner.de>,
"Levy, Amir (Jer)" <amir.jer.levy@...el.com>,
Andy Lutomirski <luto@...nel.org>,
"Mario.Limonciello@...l.com" <Mario.Limonciello@...l.com>,
"Jared.Dominguez@...l.com" <Jared.Dominguez@...l.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 04/24] thunderbolt: Add MSI-X support
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@...ux.intel.com]
> Sent: Monday, May 22, 2017 11:52
> To: Andreas Noever <andreas.noever@...il.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Jamet, Michael
> <michael.jamet@...el.com>; Bernat, Yehezkel
> <yehezkel.bernat@...el.com>; Lukas Wunner <lukas@...ner.de>; Levy,
> Amir (Jer) <amir.jer.levy@...el.com>; Andy Lutomirski <luto@...nel.org>;
> Mario.Limonciello@...l.com; Jared.Dominguez@...l.com; Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com>; linux-kernel@...r.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.westerberg@...ux.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.jamet@...el.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > > Reviewed-by: Yehezkel Bernat <yehezkel.bernat@...el.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).
Powered by blists - more mailing lists