Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751796Ab2JBFJh (ORCPT ); Tue, 2 Oct 2012 01:09:37 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:60618 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750753Ab2JBFJf (ORCPT ); Tue, 2 Oct 2012 01:09:35 -0400 Date: Tue, 2 Oct 2012 07:09:29 +0200 From: Ingo Molnar To: Alexander Gordeev Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Thomas Gleixner , Bjorn Helgaas , Suresh Siddha , Yinghai Lu , Jeff Garzik , Matthew Wilcox , x86@kernel.org, linux-pci@vger.kernel.org, linux-ide@vger.kernel.org Subject: Re: [PATCH v3 -tip 5/5] AHCI: Support multiple MSIs Message-ID: <20121002050929.GD7756@gmail.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10099 Lines: 311 * Alexander Gordeev wrote: > Take advantage of multiple MSIs implementation on x86 - on systems with > IRQ remapping AHCI ports not only get assigned separate MSI vectors - > but also separate IRQs. As result, interrupts generated by different > ports could be serviced on different CPUs rather than on a single one. > > In cases when number of allocated MSIs is less than requested the Sharing > Last MSI mode does not get used, no matter implemented in hardware or not. > Instead, the driver assumes the advantage of multiple MSIs is negated and > falls back to the single MSI mode as if MRSM bit was set (some Intel chips > implement this strategy anyway - MRSM bit gets set even if the number of > allocated MSIs exceeds the number of implemented ports). > > Signed-off-by: Alexander Gordeev > --- > drivers/ata/ahci.c | 91 ++++++++++++++++++++++++++++++++++++-- > drivers/ata/ahci.h | 6 +++ > drivers/ata/libahci.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 205 insertions(+), 10 deletions(-) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 7862d17..5463fcea 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -1057,6 +1057,84 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host) > {} > #endif > > +int ahci_init_interrupts(struct pci_dev *pdev, struct ahci_host_priv *hpriv) > +{ > + int rc; > + unsigned int maxvec; > + > + if (!(hpriv->flags & AHCI_HFLAG_NO_MSI)) { > + rc = pci_enable_msi_block_auto(pdev, &maxvec); > + if (rc > 0) { > + if ((rc == maxvec) || (rc == 1)) > + return rc; > + /* assume that advantage of multipe MSIs is negated, > + * so fallback to single MSI mode to save resources */ Please use the customary (multi-line) comment style: /* * Comment ..... * ...... goes here. */ specified in Documentation/CodingStyle. > + pci_disable_msi(pdev); > + if (!pci_enable_msi(pdev)) > + return 1; > + } > + } > + > + pci_intx(pdev, 1); > + return 0; > +} > + > +/** > + * ahci_host_activate - start AHCI host, request IRQs and register it > + * @host: target ATA host > + * @irq: base IRQ number to request > + * @n_msis: number of MSIs allocated for this host > + * @irq_handler: irq_handler used when requesting IRQs > + * @irq_flags: irq_flags used when requesting IRQs > + * > + * Similar to ata_host_activate, but requests IRQs according to AHCI-1.1 > + * when multiple MSIs were allocated. That is one MSI per port, starting > + * from @irq. > + * > + * LOCKING: > + * Inherited from calling layer (may sleep). > + * > + * RETURNS: > + * 0 on success, -errno otherwise. > + */ > +int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) > +{ > + int i, rc; > + > + /* Sharing Last Message among several ports is not supported */ > + if (n_msis < host->n_ports) > + return -EINVAL; > + > + rc = ata_host_start(host); > + if (rc) > + return rc; > + > + for (i = 0; i < host->n_ports; i++) { > + rc = devm_request_threaded_irq(host->dev, > + irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED, > + dev_driver_string(host->dev), host->ports[i]); > + if (rc) > + goto out_free_irqs; > + } > + > + for (i = 0; i < host->n_ports; i++) > + ata_port_desc(host->ports[i], "irq %d", irq + i); > + > + rc = ata_host_register(host, &ahci_sht); > + if (rc) > + goto out_free_all_irqs; > + > + return 0; > + > +out_free_all_irqs: > + i = host->n_ports; > +out_free_irqs: > + for (; i; i--) > + devm_free_irq(host->dev, irq + i - 1, host->ports[i]); Please test the failure path somehow - for example I'm quite sure that the line above is buggy and will crash/hang a real system: it should be host->ports[i-1]. Writing it as: devm_free_irq(host->dev, irq + i-1, host->ports[i-1]); would help readability as well as bit - or just: for (i--; i >= 0; i--) devm_free_irq(host->dev, irq + i, host->ports[i]); (untested) > + > + return rc; > +} > + > static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > { > unsigned int board_id = ent->driver_data; > @@ -1065,7 +1143,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > struct device *dev = &pdev->dev; > struct ahci_host_priv *hpriv; > struct ata_host *host; > - int n_ports, i, rc; > + int n_ports, n_msis, i, rc; > int ahci_pci_bar = AHCI_PCI_BAR_STANDARD; > > VPRINTK("ENTER\n"); > @@ -1150,11 +1228,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (ahci_sb600_enable_64bit(pdev)) > hpriv->flags &= ~AHCI_HFLAG_32BIT_ONLY; > > - if ((hpriv->flags & AHCI_HFLAG_NO_MSI) || pci_enable_msi(pdev)) > - pci_intx(pdev, 1); > - > hpriv->mmio = pcim_iomap_table(pdev)[ahci_pci_bar]; > > + n_msis = ahci_init_interrupts(pdev, hpriv); > + if (n_msis > 1) > + hpriv->flags |= AHCI_HFLAG_MULTI_MSI; > + > /* save initial config */ > ahci_pci_save_initial_config(pdev, hpriv); > > @@ -1250,6 +1329,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > ahci_pci_print_info(host); > > pci_set_master(pdev); > + > + if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) > + return ahci_host_activate(host, pdev->irq, n_msis); > + > return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED, > &ahci_sht); > } > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 57eb1c2..24251e8 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -216,6 +216,7 @@ enum { > AHCI_HFLAG_DELAY_ENGINE = (1 << 15), /* do not start engine on > port start (wait until > error-handling stage) */ > + AHCI_HFLAG_MULTI_MSI = (1 << 16), /* multiple PCI MSIs */ > > /* ap->flags bits */ > > @@ -282,6 +283,8 @@ struct ahci_port_priv { > unsigned int ncq_saw_d2h:1; > unsigned int ncq_saw_dmas:1; > unsigned int ncq_saw_sdb:1; > + u32 intr_status; /* interrupts to handle */ > + spinlock_t lock; In general it's nice to add a small comment that explains what data the lock protects precisely and in what circumstances it's used - especially as it's in the middle of a structure, making it unclear at first sight whether it's for the whole ahci_port descriptor or just part of it. > u32 intr_mask; /* interrupts to enable */ > bool fbs_supported; /* set iff FBS is supported */ > bool fbs_enabled; /* set iff FBS is enabled */ > @@ -343,7 +346,10 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv, > struct ata_port_info *pi); > int ahci_reset_em(struct ata_host *host); > irqreturn_t ahci_interrupt(int irq, void *dev_instance); > +irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance); > +irqreturn_t ahci_thread_fn(int irq, void *dev_instance); > void ahci_print_info(struct ata_host *host, const char *scc_s); > +int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis); > > static inline void __iomem *__ahci_port_base(struct ata_host *host, > unsigned int port_no) > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 555c07a..3b54e84 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1639,19 +1639,16 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat) > ata_port_abort(ap); > } > > -static void ahci_port_intr(struct ata_port *ap) > +static void ahci_handle_port_interrupt(struct ata_port *ap, > + void __iomem *port_mmio, u32 status) > { > - void __iomem *port_mmio = ahci_port_base(ap); > struct ata_eh_info *ehi = &ap->link.eh_info; > struct ahci_port_priv *pp = ap->private_data; > struct ahci_host_priv *hpriv = ap->host->private_data; > int resetting = !!(ap->pflags & ATA_PFLAG_RESETTING); > - u32 status, qc_active = 0; > + u32 qc_active = 0; > int rc; > > - status = readl(port_mmio + PORT_IRQ_STAT); > - writel(status, port_mmio + PORT_IRQ_STAT); > - > /* ignore BAD_PMP while resetting */ > if (unlikely(resetting)) > status &= ~PORT_IRQ_BAD_PMP; > @@ -1727,6 +1724,107 @@ static void ahci_port_intr(struct ata_port *ap) > } > } > > +void ahci_port_intr(struct ata_port *ap) > +{ > + void __iomem *port_mmio = ahci_port_base(ap); > + u32 status; > + > + status = readl(port_mmio + PORT_IRQ_STAT); > + writel(status, port_mmio + PORT_IRQ_STAT); > + > + ahci_handle_port_interrupt(ap, port_mmio, status); > +} > + > +irqreturn_t ahci_thread_fn(int irq, void *dev_instance) > +{ > + struct ata_port *ap = dev_instance; > + struct ahci_port_priv *pp = ap->private_data; > + void __iomem *port_mmio = ahci_port_base(ap); > + unsigned long flags; > + u32 status; > + > + spin_lock_irqsave(&ap->host->lock, flags); > + status = pp->intr_status; > + if (status) > + pp->intr_status = 0; > + spin_unlock_irqrestore(&ap->host->lock, flags); > + > + spin_lock_bh(ap->lock); > + ahci_handle_port_interrupt(ap, port_mmio, status); > + spin_unlock_bh(ap->lock); > + > + return IRQ_HANDLED; > +} > +EXPORT_SYMBOL_GPL(ahci_thread_fn); > + > +void ahci_hw_port_interrupt(struct ata_port *ap) > +{ > + void __iomem *port_mmio = ahci_port_base(ap); > + struct ahci_port_priv *pp = ap->private_data; > + u32 status; > + > + status = readl(port_mmio + PORT_IRQ_STAT); > + writel(status, port_mmio + PORT_IRQ_STAT); > + > + pp->intr_status |= status; > +} > + > +irqreturn_t ahci_hw_interrupt(int irq, void *dev_instance) > +{ > + struct ata_port *ap_this = dev_instance; > + struct ahci_port_priv *pp = ap_this->private_data; > + struct ata_host *host = ap_this->host; > + struct ahci_host_priv *hpriv = host->private_data; > + void __iomem *mmio = hpriv->mmio; > + unsigned int i; > + u32 irq_stat, irq_masked; > + > + VPRINTK("ENTER\n"); Is this per IRQ handler execution debugging code still needed? Same for the other VPRINTK() lines in this patch. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/