consistent_sync on ioremap'ed area

[Anton Vorontsov]

On Sat, May 19, 2007 at 11:32:49PM +0100, Russell King - ARM Linux wrote:
> On Sat, May 19, 2007 at 10:56:38PM +0400, Anton Vorontsov wrote:
> > Hi all,
> > 
> > During 2.6.21 development BUG_ON() was added to consistent_sync
> > function. Thus, I'm curious if it is correct.
> 
> consistent_sync() exists to provide the DMA mapping code with the
> required functionality to ensure cache coherency in the system.
> That is it's sole purpose.  That's why it's in a file to do with
> DMA mapping and why it takes DMA-related arguments.
> 
> As the requirements for the DMA mapping code change with enhancements
> in the ARM architecture, this function will continue to evolve to
> ensure that it continues to forfill its purpose.

I see...

> > drivers/mtd/maps/mainstone-flash.c
> > drivers/mtd/maps/lubbock-flash.c
> 
> What this means is that improper usage will break at some point.
> The two MTD map drivers that you have identified are the only two
> abusers of this function.  They make assumptions about the behaviour
> of this function which are no longer valid.

The key words are "no longer". In time that drivers were written (and
they were accepted) using that function was okay, I guess. I've run git
blame on consistent.c, and seeing that comment regarding "no no, don't
call it directly" appeard relatively recently (2006-11-21).

I'm curious when exactly behaviour of this function became invalid in
MTD usage context. Maybe we can apply some band-aid: copy older, still
valid function under different name, with huge "TODO:" regarding
generic API need?

> > So, is BUG_ON() inside consistent_sync really correct? If it is, then
> > what is wrong?
> 
> Yes.  Whenever this function is called on behalf of the DMA code,
> the requirements will be met - and these requirements are brought
> on by the L2 cache support.
> 
> These two drivers need fixing, by inventing a new cache coherency
> interface for the purpose that they're trying to implement - cache
> flushing due to the backing data changing in an ioremapped cachable
> region.
> 
> Carefully consider two points:
> 
> 1. if a PXA device became DMA coherent, consistent_sync() would become
>    a no-op.  The above drivers break.
> 
> 2. what would be the implication if we were to continue to allow these
>    MTD drivers to use this, and then a PXA platform came along which
>    had L2 cache _and_ re-used the lubbock or mainstone flash driver.
> 
> It would be nice if these two map drivers could be fixed.  As I've
> said on this list recently (and above), the right way forward is to
> invent a new purpose-defined API to satisfy the MTD drivers.

(1)

Well.. speaking of me, it will take months to get into all mm stuff,
and design new mm thing in correct/generic way..

> Since my PXA hardware (lubbock) is quite a bit of hastle to get going
> (I estimate at least a day's worth of fiddling around) it's not
> something I'm able to look into.  Patches welcome.

..and because of (1), you hardly can expect this patch from me
in near future.

Thus, it's better to ask help, loudly. Help! Anyone?
(Nicolas Pitre, linux-mtd and kernel-discuss added to Cc)

Thanks,

-- 
Anton Vorontsov
email: cbou at mail.ru
backup email: ya-cbou at yandex.ru
irc://irc.freenode.org/bd2