* [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode [not found] <cover.1411031922.git.agordeev@redhat.com> @ 2014-09-19 8:25 ` Alexander Gordeev 2014-09-19 11:33 ` Alexander Gordeev 2014-09-19 16:34 ` Dan Williams 0 siblings, 2 replies; 4+ messages in thread From: Alexander Gordeev @ 2014-09-19 8:25 UTC (permalink / raw) To: linux-kernel; +Cc: linux-ide, Alexander Gordeev, Jiang, Dave As described in AHCI v1.0 specification chapter 10.6.2.2 "Multiple MSI Based Messages" generation of interrupts is not controlled through the HOST_IRQ_STAT register. Considering MMIO access is expensive remove unnecessary reading and writing of HOST_IRQ_STAT register. Further, serializing access to the host data is no longer needed and the interrupt service routine can avoid competing on the host lock. Signed-off-by: Alexander Gordeev <agordeev@redhat.com> Suggested-by: "Jiang, Dave" <dave.jiang@intel.com> Cc: linux-ide@vger.kernel.org --- drivers/ata/ahci.h | 1 + drivers/ata/libahci.c | 54 ++++++++------------------------------------------- 2 files changed, 9 insertions(+), 46 deletions(-) diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index c12f590..b8e117a 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -305,6 +305,7 @@ struct ahci_port_priv { unsigned int ncq_saw_dmas:1; unsigned int ncq_saw_sdb:1; u32 intr_status; /* interrupts to handle */ + spinlock_t intr_lock; /* protects intr_status */ spinlock_t lock; /* protects parent ata_port */ u32 intr_mask; /* interrupts to enable */ bool fbs_supported; /* set iff FBS is supported */ diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 169c272..b9c4e93 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1785,11 +1785,11 @@ irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance) unsigned long flags; u32 status; - spin_lock_irqsave(&ap->host->lock, flags); + spin_lock_irqsave(&pp->intr_lock, flags); status = pp->intr_status; if (status) pp->intr_status = 0; - spin_unlock_irqrestore(&ap->host->lock, flags); + spin_unlock_irqrestore(&pp->intr_lock, flags); spin_lock_bh(ap->lock); ahci_handle_port_interrupt(ap, status); @@ -1842,53 +1842,14 @@ static void ahci_update_intr_status(struct ata_port *ap) irqreturn_t ahci_multi_irqs_intr(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; + struct ata_port *ap = dev_instance; + struct ahci_port_priv *pp = ap->private_data; VPRINTK("ENTER\n"); - spin_lock(&host->lock); - - irq_stat = readl(mmio + HOST_IRQ_STAT); - - if (!irq_stat) { - u32 status = pp->intr_status; - - spin_unlock(&host->lock); - - VPRINTK("EXIT\n"); - - return status ? IRQ_WAKE_THREAD : IRQ_NONE; - } - - irq_masked = irq_stat & hpriv->port_map; - - for (i = 0; i < host->n_ports; i++) { - struct ata_port *ap; - - if (!(irq_masked & (1 << i))) - continue; - - ap = host->ports[i]; - if (ap) { - ahci_update_intr_status(ap); - VPRINTK("port %u\n", i); - } else { - VPRINTK("port %u (no irq)\n", i); - if (ata_ratelimit()) - dev_warn(host->dev, - "interrupt on disabled port %u\n", i); - } - } - - writel(irq_stat, mmio + HOST_IRQ_STAT); - - spin_unlock(&host->lock); + spin_lock(&pp->intr_lock); + ahci_update_intr_status(ap); + spin_unlock(&pp->intr_lock); VPRINTK("EXIT\n"); @@ -2366,6 +2327,7 @@ static int ahci_port_start(struct ata_port *ap) */ pp->intr_mask = DEF_PORT_IRQ; + spin_lock_init(&pp->intr_lock); spin_lock_init(&pp->lock); ap->lock = &pp->lock; -- 1.8.3.1 -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode 2014-09-19 8:25 ` [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode Alexander Gordeev @ 2014-09-19 11:33 ` Alexander Gordeev 2014-09-19 16:34 ` Dan Williams 1 sibling, 0 replies; 4+ messages in thread From: Alexander Gordeev @ 2014-09-19 11:33 UTC (permalink / raw) To: linux-kernel; +Cc: linux-ide, Jiang, Dave On Fri, Sep 19, 2014 at 09:25:52AM +0100, Alexander Gordeev wrote: > As described in AHCI v1.0 specification chapter 10.6.2.2 > "Multiple MSI Based Messages" generation of interrupts > is not controlled through the HOST_IRQ_STAT register. > > Considering MMIO access is expensive remove unnecessary > reading and writing of HOST_IRQ_STAT register. > > Further, serializing access to the host data is no longer > needed and the interrupt service routine can avoid competing > on the host lock. > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > Suggested-by: "Jiang, Dave" <dave.jiang@intel.com> > Cc: linux-ide@vger.kernel.org > --- > drivers/ata/ahci.h | 1 + > drivers/ata/libahci.c | 54 ++++++++------------------------------------------- > 2 files changed, 9 insertions(+), 46 deletions(-) > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index c12f590..b8e117a 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -305,6 +305,7 @@ struct ahci_port_priv { > unsigned int ncq_saw_dmas:1; > unsigned int ncq_saw_sdb:1; > u32 intr_status; /* interrupts to handle */ > + spinlock_t intr_lock; /* protects intr_status */ > spinlock_t lock; /* protects parent ata_port */ > u32 intr_mask; /* interrupts to enable */ > bool fbs_supported; /* set iff FBS is supported */ > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 169c272..b9c4e93 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1785,11 +1785,11 @@ irqreturn_t ahci_port_thread_fn(int irq, void *dev_instance) > unsigned long flags; > u32 status; > > - spin_lock_irqsave(&ap->host->lock, flags); > + spin_lock_irqsave(&pp->intr_lock, flags); > status = pp->intr_status; > if (status) > pp->intr_status = 0; > - spin_unlock_irqrestore(&ap->host->lock, flags); > + spin_unlock_irqrestore(&pp->intr_lock, flags); This change ^^^ is wrong in single interrupt mode. Self-Nack. > spin_lock_bh(ap->lock); > ahci_handle_port_interrupt(ap, status); > @@ -1842,53 +1842,14 @@ static void ahci_update_intr_status(struct ata_port *ap) > > irqreturn_t ahci_multi_irqs_intr(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; > + struct ata_port *ap = dev_instance; > + struct ahci_port_priv *pp = ap->private_data; > > VPRINTK("ENTER\n"); > > - spin_lock(&host->lock); > - > - irq_stat = readl(mmio + HOST_IRQ_STAT); > - > - if (!irq_stat) { > - u32 status = pp->intr_status; > - > - spin_unlock(&host->lock); > - > - VPRINTK("EXIT\n"); > - > - return status ? IRQ_WAKE_THREAD : IRQ_NONE; > - } > - > - irq_masked = irq_stat & hpriv->port_map; > - > - for (i = 0; i < host->n_ports; i++) { > - struct ata_port *ap; > - > - if (!(irq_masked & (1 << i))) > - continue; > - > - ap = host->ports[i]; > - if (ap) { > - ahci_update_intr_status(ap); > - VPRINTK("port %u\n", i); > - } else { > - VPRINTK("port %u (no irq)\n", i); > - if (ata_ratelimit()) > - dev_warn(host->dev, > - "interrupt on disabled port %u\n", i); > - } > - } > - > - writel(irq_stat, mmio + HOST_IRQ_STAT); > - > - spin_unlock(&host->lock); > + spin_lock(&pp->intr_lock); > + ahci_update_intr_status(ap); > + spin_unlock(&pp->intr_lock); > > VPRINTK("EXIT\n"); > > @@ -2366,6 +2327,7 @@ static int ahci_port_start(struct ata_port *ap) > */ > pp->intr_mask = DEF_PORT_IRQ; > > + spin_lock_init(&pp->intr_lock); > spin_lock_init(&pp->lock); > ap->lock = &pp->lock; > > -- > 1.8.3.1 > > -- > Regards, > Alexander Gordeev > agordeev@redhat.com -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode 2014-09-19 8:25 ` [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode Alexander Gordeev 2014-09-19 11:33 ` Alexander Gordeev @ 2014-09-19 16:34 ` Dan Williams 2014-09-19 18:39 ` Alexander Gordeev 1 sibling, 1 reply; 4+ messages in thread From: Dan Williams @ 2014-09-19 16:34 UTC (permalink / raw) To: Alexander Gordeev Cc: Linux Kernel Mailing List, IDE/ATA development list, Jiang, Dave On Fri, Sep 19, 2014 at 1:25 AM, Alexander Gordeev <agordeev@redhat.com> wrote: > As described in AHCI v1.0 specification chapter 10.6.2.2 > "Multiple MSI Based Messages" generation of interrupts > is not controlled through the HOST_IRQ_STAT register. > > Considering MMIO access is expensive remove unnecessary > reading and writing of HOST_IRQ_STAT register. > > Further, serializing access to the host data is no longer > needed and the interrupt service routine can avoid competing > on the host lock. > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > Suggested-by: "Jiang, Dave" <dave.jiang@intel.com> > Cc: linux-ide@vger.kernel.org > --- > drivers/ata/ahci.h | 1 + > drivers/ata/libahci.c | 54 ++++++++------------------------------------------- > 2 files changed, 9 insertions(+), 46 deletions(-) > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index c12f590..b8e117a 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -305,6 +305,7 @@ struct ahci_port_priv { > unsigned int ncq_saw_dmas:1; > unsigned int ncq_saw_sdb:1; > u32 intr_status; /* interrupts to handle */ > + spinlock_t intr_lock; /* protects intr_status */ Why introduce a new lock? Can't we switch to per-ata port locking rather than ata_host locking? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode 2014-09-19 16:34 ` Dan Williams @ 2014-09-19 18:39 ` Alexander Gordeev 0 siblings, 0 replies; 4+ messages in thread From: Alexander Gordeev @ 2014-09-19 18:39 UTC (permalink / raw) To: Dan Williams Cc: Linux Kernel Mailing List, IDE/ATA development list, Jiang, Dave On Fri, Sep 19, 2014 at 09:34:34AM -0700, Dan Williams wrote: > On Fri, Sep 19, 2014 at 1:25 AM, Alexander Gordeev <agordeev@redhat.com> wrote: > > As described in AHCI v1.0 specification chapter 10.6.2.2 > > "Multiple MSI Based Messages" generation of interrupts > > is not controlled through the HOST_IRQ_STAT register. > > > > Considering MMIO access is expensive remove unnecessary > > reading and writing of HOST_IRQ_STAT register. > > > > Further, serializing access to the host data is no longer > > needed and the interrupt service routine can avoid competing > > on the host lock. > > > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > > Suggested-by: "Jiang, Dave" <dave.jiang@intel.com> > > Cc: linux-ide@vger.kernel.org > > --- > > drivers/ata/ahci.h | 1 + > > drivers/ata/libahci.c | 54 ++++++++------------------------------------------- > > 2 files changed, 9 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > > index c12f590..b8e117a 100644 > > --- a/drivers/ata/ahci.h > > +++ b/drivers/ata/ahci.h > > @@ -305,6 +305,7 @@ struct ahci_port_priv { > > unsigned int ncq_saw_dmas:1; > > unsigned int ncq_saw_sdb:1; > > u32 intr_status; /* interrupts to handle */ > > + spinlock_t intr_lock; /* protects intr_status */ > > Why introduce a new lock? Can't we switch to per-ata port locking > rather than ata_host locking? We could. But this case hardware context interrupt handler would compete with threads/softriqs, which is exactly what I tried to avoid. With the separate lock we *only* update ahci_port_priv::intr_status with interrupts disabled. -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-19 18:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1411031922.git.agordeev@redhat.com>
2014-09-19 8:25 ` [PATCH v3 6/5] AHCI: Do not read HOST_IRQ_STAT register in multi-MSI mode Alexander Gordeev
2014-09-19 11:33 ` Alexander Gordeev
2014-09-19 16:34 ` Dan Williams
2014-09-19 18:39 ` Alexander Gordeev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).