Re: [patch for 2.6.33? 1/1] ata: call flush_dcache_page() around PIO data transfers in libata-aff.c

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 02 Feb 2010 16:58:38 -0600
James Bottomley <James.Bottomley@suse.de> wrote:

> On Tue, 2010-02-02 at 14:11 -0800, akpm@linux-foundation.org wrote:
> > From: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > 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@arm.com>
> > Cc: Jeff Garzik <jgarzik@pobox.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: <stable@kernel.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  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.

The upper layers don't know that the CPU touched the data!  If the
driver did a DMA transfer then such a flush is unneeded, so we don't do
it.

> >  	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
>