Re: [PATCH] iounmap after ioremap in efi_enter_virtual_mode, file

[Roel Kluin <12o3l@tiscali.nl>]

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))
+				iounmap(md->virt_addr);
+		}
 		printk(KERN_WARNING "warning: unable to switch EFI into virtual mode "
 		       "(status=%lu)\n", status);
 		return;
---

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

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