| Message ID | 201306010238.35442.sergei.shtylyov@cogentembedded.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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
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.
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);
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