On Fri, 2006-08-04 at 09:47 +0200, Haavard Skinnemoen wrote: > On Fri, 14 Jul 2006 08:48:43 -0700 > Dave Hansen <[EMAIL PROTECTED]> wrote: > > It would also be nice to see a _couple_ of patches that perhaps > > abstract a thing or two into generic code. I know that new > > architectures usually begin with a 'cp -r', but it would be nice to > > see a wee bit of code refactoring as a small price of admission. > > Some of the ioremap and other pagetable code looked pretty generic to > > me. > > Ok, here's a first try. > > This patch moves the i386 implementation of ioremap_page_range() into > lib/ioremap.c for use by other architectures.
Wow. Very nice. > There's one difference between the generic ioremap_page_range and the > i386 version: it takes a pgprot_t argument instead of unsigned long > flags, meaning that the arch-specific ioremap() implementation must > set all pte flags before calling ioremap_page_range() instead of > in the lowest-level page remapping function. It looks like they were pretty static before, anyway. I guess, in the worst case, you could make a weak symbol in lib/ioremap.c that does arch_ioremap_pgprot(). If an architecture needs to do something special, they could override it. But, unless this is causing real problems, I don't see any serious reason to do it. It can wail until we actually run into a user that needs it. > If you think this looks like a good idea, I'll split out the i386 > modifications in a separate patch and submit patches for several other > architectures as well. > > To get the review started, here are a couple of questions: > * Wouldn't it make more sense to call flush_cache_vmap() instead of > flush_cache_all()? Yup, probably. The ioremap code probably predates the existence of flush_cache_vmap(). > * Why do we need to call flush_tlb_all()? I thought you only needed > to do that when changing/removing existing mappings... I must not be understanding the flush_cache*() functions correctly because the vmalloc() code does its flush_cache_vmap() _after_ the vmalloc area is set up. In any case, vmalloc() apparently does something very close to what you need, and it does what you suggest: use flush_cache_vmap(), intends to only work on pte_none() ptes, and doesn't call a tlb flush function afterwords. Unless there is something to differentiate ioremap's functionality (say, the random pte flags you can set with ioremap) I can't think of why ioremap is different. BTW, does this new generic ioremap code work on _your_ architecture? ;) Have you done a quick survey to see how many other architectures can use it? -- Dave - To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
