Re: consistent_sync on ioremap'ed area

Re: 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@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: consistent_sync on ioremap'ed area

[Russell King - ARM Linux]

On Sun, May 20, 2007 at 05:56:22AM +0400, Anton Vorontsov wrote:
> 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.

At the time when the use appeared, I said it was incorrect.  I got ignored.

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

Maybe, but it needs a new home rather than being in consistent.c.
However, the problem with band-aids is that they tend to become
permanent, so a little bit of thought now helps a lot.

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

I'm not talking about re-designing the entire cache flushing API.  I'm
talking about supplementing it with one additional function that takes
some appropriate arguments to deal with the problem.

The problem that needs solving for MTD is:

  we have two mappings of the same physical address space.  One mapping
  is cached.  The other is uncached.  Due to writes to the uncached
  mapping, cached data may become stale without the processor realising
  it.

So, to solve the problem we need to invalidate the contents of the cache
in some ioremapped region or sub-region.  We know what operation needs to
be performed, so passing arguments like DMA direction arguments are
utterly senseless.

However, we do need a way to describe the region.  Given the possibility
of a virtually mapped L1 cache and a physically mapped L2 cache, both of
which need dealing with, maybe the best approach would be to pass the
return value from ioremap, the cookie given to ioremap, offset, and size?

What about a name?  flush_ioremap_region() maybe?

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

I don't think it's that far off.

PS, it's rude to cross-post between member post only lists and open lists.
It's in the lists.arm.linux.org.uk etiquette.

Re: consistent_sync on ioremap'ed area

[Anton Vorontsov]

On Sun, May 20, 2007 at 09:24:27AM +0100, Russell King - ARM Linux wrote:
> On Sun, May 20, 2007 at 05:56:22AM +0400, Anton Vorontsov wrote:
[...]
> > > 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.
> 
> At the time when the use appeared, I said it was incorrect.  I got ignored.

I see.

> > 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?
> 
> Maybe, but it needs a new home rather than being in consistent.c.
> However, the problem with band-aids is that they tend to become
> permanent, so a little bit of thought now helps a lot.

Yes, that happens. But on the other side band-aiding sometimes helps,
i.e. later, looking at lots of band-aids you're seeing whole picture -
all demands. And then could refactor code to satisfy these demands in
generic manner. Though, I'm not advocating this particular case.

[...]
> > Well.. speaking of me, it will take months to get into all mm stuff,
> > and design new mm thing in correct/generic way..
> 
> I'm not talking about re-designing the entire cache flushing API.  I'm
> talking about supplementing it with one additional function that takes
> some appropriate arguments to deal with the problem.
> 
> The problem that needs solving for MTD is:
> 
>   we have two mappings of the same physical address space.  One mapping
>   is cached.  The other is uncached.  Due to writes to the uncached
>   mapping, cached data may become stale without the processor realising
>   it.
> 
> So, to solve the problem we need to invalidate the contents of the cache
> in some ioremapped region or sub-region.  We know what operation needs to
> be performed, so passing arguments like DMA direction arguments are
> utterly senseless.
> 
> However, we do need a way to describe the region.  Given the possibility
> of a virtually mapped L1 cache and a physically mapped L2 cache, both of
> which need dealing with, maybe the best approach would be to pass the
> return value from ioremap, the cookie given to ioremap, offset, and size?
> 
> What about a name?  flush_ioremap_region() maybe?

Um.. if we'll look how exactly MTD drivers tried to use
consistent_sync(), we'll see that they were doing dmac_inv_range()
through consistent_sync(), i.e. they tried to invalidate/discard
cache, to eliminate possible write backs (?). So, inval_ioremap_region()
seems better name for this purpose?

> > > 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.
> 
> I don't think it's that far off.
> 
> PS, it's rude to cross-post between member post only lists and open lists.
> It's in the lists.arm.linux.org.uk etiquette.

Sorry.

I've actually read etiquette docs at arm.linux.org.uk, but of course
I forgot something (not surprisingly, LAKML is single members-only list
I'm taking a part in, and on open lists it's normal practice adding other
related lists to Cc).

Well, as you didn't altered To and Cc, I still keeping them that way,
it's better keep people informed than discuss things behind their back
(though I do understand that it brings mess to LAKML when non-subscribers
answers).

Thanks,

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

Re: consistent_sync on ioremap'ed area

[David Woodhouse]

On Sun, 2007-05-20 at 09:24 +0100, Russell King - ARM Linux wrote:
> PS, it's rude to cross-post between member post only lists and open
> lists. It's in the lists.arm.linux.org.uk etiquette. 

Lists to which only the members can post are in themselves rude. There's
little place for them in Linux development.

-- 
dwmw2