Re: [PATCH 3/6] sata_sil: new interrupt handler

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Jeff Garzik wrote:
>>>> +    /* ack bmdma irq events */
>>>> +    ap->ops->irq_clear(ap);
>>>
>>> Don't use hook in LLDD
>>
>> Okay.
>>
>>>> +    spin_lock_irqsave(&host_set->lock, flags);
>>>> +
>>>> +    for (i = 0; i < host_set->n_ports; i++) {
>>>> +        struct ata_port *ap = host_set->ports[i];
>>>> +        u32 bmdma2 = readl(mmio_base + sil_port[ap->port_no].bmdma2);
>>>> +
>>>> +        if (unlikely(!ap || ap->flags & ATA_FLAG_DISABLED))
>>>> +            continue;
>>>> +
>>>> +        if (!(bmdma2 & SIL_DMA_COMPLETE))
>>>> +            continue;
>>>> +
>>>> +        sil_host_intr(ap, bmdma2);
>>>> +        handled = 1;
>>>> +    }
>>>> +
>>>> +    spin_unlock_irqrestore(&host_set->lock, flags);
>>>
>>> NAK, unconditionally use spin_lock() and spin_unlock() on this hardware.
>>
>> Can you explain more on this?  We almost never use those in libata and 
>> I don't see what's different for sata_sil.
> 
> er, huh?  _Most_ libata drivers use spin_lock().
> 
> The only case where spin_lock_irqsave() should be used is when you are 
> dealing with multiple independent interrupts, i.e. the legacy case of 
> irqs 14/15.
> 
> Thus, the core uses the safe-in-all-cases version, and drivers that 
> implement their own interrupt handler can implement the better version.
> 
> [jgarzik@pretzel linux-2.6]$ grep -wl spin_lock drivers/scsi/ahci.c 
> drivers/scsi/pdc_adma.c drivers/scsi/sata_*.c
> drivers/scsi/ahci.c
> drivers/scsi/pdc_adma.c
> drivers/scsi/sata_mv.c
> drivers/scsi/sata_promise.c
> drivers/scsi/sata_qstor.c
> drivers/scsi/sata_sil24.c
> drivers/scsi/sata_sx4.c
> drivers/scsi/sata_vsc.c
> 

Ouch, you're right.  Sorry about the noise.

-- 
tejun