Re: [PATCH] sata_sil: improved interrupt handling

[Jeff Garzik <jgarzik@pobox.com>]

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