linux-kernel - Re: [PATCH 2.6.19 2/3] sata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4572CEE7.502@gmail.com>
Date:	Sun, 03 Dec 2006 22:19:35 +0900
From:	Tejun Heo <htejun@...il.com>
To:	Jeff Garzik <jeff@...zik.org>
CC:	Mikael Pettersson <mikpe@...uu.se>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

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