Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Hello, Mikael.
>>
>> Thanks for doing this.
>>
>> Mikael Pettersson wrote:
>> [--snip--]
>>> +static void pdc_freeze(struct ata_port *ap)
>>> +{
>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>> +    u32 tmp;
>>> +
>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>> +    tmp |= PDC_IRQ_DISABLE;
>>> +    tmp &= ~PDC_DMA_ENABLE;
>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil
>>> does this */
>>
>> Just drop the above line.
>>
>>> +}
>>> +
>>> +static void pdc_thaw(struct ata_port *ap)
>>> +{
>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>> +    u32 tmp;
>>> +
>>> +    /* clear IRQ */
>>> +    readl(mmio + PDC_INT_SEQMASK);
>>> +
>>> +    /* turn IRQ back on */
>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>> +    tmp &= ~PDC_IRQ_DISABLE;
>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */
>>
>> Ditto.
> 
> Why do you think these flushes are not needed?

1. for thaw, it doesn't matter.  it's always followed by further IO
operations.

2. for freeze, interrupt delivery is asynchronous to IO anyway and
freeze is also called from outside of interrupt handler.  IRQ handler
must be ready to handle spurious interrupts on a frozen port (Note they
automatically are because they can't access aborted qc's).  As long as
it gets quiesced after finite number of interrupts, it's okay.

3. we don't have them in ahci nor sata_sil24.

But, having those flushes won't hurt either.  What was the conclusion of
mmio <-> spinlock sync discussion?  I always feel kind of uncomfortable
about readl() flushes.  I think they're too subtle.

-- 
tejun