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


-
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/