* Alexander Gordeev <agordeev@xxxxxxxxxx> 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 <agordeev@xxxxxxxxxx>
> ---
> 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-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
