Re: [PATCH v2 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>, Andrew Lunn <andrew@lunn.ch>,
	Baruch Siach <baruch@tkos.co.il>,
	Jason Cooper <jason@lakedaemon.net>,
	Nadav Haklai <nadavh@marvell.com>,
	devicetree@vger.kernel.org,
	Antoine Tenart <antoine.tenart@bootlin.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Maxime Chevallier <maxime.chevallier@bootlin.com>,
	linux-ide@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Rob Herring <robh+dt@kernel.org>, Jens Axboe <axboe@kernel.dk>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH v2 01/10] ata: libahci: Ensure the host interrupt status bits are cleared
Date: Thu, 7 Mar 2019 18:19:30 +0100	[thread overview]
Message-ID: <20190307181930.7142a9d7@xps13> (raw)
In-Reply-To: <57c2d67c-7411-0fca-933e-a76379f3165c@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel