Patchwork sata_rcar: fix interrupt handling

login
register
mail settings
Submitter Sergei Shtylyov
Date May 31, 2013, 10:38 p.m.
Message ID <201306010238.35442.sergei.shtylyov@cogentembedded.com>
Download mbox | patch
Permalink /patch/248033/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Sergei Shtylyov - May 31, 2013, 10:38 p.m.
The driver's interrupt handling code is too picky in deciding whether it should
handle an interrupt or not which causes completely unneeded spurious interrupts.
Thus make sata_rcar_{ata|serr}_interrupt() *void*; add ATA status register read
to sata_rcar_ata_interrupt() to clear an unexpected ATA interrupt -- it doesn't
get cleared by writing to the SATAINTSTAT register in the interrupt mode we use.

Also, in sata_rcar_ata_interrupt() we should check SATAINTSTAT register only for
enabled interrupts and we should clear  only those interrupts  that we have read
as active first time around, because else we have  a  race and risk clearing  an
interrupt that  can  occur between read  and write of the  SATAINTSTAT  register
and never registering it...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
This patch is against 'for-3.10-fixes' branch of Tejun Heo's 'libata.git' repo.
It will cause some rejects when merged to 'for-3.11' branch... 

 drivers/ata/sata_rcar.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - May 31, 2013, 11:06 p.m.
Hello.

On 06/01/2013 02:38 AM, Sergei Shtylyov wrote:

> The driver's interrupt handling code is too picky in deciding whether it should
> handle an interrupt or not which causes completely unneeded spurious interrupts.
> Thus make sata_rcar_{ata|serr}_interrupt() *void*; add ATA status register read
> to sata_rcar_ata_interrupt() to clear an unexpected ATA interrupt -- it doesn't
> get cleared by writing to the SATAINTSTAT register in the interrupt mode we use.
>
> Also, in sata_rcar_ata_interrupt() we should check SATAINTSTAT register only for
> enabled interrupts and we should clear  only those interrupts  that we have read
> as active first time around, because else we have  a  race and risk clearing  an
> interrupt that  can  occur between read  and write of the  SATAINTSTAT  register
> and never registering it...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

     Darn, I forgot to Cc: stable@vger.kernel.org again.

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 2, 2013, 7:58 a.m.
On Sat, Jun 01, 2013 at 03:06:24AM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 06/01/2013 02:38 AM, Sergei Shtylyov wrote:
> 
> >The driver's interrupt handling code is too picky in deciding whether it should
> >handle an interrupt or not which causes completely unneeded spurious interrupts.
> >Thus make sata_rcar_{ata|serr}_interrupt() *void*; add ATA status register read
> >to sata_rcar_ata_interrupt() to clear an unexpected ATA interrupt -- it doesn't
> >get cleared by writing to the SATAINTSTAT register in the interrupt mode we use.
> >
> >Also, in sata_rcar_ata_interrupt() we should check SATAINTSTAT register only for
> >enabled interrupts and we should clear  only those interrupts  that we have read
> >as active first time around, because else we have  a  race and risk clearing  an
> >interrupt that  can  occur between read  and write of the  SATAINTSTAT  register
> >and never registering it...
> >
> >Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
>     Darn, I forgot to Cc: stable@vger.kernel.org again.

Applied to libata/for-3.10-fixes w/ stable cc'd.  BTW, the driver
generates the following compile warning.

   CC      drivers/ata/sata_rcar.o
   drivers/ata/sata_rcar.c: In function ‘sata_rcar_thaw’:
   drivers/ata/sata_rcar.c:183:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
   
Care to fix it?

Thanks.
Sergei Shtylyov - June 2, 2013, 11:39 a.m.
Hello.

On 02-06-2013 11:58, Tejun Heo wrote:

>>> The driver's interrupt handling code is too picky in deciding whether it should
>>> handle an interrupt or not which causes completely unneeded spurious interrupts.
>>> Thus make sata_rcar_{ata|serr}_interrupt() *void*; add ATA status register read
>>> to sata_rcar_ata_interrupt() to clear an unexpected ATA interrupt -- it doesn't
>>> get cleared by writing to the SATAINTSTAT register in the interrupt mode we use.

>>> Also, in sata_rcar_ata_interrupt() we should check SATAINTSTAT register only for
>>> enabled interrupts and we should clear  only those interrupts  that we have read
>>> as active first time around, because else we have  a  race and risk clearing  an
>>> interrupt that  can  occur between read  and write of the  SATAINTSTAT  register
>>> and never registering it...

>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>>      Darn, I forgot to Cc: stable@vger.kernel.org again.

> Applied to libata/for-3.10-fixes w/ stable cc'd.  BTW, the driver
> generates the following compile warning.

>     CC      drivers/ata/sata_rcar.o
>     drivers/ata/sata_rcar.c: In function ‘sata_rcar_thaw’:
>     drivers/ata/sata_rcar.c:183:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]

     It compiles without warnings for me, IIRC. What version of gcc are 
you using?

> Care to fix it?

    I'll look into it...

> Thanks.

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - June 2, 2013, 5 p.m.
Hello.

On 06/02/2013 03:39 PM, Sergei Shtylyov wrote:

>
>>>      Darn, I forgot to Cc: stable@vger.kernel.org again.
>
>> Applied to libata/for-3.10-fixes w/ stable cc'd.  BTW, the driver
>> generates the following compile warning.
>
>>     CC      drivers/ata/sata_rcar.o
>>     drivers/ata/sata_rcar.c: In function ‘sata_rcar_thaw’:
>>     drivers/ata/sata_rcar.c:183:2: warning: large integer implicitly 
>> truncated to unsigned type [-Woverflow]
>
>     It compiles without warnings for me, IIRC. What version of gcc are 
> you using?

>> Care to fix it?
>
>    I'll look into it...


     I tried 4.2.0, 4.3.3 (that one broke with segmentation fault), and 
4.7.3 and no warning...
So, please tell me what version you have for the changelog.

>> Thanks.
>

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 3, 2013, 3:49 a.m.
Hey,

On Sun, Jun 02, 2013 at 09:00:11PM +0400, Sergei Shtylyov wrote:
> >>    CC      drivers/ata/sata_rcar.o
> >>    drivers/ata/sata_rcar.c: In function ‘sata_rcar_thaw’:
> >>    drivers/ata/sata_rcar.c:183:2: warning: large integer
> >>implicitly truncated to unsigned type [-Woverflow]
> >
> >    It compiles without warnings for me, IIRC. What version of gcc
> >are you using?
> 
> >>Care to fix it?
> >
> >   I'll look into it...
> 
>     I tried 4.2.0, 4.3.3 (that one broke with segmentation fault),
> and 4.7.3 and no warning...
> So, please tell me what version you have for the changelog.

I'm on fedora 19 beta x86-64.

$ gcc --version
gcc (GCC) 4.8.0 20130526 (Red Hat 4.8.0-8)
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

# make drivers/ata/sata_rcar.o
...
  CC      drivers/ata/sata_rcar.o
  drivers/ata/sata_rcar.c: In function ‘sata_rcar_thaw’:
  drivers/ata/sata_rcar.c:183:2: warning: large integer implicitly truncated to unsigned type [-Woverflow]
    iowrite32(~SATA_RCAR_INT_MASK, priv->base + SATAINTSTAT_REG);
    ^
Thanks.

Patch

Index: libata/drivers/ata/sata_rcar.c
===================================================================
--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -619,17 +619,16 @@  static struct ata_port_operations sata_r
 	.bmdma_status		= sata_rcar_bmdma_status,
 };
 
-static int sata_rcar_serr_interrupt(struct ata_port *ap)
+static void sata_rcar_serr_interrupt(struct ata_port *ap)
 {
 	struct sata_rcar_priv *priv = ap->host->private_data;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	int freeze = 0;
-	int handled = 0;
 	u32 serror;
 
 	serror = ioread32(priv->base + SCRSERR_REG);
 	if (!serror)
-		return 0;
+		return;
 
 	DPRINTK("SError @host_intr: 0x%x\n", serror);
 
@@ -642,7 +641,6 @@  static int sata_rcar_serr_interrupt(stru
 		ata_ehi_push_desc(ehi, "%s", "hotplug");
 
 		freeze = serror & SERR_COMM_WAKE ? 0 : 1;
-		handled = 1;
 	}
 
 	/* freeze or abort */
@@ -650,11 +648,9 @@  static int sata_rcar_serr_interrupt(stru
 		ata_port_freeze(ap);
 	else
 		ata_port_abort(ap);
-
-	return handled;
 }
 
-static int sata_rcar_ata_interrupt(struct ata_port *ap)
+static void sata_rcar_ata_interrupt(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc;
 	int handled = 0;
@@ -663,7 +659,9 @@  static int sata_rcar_ata_interrupt(struc
 	if (qc)
 		handled |= ata_bmdma_port_intr(ap, qc);
 
-	return handled;
+	/* be sure to clear ATA interrupt */
+	if (!handled)
+		sata_rcar_check_status(ap);
 }
 
 static irqreturn_t sata_rcar_interrupt(int irq, void *dev_instance)
@@ -678,20 +676,21 @@  static irqreturn_t sata_rcar_interrupt(i
 	spin_lock_irqsave(&host->lock, flags);
 
 	sataintstat = ioread32(priv->base + SATAINTSTAT_REG);
+	sataintstat &= SATA_RCAR_INT_MASK;
 	if (!sataintstat)
 		goto done;
 	/* ack */
-	iowrite32(sataintstat & ~SATA_RCAR_INT_MASK,
-		 priv->base + SATAINTSTAT_REG);
+	iowrite32(~sataintstat & 0x7ff, priv->base + SATAINTSTAT_REG);
 
 	ap = host->ports[0];
 
 	if (sataintstat & SATAINTSTAT_ATA)
-		handled |= sata_rcar_ata_interrupt(ap);
+		sata_rcar_ata_interrupt(ap);
 
 	if (sataintstat & SATAINTSTAT_SERR)
-		handled |= sata_rcar_serr_interrupt(ap);
+		sata_rcar_serr_interrupt(ap);
 
+	handled = 1;
 done:
 	spin_unlock_irqrestore(&host->lock, flags);