[PATCH] sata_sil: improved interrupt handling

[PATCH] sata_sil: improved interrupt handling

[Jeff Garzik]

Just committed the following to the 'sii-irq' branch of libata-dev.git,
and verified it on an Adaptec 1210SA (3112).

Haven't decided whether I will push it upstream or not, but I think I
will.  It does a bit better job of handling handling errors, and should
be more efficient (less CPU usage) than the standard ATA interrupt
handler as well.

For users seeing sata_sil problems, this may make them happy.


commit b6abf7755a79383f0e5f108d23a0394f156c54c1
Author: Jeff Garzik <jgarzik@pobox.com>
Date:   Sat Dec 3 00:30:57 2005 -0500

    [libata sata_sil] improved interrupt handling

 drivers/scsi/sata_sil.c |  118 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 3609186..37398a5 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -85,6 +85,7 @@ static void sil_dev_config(struct ata_po
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 static void sil_post_set_mode (struct ata_port *ap);
+static irqreturn_t sil_irq (int irq, void *dev_instance, struct pt_regs *regs);
 
 
 static const struct pci_device_id sil_pci_tbl[] = {
@@ -167,7 +168,7 @@ static const struct ata_port_operations 
 	.qc_prep		= ata_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
 	.eng_timeout		= ata_eng_timeout,
-	.irq_handler		= ata_interrupt,
+	.irq_handler		= sil_irq,
 	.irq_clear		= ata_bmdma_irq_clear,
 	.scr_read		= sil_scr_read,
 	.scr_write		= sil_scr_write,
@@ -233,6 +234,121 @@ MODULE_DEVICE_TABLE(pci, sil_pci_tbl);
 MODULE_VERSION(DRV_VERSION);
 
 
+static inline void sil_port_irq(struct ata_port *ap, void __iomem *mmio,
+				u8 dma_stat, u8 dma_stat_mask)
+{
+	struct ata_queued_cmd *qc = NULL;
+	unsigned int err_mask = AC_ERR_OTHER;
+	int complete = 1;
+	u8 dev_stat;
+
+	/* Exit now, if port or port's irqs are disabled */
+	if (ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR)) {
+		complete = 0;
+		goto out;
+	}
+
+	/* Get active command */
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	if ((!qc) || (qc->tf.ctl & ATA_NIEN)) {
+		complete = 0;
+		goto out;
+	}
+
+	/* Stop DMA, if doing DMA */
+	switch (qc->tf.protocol) {
+	case ATA_PROT_DMA:
+	case ATA_PROT_ATAPI_DMA:
+		ata_bmdma_stop(qc);
+		break;
+
+	default:
+		/* do nothing */
+		break;
+	}
+
+	/* Catch PCI bus errors */
+	if (unlikely(dma_stat_mask & ATA_DMA_ERR)) {
+		struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
+		u16 pci_stat;
+
+		pci_read_config_word(pdev, PCI_STATUS, &pci_stat);
+		pci_write_config_word(pdev, PCI_STATUS, pci_stat);
+
+		err_mask = AC_ERR_HOST_BUS;
+
+		printk(KERN_ERR "ata%u: PCI error, pci %x, dma %x\n",
+			ap->id, pci_stat, dma_stat);
+		goto out;
+	}
+
+	/* Read device Status, clear device interrupt */
+	dev_stat = ata_check_status(ap);
+
+	/* Let timeout handler handle stuck BSY */
+	if (unlikely(dev_stat & ATA_BUSY)) {
+		complete = 0;
+		goto out;
+	}
+
+	/* Did S/G table specify a size smaller than the transfer size?  */
+	if (unlikely(dma_stat_mask == 0)) {
+		printk(KERN_ERR "ata%u: BUG: SG size underflow\n", ap->id);
+		err_mask = AC_ERR_OTHER; /* only occurs due to coder error? */
+		goto out;
+	}
+
+	/* Clear 311x DMA completion indicator */
+	writeb(ATA_DMA_INTR, mmio + ATA_DMA_STATUS);
+
+	/* Finally, complete the ATA command transaction */
+	ata_qc_complete(qc, ac_err_mask(dev_stat));
+	return;
+
+out:
+	ata_chk_status(ap);
+	writeb(dma_stat_mask, mmio + ATA_DMA_STATUS);
+	if (complete)
+		ata_qc_complete(qc, err_mask);
+}
+
+static irqreturn_t sil_irq (int irq, void *dev_instance, struct pt_regs *regs)
+{
+	struct ata_host_set *host_set = dev_instance;
+	unsigned int i, handled = 0;
+
+	spin_lock(&host_set->lock);
+
+	for (i = 0; i < host_set->n_ports; i++) {
+		struct ata_port *ap;
+		void __iomem *mmio;
+		u8 status, mask;
+		u32 serr;
+
+		ap = host_set->ports[i];
+		if (!ap)
+			continue;
+
+		mmio = (void __iomem *) ap->ioaddr.bmdma_addr;
+		status = readb(mmio + ATA_DMA_STATUS);
+		mask = status & (ATA_DMA_INTR | ATA_DMA_ERR | ATA_DMA_ACTIVE);
+		if (mask == ATA_DMA_ACTIVE)
+			continue;
+
+		handled = 1;
+
+		sil_port_irq(ap, mmio, status, mask);
+
+		serr = sil_scr_read(ap, SCR_ERROR);
+		if (serr)
+			sil_scr_write(ap, SCR_ERROR, serr);
+	}
+
+	spin_unlock(&host_set->lock);
+
+	return IRQ_RETVAL(handled);
+}
+
 static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
 {
 	u8 cache_line = 0;

Re: [PATCH] sata_sil: improved interrupt handling

[Tejun Heo]

Hi, Jeff.

Jeff Garzik wrote:
> Just committed the following to the 'sii-irq' branch of libata-dev.git,
> and verified it on an Adaptec 1210SA (3112).
> 
> Haven't decided whether I will push it upstream or not, but I think I
> will.  It does a bit better job of handling handling errors, and should
> be more efficient (less CPU usage) than the standard ATA interrupt
> handler as well.
> 
> For users seeing sata_sil problems, this may make them happy.
> 

This patch doesn't make any difference on my sil3114 controller.  I'll 
write about it in the m15w thread.

Also, this patch doesn't implement proper handling of PIO protocols and 
thus breaks ALL branch.

It seems to me that the changes made by the new interrupt handler is not 
very sil3112 specific.  Is there any reason this change is sil3112 specific?

> 
> commit b6abf7755a79383f0e5f108d23a0394f156c54c1
> Author: Jeff Garzik <jgarzik@pobox.com>
> Date:   Sat Dec 3 00:30:57 2005 -0500
> 
>     [libata sata_sil] improved interrupt handling
> 
>  drivers/scsi/sata_sil.c |  118 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
> index 3609186..37398a5 100644
> --- a/drivers/scsi/sata_sil.c
> +++ b/drivers/scsi/sata_sil.c
> @@ -85,6 +85,7 @@ static void sil_dev_config(struct ata_po
>  static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
>  static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
>  static void sil_post_set_mode (struct ata_port *ap);
> +static irqreturn_t sil_irq (int irq, void *dev_instance, struct pt_regs *regs);
>  
>  
>  static const struct pci_device_id sil_pci_tbl[] = {
> @@ -167,7 +168,7 @@ static const struct ata_port_operations 
>  	.qc_prep		= ata_qc_prep,
>  	.qc_issue		= ata_qc_issue_prot,
>  	.eng_timeout		= ata_eng_timeout,
> -	.irq_handler		= ata_interrupt,
> +	.irq_handler		= sil_irq,
>  	.irq_clear		= ata_bmdma_irq_clear,
>  	.scr_read		= sil_scr_read,
>  	.scr_write		= sil_scr_write,
> @@ -233,6 +234,121 @@ MODULE_DEVICE_TABLE(pci, sil_pci_tbl);
>  MODULE_VERSION(DRV_VERSION);
>  
>  
> +static inline void sil_port_irq(struct ata_port *ap, void __iomem *mmio,
> +				u8 dma_stat, u8 dma_stat_mask)
> +{
> +	struct ata_queued_cmd *qc = NULL;
> +	unsigned int err_mask = AC_ERR_OTHER;
> +	int complete = 1;
> +	u8 dev_stat;
> +
> +	/* Exit now, if port or port's irqs are disabled */
> +	if (ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR)) {
> +		complete = 0;
> +		goto out;
> +	}

Hmmm... By performing this test here, we end up reading bmdma status 
registers of all ports everytime an interrupt occurs.  Is this to 
prevent screaming IRQ problem?

> +
> +	/* Get active command */
> +	qc = ata_qc_from_tag(ap, ap->active_tag);
> +	if ((!qc) || (qc->tf.ctl & ATA_NIEN)) {
> +		complete = 0;
> +		goto out;
> +	}
> +
> +	/* Stop DMA, if doing DMA */
> +	switch (qc->tf.protocol) {
> +	case ATA_PROT_DMA:
> +	case ATA_PROT_ATAPI_DMA:
> +		ata_bmdma_stop(qc);
> +		break;
> +
> +	default:
> +		/* do nothing */
> +		break;
> +	}
> +
> +	/* Catch PCI bus errors */
> +	if (unlikely(dma_stat_mask & ATA_DMA_ERR)) {
> +		struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
> +		u16 pci_stat;
> +
> +		pci_read_config_word(pdev, PCI_STATUS, &pci_stat);
> +		pci_write_config_word(pdev, PCI_STATUS, pci_stat);
> +
> +		err_mask = AC_ERR_HOST_BUS;
> +
> +		printk(KERN_ERR "ata%u: PCI error, pci %x, dma %x\n",
> +			ap->id, pci_stat, dma_stat);
> +		goto out;
> +	}
> +
> +	/* Read device Status, clear device interrupt */
> +	dev_stat = ata_check_status(ap);
> +
> +	/* Let timeout handler handle stuck BSY */
> +	if (unlikely(dev_stat & ATA_BUSY)) {
> +		complete = 0;
> +		goto out;
> +	}
> +
> +	/* Did S/G table specify a size smaller than the transfer size?  */
> +	if (unlikely(dma_stat_mask == 0)) {
> +		printk(KERN_ERR "ata%u: BUG: SG size underflow\n", ap->id);
> +		err_mask = AC_ERR_OTHER; /* only occurs due to coder error? */
> +		goto out;
> +	}
> +
> +	/* Clear 311x DMA completion indicator */
> +	writeb(ATA_DMA_INTR, mmio + ATA_DMA_STATUS);
> +
> +	/* Finally, complete the ATA command transaction */
> +	ata_qc_complete(qc, ac_err_mask(dev_stat));
> +	return;
> +
> +out:
> +	ata_chk_status(ap);
> +	writeb(dma_stat_mask, mmio + ATA_DMA_STATUS);
> +	if (complete)
> +		ata_qc_complete(qc, err_mask);
> +}
> +
> +static irqreturn_t sil_irq (int irq, void *dev_instance, struct pt_regs *regs)
> +{
> +	struct ata_host_set *host_set = dev_instance;
> +	unsigned int i, handled = 0;
> +
> +	spin_lock(&host_set->lock);
> +
> +	for (i = 0; i < host_set->n_ports; i++) {
> +		struct ata_port *ap;
> +		void __iomem *mmio;
> +		u8 status, mask;
> +		u32 serr;
> +
> +		ap = host_set->ports[i];
> +		if (!ap)
> +			continue;
> +
> +		mmio = (void __iomem *) ap->ioaddr.bmdma_addr;
> +		status = readb(mmio + ATA_DMA_STATUS);
> +		mask = status & (ATA_DMA_INTR | ATA_DMA_ERR | ATA_DMA_ACTIVE);
> +		if (mask == ATA_DMA_ACTIVE)
> +			continue;
> +
> +		handled = 1;
> +
> +		sil_port_irq(ap, mmio, status, mask);
> +
> +		serr = sil_scr_read(ap, SCR_ERROR);
> +		if (serr)
> +			sil_scr_write(ap, SCR_ERROR, serr);
> +	}
> +
> +	spin_unlock(&host_set->lock);
> +
> +	return IRQ_RETVAL(handled);
> +}
> +
>  static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
>  {
>  	u8 cache_line = 0;
> 


-- 
tejun

Re: [PATCH] sata_sil: improved interrupt handling

[Jeff Garzik]

Tejun Heo wrote:
> Hi, Jeff.
> 
> Jeff Garzik wrote:
> 
>> Just committed the following to the 'sii-irq' branch of libata-dev.git,
>> and verified it on an Adaptec 1210SA (3112).
>>
>> Haven't decided whether I will push it upstream or not, but I think I
>> will.  It does a bit better job of handling handling errors, and should
>> be more efficient (less CPU usage) than the standard ATA interrupt
>> handler as well.
>>
>> For users seeing sata_sil problems, this may make them happy.
>>
> 
> This patch doesn't make any difference on my sil3114 controller.  I'll 
> write about it in the m15w thread.

"doesn't make any difference" I will interpret to mean there are no 
regressions.


> Also, this patch doesn't implement proper handling of PIO protocols and 
> thus breaks ALL branch.

PIO should work fine, modulo the obvious changes for ATA_FLAG_NOINTR 
disappearance.


> It seems to me that the changes made by the new interrupt handler is not 
> very sil3112 specific.  Is there any reason this change is sil3112 
> specific?

There is nothing 3112-specific about this new code.


>> commit b6abf7755a79383f0e5f108d23a0394f156c54c1
>> Author: Jeff Garzik <jgarzik@pobox.com>
>> Date:   Sat Dec 3 00:30:57 2005 -0500
>>
>>     [libata sata_sil] improved interrupt handling
>>
>>  drivers/scsi/sata_sil.c |  118 
>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 117 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
>> index 3609186..37398a5 100644
>> --- a/drivers/scsi/sata_sil.c
>> +++ b/drivers/scsi/sata_sil.c
>> @@ -85,6 +85,7 @@ static void sil_dev_config(struct ata_po
>>  static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
>>  static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, 
>> u32 val);
>>  static void sil_post_set_mode (struct ata_port *ap);
>> +static irqreturn_t sil_irq (int irq, void *dev_instance, struct 
>> pt_regs *regs);
>>  
>>  
>>  static const struct pci_device_id sil_pci_tbl[] = {
>> @@ -167,7 +168,7 @@ static const struct ata_port_operations      
>> .qc_prep        = ata_qc_prep,
>>      .qc_issue        = ata_qc_issue_prot,
>>      .eng_timeout        = ata_eng_timeout,
>> -    .irq_handler        = ata_interrupt,
>> +    .irq_handler        = sil_irq,
>>      .irq_clear        = ata_bmdma_irq_clear,
>>      .scr_read        = sil_scr_read,
>>      .scr_write        = sil_scr_write,
>> @@ -233,6 +234,121 @@ MODULE_DEVICE_TABLE(pci, sil_pci_tbl);
>>  MODULE_VERSION(DRV_VERSION);
>>  
>>  
>> +static inline void sil_port_irq(struct ata_port *ap, void __iomem *mmio,
>> +                u8 dma_stat, u8 dma_stat_mask)
>> +{
>> +    struct ata_queued_cmd *qc = NULL;
>> +    unsigned int err_mask = AC_ERR_OTHER;
>> +    int complete = 1;
>> +    u8 dev_stat;
>> +
>> +    /* Exit now, if port or port's irqs are disabled */
>> +    if (ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR)) {
>> +        complete = 0;
>> +        goto out;
>> +    }
> 
> 
> Hmmm... By performing this test here, we end up reading bmdma status 
> registers of all ports everytime an interrupt occurs.  Is this to 
> prevent screaming IRQ problem?

That's the preferred way to handle interrupts on this hardware.  Normal 
ATA is broken due to the lack of a way to ask "did I receive an 
interrupt?" without unduly affecting state.  311x, like AHCI, sata_sil24 
and other hardware, provides a method to easily determine if a PCI 
interrupt is owned by the hardware or not.

The code should eliminate all screaming interrupt problems, yes.

As an aside, 3114 should use a single 32-bit read of the Interrupt 
Summary register, rather than the four separate 8-bit reads that this 
code does.

	Jeff

Re: [PATCH] sata_sil: improved interrupt handling

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Hi, Jeff.
>>
>> Jeff Garzik wrote:
>>
>>> Just committed the following to the 'sii-irq' branch of libata-dev.git,
>>> and verified it on an Adaptec 1210SA (3112).
>>>
>>> Haven't decided whether I will push it upstream or not, but I think I
>>> will.  It does a bit better job of handling handling errors, and should
>>> be more efficient (less CPU usage) than the standard ATA interrupt
>>> handler as well.
>>>
>>> For users seeing sata_sil problems, this may make them happy.
>>>
>>
>> This patch doesn't make any difference on my sil3114 controller.  I'll 
>> write about it in the m15w thread.
> 
> 
> "doesn't make any difference" I will interpret to mean there are no 
> regressions.
> 

I wasn't clear enough.  I meant that the change did not fix the 
seemlingly-m15w problems on 3114.

> 
>> Also, this patch doesn't implement proper handling of PIO protocols 
>> and thus breaks ALL branch.
> 
> 
> PIO should work fine, modulo the obvious changes for ATA_FLAG_NOINTR 
> disappearance.
> 

I took out ATA_FLAG_NOINTR test from sata_sil in the latest ALL branch 
and tested it.  It fails to read IDENTIFY data.  Actually, there is no 
code to read PIO data.  It should be done in the interrupt handler but 
sil_port_irq doesn't do it.

scsi3 : sata_sil
ata4: SATA link up 1.5 Gbps (SStatus 113)
ata4: dev 0 cfg 49:0000 82:0000 83:0000 84:0000 85:0000 86:0000 87:0000 
88:0000
ata4: no dma
ata4: dev 0 not supported, ignoring
scsi4 : sata_sil
ata5: SATA link up 1.5 Gbps (SStatus 113)
ata5: dev 0 cfg 49:0000 82:0000 83:0000 84:0000 85:0000 86:0000 87:0000 
88:0000
ata5: no dma
ata5: dev 0 not supported, ignoring
scsi5 : sata_sil
ata6: SATA link up 1.5 Gbps (SStatus 113)
ata6: dev 0 cfg 49:0000 82:0000 83:0000 84:0000 85:0000 86:0000 87:0000 
88:0000
ata6: no dma
ata6: dev 0 not supported, ignoring
scsi6 : sata_sil

> 
>> It seems to me that the changes made by the new interrupt handler is 
>> not very sil3112 specific.  Is there any reason this change is sil3112 
>> specific?
> 
> 
> There is nothing 3112-specific about this new code.
> 
> 
>>> commit b6abf7755a79383f0e5f108d23a0394f156c54c1
>>> Author: Jeff Garzik <jgarzik@pobox.com>
>>> Date:   Sat Dec 3 00:30:57 2005 -0500
>>>
>>>     [libata sata_sil] improved interrupt handling
>>>
>>>  drivers/scsi/sata_sil.c |  118 
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 117 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
>>> index 3609186..37398a5 100644
>>> --- a/drivers/scsi/sata_sil.c
>>> +++ b/drivers/scsi/sata_sil.c
>>> @@ -85,6 +85,7 @@ static void sil_dev_config(struct ata_po
>>>  static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
>>>  static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, 
>>> u32 val);
>>>  static void sil_post_set_mode (struct ata_port *ap);
>>> +static irqreturn_t sil_irq (int irq, void *dev_instance, struct 
>>> pt_regs *regs);
>>>  
>>>  
>>>  static const struct pci_device_id sil_pci_tbl[] = {
>>> @@ -167,7 +168,7 @@ static const struct ata_port_operations      
>>> .qc_prep        = ata_qc_prep,
>>>      .qc_issue        = ata_qc_issue_prot,
>>>      .eng_timeout        = ata_eng_timeout,
>>> -    .irq_handler        = ata_interrupt,
>>> +    .irq_handler        = sil_irq,
>>>      .irq_clear        = ata_bmdma_irq_clear,
>>>      .scr_read        = sil_scr_read,
>>>      .scr_write        = sil_scr_write,
>>> @@ -233,6 +234,121 @@ MODULE_DEVICE_TABLE(pci, sil_pci_tbl);
>>>  MODULE_VERSION(DRV_VERSION);
>>>  
>>>  
>>> +static inline void sil_port_irq(struct ata_port *ap, void __iomem 
>>> *mmio,
>>> +                u8 dma_stat, u8 dma_stat_mask)
>>> +{
>>> +    struct ata_queued_cmd *qc = NULL;
>>> +    unsigned int err_mask = AC_ERR_OTHER;
>>> +    int complete = 1;
>>> +    u8 dev_stat;
>>> +
>>> +    /* Exit now, if port or port's irqs are disabled */
>>> +    if (ap->flags & (ATA_FLAG_PORT_DISABLED | ATA_FLAG_NOINTR)) {
>>> +        complete = 0;
>>> +        goto out;
>>> +    }
>>
>>
>>
>> Hmmm... By performing this test here, we end up reading bmdma status 
>> registers of all ports everytime an interrupt occurs.  Is this to 
>> prevent screaming IRQ problem?
> 
> 
> That's the preferred way to handle interrupts on this hardware.  Normal 
> ATA is broken due to the lack of a way to ask "did I receive an 
> interrupt?" without unduly affecting state.  311x, like AHCI, sata_sil24 
> and other hardware, provides a method to easily determine if a PCI 
> interrupt is owned by the hardware or not.
> 

Ah... I see.  Section 7.4 of sii3112 manual says that

Wait until an IDE channel interrupt (bit 11 in the IDEx Task File Timing 
+ Configuration + Status register is set).

Where the register is at BAR5 + 0xA0 and bit 11 is

• Bit [11]: Interrupt Status (R) – IDE0 Interrupt Status. This bit set 
indicates that an interrupt is pending on IDE0. This bit provides 
real-time status of the IDE0 interrupt pin.

And, section 6.7.1 (PCI bus master - IDE0 register) says

• Bit [18]: IDE0 DMA Comp (R/W1C) – IDE0 DMA Completion Interrupt. 
During write DMA operation, This bit set indicates that the IDE0 
interrupt has been asserted and all data has been written to system 
memory. During Read DMA, This bit set indicates that the IDE0 interrupt 
has been asserted. This bit must be W1C by software when set during DMA 
operation (bit 0 is set). During normal operation, this bit reflects 
IDE0 interrupt line.

So, the last sentence means that while on DMA command is in progress, 
bit 18 of the PCI bus master is identical to bit 11 of the conf/status 
register.  Right?

Yeap, I agree that this is a good change although it hurts a little bit 
that it causes a few extra PCI transactions.

Also, it seems a little bit weird that the code enters interrupt 
handling even for ports which don't have ATA_DMA_INTR set, although I 
don't think the current code would bogusly finish commands due to the 
ATA_BUSY check.  Is this intentional?

> The code should eliminate all screaming interrupt problems, yes.
> 
> As an aside, 3114 should use a single 32-bit read of the Interrupt 
> Summary register, rather than the four separate 8-bit reads that this 
> code does.

That sounds cool.

-- 
tejun

Re: [PATCH] sata_sil: improved interrupt handling

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>> Hi, Jeff.
>>>
>>> Jeff Garzik wrote:
>>>
>>>> Just committed the following to the 'sii-irq' branch of libata-dev.git,
>>>> and verified it on an Adaptec 1210SA (3112).
>>>>
>>>> Haven't decided whether I will push it upstream or not, but I think I
>>>> will.  It does a bit better job of handling handling errors, and should
>>>> be more efficient (less CPU usage) than the standard ATA interrupt
>>>> handler as well.
>>>>
>>>> For users seeing sata_sil problems, this may make them happy.
>>>>
>>>
>>> This patch doesn't make any difference on my sil3114 controller.  
>>> I'll write about it in the m15w thread.
>>
>>
>>
>> "doesn't make any difference" I will interpret to mean there are no 
>> regressions.
>>
> 
> I wasn't clear enough.  I meant that the change did not fix the 
> seemlingly-m15w problems on 3114.

That's expected.  3114 should work (or not) as before.


>>> Also, this patch doesn't implement proper handling of PIO protocols 
>>> and thus breaks ALL branch.
>>
>>
>>
>> PIO should work fine, modulo the obvious changes for ATA_FLAG_NOINTR 
>> disappearance.
>>
> 
> I took out ATA_FLAG_NOINTR test from sata_sil in the latest ALL branch 
> and tested it.  It fails to read IDENTIFY data.  Actually, there is no 
> code to read PIO data.  It should be done in the interrupt handler but 
> sil_port_irq doesn't do it.

Correct.  sata_sil only works against upstream 2.6.x, not 
libata-dev.git#ALL.

The next step is to support PIO-via-DMA, but supporting the updates in 
the irq-pio branch may also be a good next-step.


>> That's the preferred way to handle interrupts on this hardware.  
>> Normal ATA is broken due to the lack of a way to ask "did I receive an 
>> interrupt?" without unduly affecting state.  311x, like AHCI, 
>> sata_sil24 and other hardware, provides a method to easily determine 
>> if a PCI interrupt is owned by the hardware or not.
>>
> 
> Ah... I see.  Section 7.4 of sii3112 manual says that
> 
> Wait until an IDE channel interrupt (bit 11 in the IDEx Task File Timing 
> + Configuration + Status register is set).
> 
> Where the register is at BAR5 + 0xA0 and bit 11 is
> 
> • Bit [11]: Interrupt Status (R) – IDE0 Interrupt Status. This bit set 
> indicates that an interrupt is pending on IDE0. This bit provides 
> real-time status of the IDE0 interrupt pin.
> 
> And, section 6.7.1 (PCI bus master - IDE0 register) says
> 
> • Bit [18]: IDE0 DMA Comp (R/W1C) – IDE0 DMA Completion Interrupt. 
> During write DMA operation, This bit set indicates that the IDE0 
> interrupt has been asserted and all data has been written to system 
> memory. During Read DMA, This bit set indicates that the IDE0 interrupt 
> has been asserted. This bit must be W1C by software when set during DMA 
> operation (bit 0 is set). During normal operation, this bit reflects 
> IDE0 interrupt line.
> 
> So, the last sentence means that while on DMA command is in progress, 
> bit 18 of the PCI bus master is identical to bit 11 of the conf/status 
> register.  Right?

Not quite.  When the operation is DMA, the 8-bit bmdma status reflects 
the DMA operation.  When the operation is not DMA, the status reflects 
the IDE interrupt line.


> Yeap, I agree that this is a good change although it hurts a little bit 
> that it causes a few extra PCI transactions.

What, for the non-existent ports?

Further code should be added to disable the interrupts for the disabled 
ports, then we can skip the check.


> Also, it seems a little bit weird that the code enters interrupt 
> handling even for ports which don't have ATA_DMA_INTR set, although I 
> don't think the current code would bogusly finish commands due to the 
> ATA_BUSY check.  Is this intentional?

dma_stat_mask==0 check needs to be added, for non-DMA commands.  That's 
about it.

	Jeff