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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2010-02-02 at 15:05 -0800, Andrew Morton wrote:
> On Tue, 02 Feb 2010 16:58:38 -0600
> James Bottomley <James.Bottomley@xxxxxxx> wrote:
> 
> > 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.
> 
> The upper layers don't know that the CPU touched the data!

The upper layers know to flush the user aliases.

>   If the
> driver did a DMA transfer then such a flush is unneeded, so we don't do
> it.

The point is that this is a write case (transfer from kernel to device)
so the underlying data should remain stable.  Once __get_user_pages()
has cohered the aliases, nothing should decohere them because nothing
should be altering the data in the page.

The danger case for VI caches is the read case because there you are
altering data via the kernel aliases.

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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux