[PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[Tejun Heo]

host->ports[i] is never NULL if i < host->n_ports and non-NULL return
from ata_qc_from_tag() guarantees that the returned qc is active.
Drop unnecessary tests.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-sff.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Index: ata/drivers/ata/libata-sff.c
===================================================================
--- ata.orig/drivers/ata/libata-sff.c
+++ ata/drivers/ata/libata-sff.c
@@ -1767,18 +1767,15 @@ irqreturn_t ata_sff_interrupt(int irq, v
 	spin_lock_irqsave(&host->lock, flags);
 
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
+		struct ata_port *ap = host->ports[i];
+		struct ata_queued_cmd *qc;
 
-		ap = host->ports[i];
-		if (ap &&
-		    !(ap->flags & ATA_FLAG_DISABLED)) {
-			struct ata_queued_cmd *qc;
+		if (unlikely(ap->flags & ATA_FLAG_DISABLED))
+			continue;
 
-			qc = ata_qc_from_tag(ap, ap->link.active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
-				handled |= ata_sff_host_intr(ap, qc);
-		}
+		qc = ata_qc_from_tag(ap, ap->link.active_tag);
+		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
+			handled |= ata_sff_host_intr(ap, qc);
 	}
 
 	spin_unlock_irqrestore(&host->lock, flags);

[PATCH #upstream-fixes 2/2] libata: implement spurious irq handling for SFF and apply it to piix

[Tejun Heo]

Traditional IDE interface sucks in that it doesn't have a reliable IRQ
pending bit, so if the controller raises IRQ while the driver is
expecting it not to, the IRQ won't be cleared and eventually the IRQ
line will be killed by interrupt subsystem.  Some controllers have
non-standard mechanism to indicate IRQ pending so that this condition
can be detected and worked around.

This patch adds an optional operation ->sff_irq_check() which will be
called for each port from the ata_sff_interrupt() if an unexpected
interrupt is received.  If the operation returns %true,
->sff_check_status() and ->sff_irq_clear() will be cleared for the
port.  Note that this doesn't mark the interrupt as handled so it
won't prevent IRQ subsystem from killing the IRQ if this mechanism
fails to clear the spurious IRQ.

This patch also implements ->sff_irq_check() for ata_piix.  Note that
this adds slight overhead to shared IRQ operation as IRQs which are
destined for other controllers will trigger extra register accesses to
check whether IDE interrupt is pending but this solves rare screaming
IRQ cases and for some curious reason also helps weird BIOS related
glitch on Samsung n130 as reported in bko#14314.

  http://bugzilla.kernel.org/show_bug.cgi?id=14314

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Johannes Stezenbach <js@sig21.net>
Reported-by: Hans Werner <hwerner4@gmx.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
---
This is generalized safe version of the previous patch which only
worked for ata_piix and marked the interrupt as handled.

Thanks.

 drivers/ata/ata_piix.c   |   18 ++++++++++++++++--
 drivers/ata/libata-sff.c |   22 ++++++++++++++++++++++
 include/linux/libata.h   |    1 +
 3 files changed, 39 insertions(+), 2 deletions(-)

Index: ata/include/linux/libata.h
===================================================================
--- ata.orig/include/linux/libata.h
+++ ata/include/linux/libata.h
@@ -857,6 +857,7 @@ struct ata_port_operations {
 	unsigned int (*sff_data_xfer)(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
 	u8   (*sff_irq_on)(struct ata_port *);
+	bool (*sff_irq_check)(struct ata_port *);
 	void (*sff_irq_clear)(struct ata_port *);
 
 	void (*bmdma_setup)(struct ata_queued_cmd *qc);
Index: ata/drivers/ata/ata_piix.c
===================================================================
--- ata.orig/drivers/ata/ata_piix.c
+++ ata/drivers/ata/ata_piix.c
@@ -173,6 +173,7 @@ static int piix_sidpr_scr_read(struct at
 			       unsigned int reg, u32 *val);
 static int piix_sidpr_scr_write(struct ata_link *link,
 				unsigned int reg, u32 val);
+static bool piix_irq_check(struct ata_port *ap);
 #ifdef CONFIG_PM
 static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
 static int piix_pci_device_resume(struct pci_dev *pdev);
@@ -309,8 +310,13 @@ static struct scsi_host_template piix_sh
 	ATA_BMDMA_SHT(DRV_NAME),
 };
 
-static struct ata_port_operations piix_pata_ops = {
+static struct ata_port_operations piix_base_ops = {
 	.inherits		= &ata_bmdma32_port_ops,
+	.sff_irq_check		= piix_irq_check,
+};
+
+static struct ata_port_operations piix_pata_ops = {
+	.inherits		= &piix_base_ops,
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= piix_set_piomode,
 	.set_dmamode		= piix_set_dmamode,
@@ -329,7 +335,7 @@ static struct ata_port_operations ich_pa
 };
 
 static struct ata_port_operations piix_sata_ops = {
-	.inherits		= &ata_bmdma32_port_ops,
+	.inherits		= &piix_base_ops,
 };
 
 static struct ata_port_operations piix_sidpr_sata_ops = {
@@ -962,6 +968,14 @@ static int piix_sidpr_scr_write(struct a
 	return 0;
 }
 
+static bool piix_irq_check(struct ata_port *ap)
+{
+	if (unlikely(!ap->ioaddr.bmdma_addr))
+		return false;
+
+	return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;
+}
+
 #ifdef CONFIG_PM
 static int piix_broken_suspend(void)
 {
Index: ata/drivers/ata/libata-sff.c
===================================================================
--- ata.orig/drivers/ata/libata-sff.c
+++ ata/drivers/ata/libata-sff.c
@@ -1778,6 +1778,28 @@ irqreturn_t ata_sff_interrupt(int irq, v
 			handled |= ata_sff_host_intr(ap, qc);
 	}
 
+	/*
+	 * If no port was expecting IRQ but the controller is actually
+	 * asserting IRQ line, nobody cared will ensue.  Check IRQ
+	 * pending status if available and clear spurious IRQ.
+	 */
+	if (!handled) {
+		for (i = 0; i < host->n_ports; i++) {
+			struct ata_port *ap = host->ports[i];
+
+			if (!ap->ops->sff_irq_check ||
+			    !ap->ops->sff_irq_check(ap))
+				continue;
+
+			if (printk_ratelimit())
+				ata_port_printk(ap, KERN_INFO,
+						"clearing spurious IRQ\n");
+
+			ap->ops->sff_check_status(ap);
+			ap->ops->sff_irq_clear(ap);
+		}
+	}
+
 	spin_unlock_irqrestore(&host->lock, flags);
 
 	return IRQ_RETVAL(handled);

Re: [PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> host->ports[i] is never NULL if i < host->n_ports and non-NULL return
> from ata_qc_from_tag() guarantees that the returned qc is active.
> Drop unnecessary tests.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/libata-sff.c |   17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> Index: ata/drivers/ata/libata-sff.c
> ===================================================================
> --- ata.orig/drivers/ata/libata-sff.c
> +++ ata/drivers/ata/libata-sff.c
> @@ -1767,18 +1767,15 @@ irqreturn_t ata_sff_interrupt(int irq, v
>  	spin_lock_irqsave(&host->lock, flags);
>  
>  	for (i = 0; i < host->n_ports; i++) {
> -		struct ata_port *ap;
> +		struct ata_port *ap = host->ports[i];
> +		struct ata_queued_cmd *qc;
>  
> -		ap = host->ports[i];
> -		if (ap &&
> -		    !(ap->flags & ATA_FLAG_DISABLED)) {
> -			struct ata_queued_cmd *qc;
> +		if (unlikely(ap->flags & ATA_FLAG_DISABLED))
> +			continue;
>  
> -			qc = ata_qc_from_tag(ap, ap->link.active_tag);
> -			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
> -			    (qc->flags & ATA_QCFLAG_ACTIVE))
> -				handled |= ata_sff_host_intr(ap, qc);
> -		}
> +		qc = ata_qc_from_tag(ap, ap->link.active_tag);
> +		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
>   

   () not needed around !x.

WBR, Sergei

Re: [PATCH #upstream-fixes 2/2] libata: implement spurious irq handling for SFF and apply it to piix

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> Traditional IDE interface sucks in that it doesn't have a reliable IRQ
> pending bit, so if the controller raises IRQ while the driver is
> expecting it not to, the IRQ won't be cleared and eventually the IRQ
> line will be killed by interrupt subsystem.  Some controllers have
> non-standard mechanism to indicate IRQ pending so that this condition
> can be detected and worked around.
>
> This patch adds an optional operation ->sff_irq_check() which will be
> called for each port from the ata_sff_interrupt() if an unexpected
> interrupt is received.  If the operation returns %true,
> ->sff_check_status() and ->sff_irq_clear() will be cleared for the
> port.  Note that this doesn't mark the interrupt as handled so it
> won't prevent IRQ subsystem from killing the IRQ if this mechanism
> fails to clear the spurious IRQ.
>
> This patch also implements ->sff_irq_check() for ata_piix.  Note that
> this adds slight overhead to shared IRQ operation as IRQs which are
> destined for other controllers will trigger extra register accesses to
> check whether IDE interrupt is pending but this solves rare screaming
> IRQ cases and for some curious reason also helps weird BIOS related
> glitch on Samsung n130 as reported in bko#14314.
>
>   http://bugzilla.kernel.org/show_bug.cgi?id=14314
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Johannes Stezenbach <js@sig21.net>
> Reported-by: Hans Werner <hwerner4@gmx.de>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> ---
> This is generalized safe version of the previous patch which only
> worked for ata_piix and marked the interrupt as handled.
>
> Thanks.
>
>  drivers/ata/ata_piix.c   |   18 ++++++++++++++++--
>  drivers/ata/libata-sff.c |   22 ++++++++++++++++++++++
>  include/linux/libata.h   |    1 +
>  3 files changed, 39 insertions(+), 2 deletions(-)
>
> Index: ata/include/linux/libata.h
> ===================================================================
> --- ata.orig/include/linux/libata.h
> +++ ata/include/linux/libata.h
> @@ -857,6 +857,7 @@ struct ata_port_operations {
>  	unsigned int (*sff_data_xfer)(struct ata_device *dev,
>  			unsigned char *buf, unsigned int buflen, int rw);
>  	u8   (*sff_irq_on)(struct ata_port *);
> +	bool (*sff_irq_check)(struct ata_port *);
>  	void (*sff_irq_clear)(struct ata_port *);
>  
>  	void (*bmdma_setup)(struct ata_queued_cmd *qc);
> Index: ata/drivers/ata/ata_piix.c
> ===================================================================
> --- ata.orig/drivers/ata/ata_piix.c
> +++ ata/drivers/ata/ata_piix.c
> @@ -173,6 +173,7 @@ static int piix_sidpr_scr_read(struct at
>  			       unsigned int reg, u32 *val);
>  static int piix_sidpr_scr_write(struct ata_link *link,
>  				unsigned int reg, u32 val);
> +static bool piix_irq_check(struct ata_port *ap);
>  #ifdef CONFIG_PM
>  static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
>  static int piix_pci_device_resume(struct pci_dev *pdev);
> @@ -309,8 +310,13 @@ static struct scsi_host_template piix_sh
>  	ATA_BMDMA_SHT(DRV_NAME),
>  };
>  
> -static struct ata_port_operations piix_pata_ops = {
> +static struct ata_port_operations piix_base_ops = {
>  	.inherits		= &ata_bmdma32_port_ops,
> +	.sff_irq_check		= piix_irq_check,
> +};
> +
> +static struct ata_port_operations piix_pata_ops = {
> +	.inherits		= &piix_base_ops,
>  	.cable_detect		= ata_cable_40wire,
>  	.set_piomode		= piix_set_piomode,
>  	.set_dmamode		= piix_set_dmamode,
> @@ -329,7 +335,7 @@ static struct ata_port_operations ich_pa
>  };
>  
>  static struct ata_port_operations piix_sata_ops = {
> -	.inherits		= &ata_bmdma32_port_ops,
> +	.inherits		= &piix_base_ops,
>  };
>   

   Not sure it was worth wasting memory on having 2 identical copies of 
the struct ata_port_operations...

>  
>  static struct ata_port_operations piix_sidpr_sata_ops = {
> @@ -962,6 +968,14 @@ static int piix_sidpr_scr_write(struct a
>  	return 0;
>  }
>  
> +static bool piix_irq_check(struct ata_port *ap)
> +{
> +	if (unlikely(!ap->ioaddr.bmdma_addr))
> +		return false;
> +
> +	return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;
> +}
> +
>   

   I'm not at all sure that old, pre-ICH controllers set this bit also 
for the PIO mode commands, not only for DMA. And if you didn't make such 
assumption, I don't see why this can't be generic and placed into 
libata-sff.c instead...

WBR, Sergei

Re: [PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[Jeff Garzik]

On 01/14/2010 02:50 AM, Tejun Heo wrote:
> host->ports[i] is never NULL if i<  host->n_ports and non-NULL return
> from ata_qc_from_tag() guarantees that the returned qc is active.
> Drop unnecessary tests.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> ---
>   drivers/ata/libata-sff.c |   17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)

This is more #upstream material, don't you think?  We are pretty deep 
into -rc at this point.

(headed out for the weekend, fyi)

	Jeff

Re: [PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[Tejun Heo]

On 01/15/2010 02:38 AM, Jeff Garzik wrote:
> On 01/14/2010 02:50 AM, Tejun Heo wrote:
>> host->ports[i] is never NULL if i<  host->n_ports and non-NULL return
>> from ata_qc_from_tag() guarantees that the returned qc is active.
>> Drop unnecessary tests.
>>
>> Signed-off-by: Tejun Heo<tj@kernel.org>
>> ---
>>   drivers/ata/libata-sff.c |   17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> This is more #upstream material, don't you think?  We are pretty deep
> into -rc at this point.

Yeap, agreed.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 1/2] libata: cleanup ata_sff_interrupt()

[Tejun Heo]

On 01/14/2010 07:20 PM, Sergei Shtylyov wrote:
>> +        qc = ata_qc_from_tag(ap, ap->link.active_tag);
>> +        if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
>>   
> 
>   () not needed around !x.

Eh... I don't know.  I personally prefer using minimum number of
parantheses but quite a few people dislike not using them when && & ||
| are mixed.  In the end, I don't think it really matters.  It's like
debating about how far the second part of a broken line should be
indented.  Many people strong opinions but in the end it doesn't
really matter.  Anyhow, if it bothers you enough to write an email
about it, I'll be happy to oblige.  :-)

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement spurious irq handling for SFF and apply it to piix

[Tejun Heo]

Hello,

On 01/14/2010 09:42 PM, Sergei Shtylyov wrote:
>> +static bool piix_irq_check(struct ata_port *ap)
>> +{
>> +    if (unlikely(!ap->ioaddr.bmdma_addr))
>> +        return false;
>> +
>> +    return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;
>> +}
>> +
>>   
> 
>   I'm not at all sure that old, pre-ICH controllers set this bit also
> for the PIO mode commands, not only for DMA. And if you didn't make such
> assumption, I don't see why this can't be generic and placed into
> libata-sff.c instead...

Because different controllers have different mechanisms for detecting
pending IRQ?  Also, I think the current round is a bit dangerous in
that it might end up accessing registers while the driver is doing
polling PIO.  Having spurious IRQ clearing support would definitely be
helpful for such conditions but it might also lead to silent data
corruption which is worse.  I'll update the patch series so that the
mechanism only kicks in only when the controller is believed to be
completely idle.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes 2/2] libata: implement spurious irq handling for SFF and apply it to piix

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

>>> +static bool piix_irq_check(struct ata_port *ap)
>>> +{
>>> +    if (unlikely(!ap->ioaddr.bmdma_addr))
>>> +        return false;
>>> +
>>> +    return ap->ops->bmdma_status(ap) & ATA_DMA_INTR;
>>> +}
>>> +
>>>   
>>>       
>>   I'm not at all sure that old, pre-ICH controllers set this bit also
>> for the PIO mode commands, not only for DMA. And if you didn't make such
>> assumption, I don't see why this can't be generic and placed into
>> libata-sff.c instead...
>>     
>
> Because different controllers have different mechanisms for detecting
> pending IRQ?

   All SFF-8038i (BMIDE) controllers have the same mechanism. They may 
have some additional interrupt bits though, reflecting the interrupt 
status in both PIO and DMA mode though.

WBR, Sergei

Re: [PATCH #upstream-fixes 2/2] libata: implement spurious irq handling for SFF and apply it to piix

[Tejun Heo]

On 01/15/2010 07:23 PM, Sergei Shtylyov wrote:
>> Because different controllers have different mechanisms for detecting
>> pending IRQ?
> 
>   All SFF-8038i (BMIDE) controllers have the same mechanism. They may
> have some additional interrupt bits though, reflecting the interrupt
> status in both PIO and DMA mode though.

Oh, yeah, I was thinking about modern piixs where the bit works as a
true IRQ pending bit regardless of command state (it works while even
idle).  I don't think the original BMIDE IRQ pending bit would be too
useful for spurious IRQ detection.  It's interlocked with DMA transfer
protocol and unless the controller is horribly broken it won't be too
useful for spurious IRQ detection.

Thanks.

-- 
tejun