Re: [edk2] [PATCH v7 19/24] ArmVirtualizationPkg: add VirtFdtDxe driver

Subject: Re: [edk2] [PATCH v7 19/24] ArmVirtualizationPkg: add VirtFdtDxe driver

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2014-09-06 16:03:24

On 6 September 2014 02:24, Laszlo Ersek  wrote:
> On 09/05/14 13:56, Ard Biesheuvel wrote:
>> This driver enumerates the device nodes in the device tree located at the
>> base address passed in gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, and
>> installs drivers for devices it cares about (GIC interrupt controller, RTC,
>> architected timer interrupt)
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Reviewed-by: Laszlo Ersek 
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c   | 282 +++++++++++++++++++++
>>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf |  61 +++++
>>  2 files changed, 343 insertions(+)
>>  create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
>>  create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.inf
>
> Thanks for addressing my v4 comments for this patch.
>
> I can also see that PSCI is new in here. Minor nits in relation:
>

Ah yes, apologies for retaining the reviewed-by, I thought I had
removed it due to the changes.

>> +    case PropertyTypePsci:
>> +
>> +      PsciMethod = fdt_getprop (DeviceTreeBase, Node, "method", &Len);
>> +
>> +      if (AsciiStrnCmp (PsciMethod, "hvc", 3) == 0) {
>> +        PcdSet32 (PcdArmPsciMethod, 1);
>> +      } else if (AsciiStrnCmp (PsciMethod, "smc", 3) == 0) {
>> +        PcdSet32 (PcdArmPsciMethod, 2);
>> +      } else {
>> +        DEBUG ((EFI_D_ERROR, "%a: Unknown PSCI method \"%s\"\n", __FUNCTION__,
>> +          PsciMethod));
>> +      }
>
> - I'm not sure if PsciMethod can be NULL after fdt_getprop()

QEMU passing a /psci node without a method property is highly
unlikely, but it is better to be safe than sorry, I suppose.
Does DEBUG () tolerate NULL arguments for %a?

> - "Len" seems to be unchecked -- possibly okay
> - The %s format specifier is incorrect in the DEBUG() call; first you
>   use AsciiStrnCmp() with PsciMethod, hence %a is needed (%s is for
>   CHAR16*)
>

OK

> Very-very unlikely to cause problems in practice; please address it in a
> followup series after this one is committed (or, only if you have to
> repost for another reason, then in v8).
>
> R-b stands.
>
> Thanks
> Laszlo
>

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel