lkml.org 
[lkml]   [2008]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [8/8] RFC: Fix some EFI problems
On Mon, 11 Feb 2008, Andi Kleen wrote:

> >From code review the EFI memory map handling has a couple of problems:
>
> - The test for _WB memory was reversed so it would set cache able memory
> to uncached
> - It would always set a wrong uninitialized zero address to uncached
> (so I suspect it always set the first few pages in phys memory to uncached,
> that is why it may have gone unnoticed)
> - It would call set_memory_x() on a fixmap address that it doesn't
> handle correct.
> - Some other problems I commented in the code (but was unable to solve
> for now)
>
> I changed the ioremaps to set the correct caching attributes
> and also corrected the ordering so it looks roughly correct now.

The only effective change is:

- if (md->attribute & EFI_MEMORY_WB)
+ if (!(md->attribute & EFI_MEMORY_WB))

I appreciate that you noticed the reverse logic, which I messed up
when I fixed up rejects.

I pulled this out as it is a real fix. The rest of this patch is just
turning code in circles for nothing, simply because it is functionally
completely irrelevant whether does simply:

if ((end >> PAGE_SHIFT) <= max_pfn_mapped)
va = __va(md->phys_addr);
else
va = efi_ioremap(md->phys_addr, size);

if (!(md->attribute & EFI_MEMORY_WB))
set_memory_uc(md->virt_addr, size);
or

if ((end >> PAGE_SHIFT) <= max_pfn_mapped) {
va = __va(md->phys_addr);

if (!(md->attribute & EFI_MEMORY_WB))
set_memory_uc(md->virt_addr, size);
} else
va = efi_ioremap(md->phys_addr, size,
!!(md->attribute & EFI_MEMORY_WB));

And you just copied the real bug in that logic as well:

set_memory_uc(md->virt_addr, size);
------------------------^^^^^^^^

which is initialized a couple of lines down.

md->virt_addr = (u64) (unsigned long) va;

The reordering/optimizing needs to be a separate patch.

Please keep bugfixes and other changes separate.

> + /* RED-PEN does not handle overlapped areas */

Can you please use CHECKME/FIXME which is used everywhere else. No need to
invent an extra marker.

Thanks,

tglx


\
 
 \ /
  Last update: 2008-02-12 21:07    [W:0.219 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site