On Tue, 2010-02-02 at 14:11 -0800, akpm@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Catalin Marinas <catalin.marinas@xxxxxxx>
>
> Depending on the direction of the transfer, flush_dcache_page() must be
> called either before (ATA_TFLAG_WRITE) or after (!ATA_TFLAG_WRITE) the
> data copying to avoid D-cache aliasing with user space or I-D cache
> coherency issues (when reading data from an ATA device using PIO, the
> kernel dirties the D-cache but there is no flush_dcache_page() required on
> Harvard architectures).
>
> This patch allows the ARM boards to use a rootfs on CompactFlash with the
> PATA platform driver.
>
> As Anfei Zhou mentioned in a recent patch ("flush dcache before writing
> into page to avoid alias"), on some architectures there may be a
> performance benefit in differentiating the flush_dcache_page() calls based
> on whether the kernel or the user page needs flushing.
>
> IMHO, we should differentiate based on the direction (kernel reading or
> writing from/to such page). In the ARM case with PIPT Harvard caches
> (newer processors), the kernel reading from a page that may be mapped in
> user space shouldn't need cache flushing. The kernel writing to such page
> would require D-cache flushing because of coherency with the I-cache.
> Currently on ARM, the latter happens in both cases.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Jeff Garzik <jgarzik@xxxxxxxxx>
> Cc: Tejun Heo <tj@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> drivers/ata/libata-sff.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff -puN drivers/ata/libata-sff.c~ata-call-flush_dcache_page-around-pio-data-transfers-in-libata-affc drivers/ata/libata-sff.c
> --- a/drivers/ata/libata-sff.c~ata-call-flush_dcache_page-around-pio-data-transfers-in-libata-affc
> +++ a/drivers/ata/libata-sff.c
> @@ -874,6 +874,9 @@ static void ata_pio_sector(struct ata_qu
>
> DPRINTK("data %s\n", qc->tf.flags & ATA_TFLAG_WRITE ? "write" : "read");
>
> + if (do_write)
> + flush_dcache_page(page);
> +
This looks wrong; the upper layers should already have made the page
aliases coherent from user to kernel by calling flush_dcache_page (in
__get_user_pages()), so the aliases should already be up to date and
this flush is spurious.
> if (PageHighMem(page)) {
> unsigned long flags;
>
> @@ -893,6 +896,9 @@ static void ata_pio_sector(struct ata_qu
> do_write);
> }
>
> + if (!do_write)
> + flush_dcache_page(page);
> +
OK, so this too looks wrong for two reasons
1. it's over flushing. Even after the write to the page by the
kernel PIO, the only alias that is dirty should be the kernel,
so this needs a flush_kernel_dcache_page() to empty the kernel
alias. It is possible user space will have speculated over the
user aliases, but there's stuff further up the block stack to
bring this back into coherence.
2. If the page really is in highmem, the flush has to happen along
the kernel alias, which you just lost because this flush is
happening after the kunmap_atomic(), so it has to occur
somewhere between the PIO operation and the kunmap.
> qc->curbytes += qc->sect_size;
> qc->cursg_ofs += qc->sect_size;
>
James
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
