consistent_sync on ioremap'ed area
A message that is not in the archive (sent off-list or not yet received): 20070519185638.GA7553@zarina
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Anton Vorontsov
Hi all,
During 2.6.21 development
BUG_ON()was added toconsistent_syncfunction. Thus, I'm curious if it is correct.
Quoted 8 lines
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...
Quoted 7 lines
Quoted 2 lines
drivers/mtd/maps/mainstone-flash.cdrivers/mtd/maps/lubbock-flash.cWhat 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?
Quoted 17 lines
Quoted 2 lines
So, is
BUG_ON()insideconsistent_syncreally 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.
Quoted 7 lines
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..
Quoted 3 lines
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,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.org/bd2
Quoted 21 lines · Anton Vorontsov
Quoted 6 lines · Russell King - ARM Linux
Quoted 4 lines · Anton Vorontsov
Hi all,
During 2.6.21 development
BUG_ON()was added toconsistent_syncfunction. Thus, I'm curious if it is correct.Quoted 8 lines
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...
Quoted 3 lines
Quoted 2 lines
drivers/mtd/maps/mainstone-flash.cdrivers/mtd/maps/lubbock-flash.c
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 21 lines
Quoted 21 lines
Quoted 2 lines
So, is
BUG_ON()insideconsistent_syncreally 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.
Quoted 8 lines
Quoted 3 lines
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?
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line · Russell King - ARM Linux
On Sun, May 20, 2007 at 05:56:22AM +0400, Anton Vorontsov wrote:
[...]
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 11 lines
Quoted 7 lines
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.
[...]
Quoted 19 lines
Quoted 2 lines
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.
Quoted 6 lines
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?
Quoted 11 lines
Quoted 6 lines
Quoted 3 lines
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,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.org/bd2
Quoted 2 lines · Russell King - ARM Linux
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.
Signature · 1 line
dwmw2