On Mon, Nov 05, 2007 at 09:45:27AM +0100, Roel Kluin wrote: > Simon Horman wrote: > > On Sun, Nov 04, 2007 at 12:30:14AM +0100, Roel Kluin wrote: > >> I am less certain about this one, so please review > >> -- > >> Iounmap when EFI won't switch to virtual mode > >> > > > > > Hi Roel, > > > > I'm not really sure what the intention of this patch is. > > But I'm pretty sure its not doing what you want it to do. > > > > I guess that you wish to reverse all the calls to ioremap() > > that are made in efi_enter_virtual_mode(). ioremap() is > > called during iterating through efi_map_start, but you only > > seem to call iounmap on whatever md happens to be set > > to at the end of the iteration. Surely you need to run through > > efi_map_start again? > > Yes, you are right, this was what I had in mind: > -- > if (status != EFI_SUCCESS) { > + for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) { > + md = p; > + if ((md->attribute & EFI_MEMORY_RUNTIME) && > + (md->attribute & EFI_MEMORY_UC) || > + (md->attribute & EFI_MEMORY_WC)) I think that you also need || (md->attribute & EFI_MEMORY_WT) or in other words if ((md->attribute & EFI_MEMORY_RUNTIME) && (md->attribute & EFI_MEMORY_UC|EFI_MEMORY_WC|EFI_MEMORY_WT)) > + iounmap(md->virt_addr); > + } > printk(KERN_WARNING "warning: unable to switch EFI into virtual mode " > "(status=%lu)\n", status); > return; Otherwise, yes, that is what I was thinking. > --- > > > The next thing that I wonder about is weather calling iounmap() > > actually reverses ioremap() in this case. Though now that I look > > at it and see that basically iounmap() will do nothing in > > this case, which seems appropriate as in this case ioremap() > > ends up being: > > > > return (void __iomem *) (__IA64_UNCACHED_OFFSET | phys_addr) > > > > Its probably not to much of a bother that all the md->virt_addr have > > been mangled and stay mangled. Though if we are concerned about such > > things, perhaps it would be cleaner to zero them on error? > > > > Like this? Yes, though I guess that iounmap() should also be called. And as I mentioned before, I'm not sure if doing this is worthwhile or not. On a fresh boot its likely that md->virt_addr is initialised to 0 for each md (I ought to check the spec). But, for instance, in a second kernel botoed by kexec, virt_addr may well be non-zero, indicating the values that were set in the previous boot. Incidently efi_call_phys() really ought to fail on in that case, though it doesn't as kexec replaces it with a noop. So I'm not sure if reseting the value to 0 has any use. Though it does feel clean to me. > > --- > if (status != EFI_SUCCESS) { > + for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) { > + md = p; > + if ((md->attribute & EFI_MEMORY_RUNTIME) && > + (md->attribute & EFI_MEMORY_UC) || > + (md->attribute & EFI_MEMORY_WC)) > + md->virt_addr = 0; > + } > printk(KERN_WARNING "warning: unable to switch EFI into virtual mode " > "(status=%lu)\n", status); > return; > --- > > I fear the remainder of your review is beyond my scope. My field of > expertise is biomedical sciences :) > > Roel > - > To unsubscribe from this list: send the line "unsubscribe linux-ia64" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Horms, California Edition - To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |