On 08/10/17 03:03, Andrew Fish wrote:
> It looks to me like if you Free pages, after you free pages you hit this 
> DEBUG message. 
> 
> 
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Page.c#L790
> 
>       if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == 
> EfiConventionalMemory ? 1 : 0)) {
>         DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory 
> types\n"));
>         return EFI_NOT_FOUND;
>       }
> 
> I'm not sure I've thought out all the paths, but would it make more sense if 
> you are trying to convert EfiConventionalMemory to EfiConventionalMemory that 
> you are trying to free pages that are already freed. That is not very obvious 
> from the above DEBUG print.  Could there be an if in the error path to print 
> a better DEBUG message for a free pages bug? 
> 
> Also to be pedantic the function change names to: CoreConvertPagesEx(). 

(Reacting only to this side question:)

I generally prefer:

  DEBUG ((
    DebugMask,
    "%a: ...",
    __FUNCTION__,
    ...
    ));

This way when the function is renamed, or the DEBUG is moved to another
function, the log will update itself.

Taking it one step further: if the DEBUG is in a widely used library
instance, then I like

  DEBUG ((
    DebugMask,
    "%a:%a: ...",
    gEfiCallerBaseName,
    __FUNCTION__,
    ...
    ));

Because this identifies the calling driver module as well (using the
BASE_NAME from the module's INF file).

("%g" with &gEfiCallerIdGuid would be more robust than "%a" with
gEfiCallerBaseName, but the log should also be readable...)

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to