Messages in this thread | | | Date | Tue, 12 Feb 2008 21:04:06 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [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
| |