| Message ID | 20190306102146.13005-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable per-port SATA interrupts and drop an hack in the IRQ subsystem | expand |
On 06/03/2019 10:21, Miquel Raynal wrote: > ahci_multi_irqs_intr_hard() is going to be used as interrupt handler > to support SATA per-port interrupts. The current logic is to check and > clear the SATA port interrupt status register only. To avoid spurious > IRQs and interrupt storms, it will be needed to clear the port > interrupt bit in the host interrupt status register as well. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/ata/libahci.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index b5f57c69c487..66d4906a5013 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) > static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > { > struct ata_port *ap = dev_instance; > + struct ata_host *host = ap->host; > + struct ahci_host_priv *hpriv = host->private_data; > void __iomem *port_mmio = ahci_port_base(ap); > + void __iomem *mmio = hpriv->mmio; > u32 status; > > VPRINTK("ENTER\n"); > @@ -1924,6 +1927,10 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > ahci_handle_port_interrupt(ap, port_mmio, status); > spin_unlock(ap->lock); > > + spin_lock(&host->lock); > + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); > + spin_unlock(&host->lock); What's not clear here is under which circumstances this is required. This write should be atomic (if it isn't, you have bigger problems), and it is at best unclear what you're avoiding by taking the host lock. Thanks, M.
Hi Marc, Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 7 Mar 2019 16:25:02 +0000: > On 06/03/2019 10:21, Miquel Raynal wrote: > > ahci_multi_irqs_intr_hard() is going to be used as interrupt handler > > to support SATA per-port interrupts. The current logic is to check and > > clear the SATA port interrupt status register only. To avoid spurious > > IRQs and interrupt storms, it will be needed to clear the port > > interrupt bit in the host interrupt status register as well. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/ata/libahci.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > > index b5f57c69c487..66d4906a5013 100644 > > --- a/drivers/ata/libahci.c > > +++ b/drivers/ata/libahci.c > > @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) > > static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > > { > > struct ata_port *ap = dev_instance; > > + struct ata_host *host = ap->host; > > + struct ahci_host_priv *hpriv = host->private_data; > > void __iomem *port_mmio = ahci_port_base(ap); > > + void __iomem *mmio = hpriv->mmio; > > u32 status; > > > > VPRINTK("ENTER\n"); > > @@ -1924,6 +1927,10 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) > > ahci_handle_port_interrupt(ap, port_mmio, status); > > spin_unlock(ap->lock); > > > > + spin_lock(&host->lock); > > + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); > > + spin_unlock(&host->lock); > > What's not clear here is under which circumstances this is required. > This write should be atomic (if it isn't, you have bigger problems), and > it is at best unclear what you're avoiding by taking the host lock. You're right, taking the lock is not needed. It is a relict of a previous implementation anyway. Thanks, Miquèl
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index b5f57c69c487..66d4906a5013 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap) static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) { struct ata_port *ap = dev_instance; + struct ata_host *host = ap->host; + struct ahci_host_priv *hpriv = host->private_data; void __iomem *port_mmio = ahci_port_base(ap); + void __iomem *mmio = hpriv->mmio; u32 status; VPRINTK("ENTER\n"); @@ -1924,6 +1927,10 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance) ahci_handle_port_interrupt(ap, port_mmio, status); spin_unlock(ap->lock); + spin_lock(&host->lock); + writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT); + spin_unlock(&host->lock); + VPRINTK("EXIT\n"); return IRQ_HANDLED;
ahci_multi_irqs_intr_hard() is going to be used as interrupt handler to support SATA per-port interrupts. The current logic is to check and clear the SATA port interrupt status register only. To avoid spurious IRQs and interrupt storms, it will be needed to clear the port interrupt bit in the host interrupt status register as well. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/ata/libahci.c | 7 +++++++ 1 file changed, 7 insertions(+)