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@xxxxxxxxx>
> > 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/