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

[Jeff Garzik <jgarzik@pobox.com>]

On 02/03/2010 12:20 PM, Andrew Morton wrote:
> On Wed, 03 Feb 2010 12:15:46 -0500 Jeff Garzik<jgarzik@pobox.com>  wrote:
>
>> On 02/03/2010 12:06 PM, Andrew Morton wrote:
>>> On Wed, 03 Feb 2010 12:00:58 -0500 Jeff Garzik<jgarzik@pobox.com>   wrote:
>>>
>>>> On 02/03/2010 11:40 AM, James Bottomley wrote:
>>>>> The fix to libata looks to be just that it should kmap all the time
>>>>> rather than trying to fiddle with the page to see if its higmem.  For
>>>>> kmap on a normal page, we should just return the offset map address and
>>>>> do all the flushing.
>>>>
>>>> libata tests PageHighMem() because it was measurably faster to do things
>>>> the current way (which includes local_irq_save/restore, only for
>>>> highmem) on boxes where it actually matters.
>>>>
>>>> It seems more efficient to add a flush where necessary, than
>>>> unconditionally punish everyone...
>>>
>>> kmap_atomic() tests PageHighMem() too - it's pretty lightweight for
>>> lowmem pages.
>>
>> Note the lack of local_irq_save/restore in our code, though...  These
>> PIO xfers are __slow__, from the perspective of a CPU manufactured in
>> the past decade; you are definitely disabling local interrupts for a
>> long time.  I suppose we could do
>>
>> 	if (high mem)
>> 		local irq save
>> 		kmap
>> 		xfer
>> 		kunmap
>> 		local irq restore
>> 	else
>> 		kmap
>> 		xfer
>> 		kunmap
>>
>> does that solve the problem for ARM, for 2.6.33?
>>
>
> It's unclear (to me) why that code is using KM_IRQ0 at all.  Can't it
> use a non-irq kmap slot?

libata may have to transfer data in response to an interrupt.  That is 
normal interrupt-driven PIO -- although it should be noted that the code 
supports polled PIO as well.


>>> Anyway, I'd draw your attention to this claim in the changelog: "This
>>> patch allows the ARM boards to use a rootfs on CompactFlash with the
>>> PATA platform driver." Immediate-term, we should be looking for a small
>>> fix for this issue which is acceptable for 2.6.33 and 2.6.32 and earlier.
>>
>> Sure...  see above.  hopefully one that does not punish -everybody-
>> though.  It would be sad to unconditionally slow down millions of volume
>> platform (read: x86) users for some embedded boards.
>
> Well.
> ata-call-flush_dcache_page-around-pio-data-transfers-in-libata-affc.patch
> is a no-op on x86.  It only has an effect on architectures which
> implement flush_dcache_page().  And I expect flush_dcache_page() is
> pretty light on those architectures, when compared with a PIO-mode
> transfer.

I don't object to the patch...  as long as the arch people are happy. 
Arch people seem to be the ones complaining, though.

	Jeff