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
>
> Signed-off-by: Roel Kluin <12o3l@xxxxxxxxxx>
> ---
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index 3f7ea13..af925ab 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -585,6 +585,8 @@ efi_enter_virtual_mode (void)
> efi_desc_size, ia64_boot_param->efi_memdesc_version,
> ia64_boot_param->efi_memmap);
> if (status != EFI_SUCCESS) {
> + if ((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;
> -
> 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
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?
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?
Some other issues that aren't part of your patch but are related.
1. Without the phys_efi patches that I posted to the linux-ia64 about
a year ago, if EFI fails to switch to virtual mode then all
SAL calls will fail. Which in turn means that the kernel will fail to
boot (unless I am mistaken and some machines don't make SAL calls
directly from the kernel).
This is because currently the kernel doesn't have any mechanism to
make SAL calls in physical mode. My patches fixed this and
introduced a switch, to force efi to stay in physical mode, for
testing purpuses. But there was no interest in the code. Actually
there were some suggestions that some machines simply couldn't
perform some opperations with EFI in physical mode.
Basically this means that the will fail to boot. That is, unless I
am mistaken and some machines don't make SAL calls directly from the
kernel.
Given that the kernel can't function with EFI in physical mode
(without the phys_efi patches), I really have to conclude that in
fact EFI never fails to switch itself into virtual mode. Otherwise
there would be machines out there that simply wouldn't boot.
This being the case, there doesn't seem to be a whole lot of point in
making sure the error path cleans up correctly. In fact, perhaps the
error path should be removed all together or just changed to
BUG("unable to switch EFI into virtual mode");
2. It seems to me that the loop in efi_enter_virtual_mode()
could be rewritten as the following without changing its
behaviour, other than debugging output. I have not tested
this theory.
for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
md = p;
if (! (md->attribute & EFI_MEMORY_RUNTIME))
continue;
md->virt_addr = (u64) ioremap(md->phys_addr, 0);
}
--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/
-
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
|