From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=ZK1+nlPf; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: leif.lindholm@linaro.org) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by groups.io with SMTP; Tue, 06 Aug 2019 10:07:23 -0700 Received: by mail-wr1-f66.google.com with SMTP id p17so88620163wrf.11 for ; Tue, 06 Aug 2019 10:07:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yJRVUQ2kyaWOx3JEi8Im+MzLjEvaX1n7p61DzXVZlmM=; b=ZK1+nlPfHrfG5Xk30ak1QJOs6uhHZ2Tvhr2SSGtS6yvhgPCZZPXzGGrUKKwijaA6Qn vck34X6bdNvD4JrQuLaDelknLxvzNDejMCj42BuhswmLU+IPyU1H0ktrZihcxDsGa5g6 q/TVZm9Rjp+wBaf2RGEwGcsKsPbWkZiPDHsbDgjbriPSuHCIv2aJMnIDUTTusRdrcIgc 2xRQkRbByfhm2qg5SshPjuVG1FVn1zr3teorYusk/+3rmgjeBnocvOC5iEhwjl7y5IZH HK9Axki++zVjHVjASH+RysQf0T8NeZPqKPOYeKF0rqolk8O8Yeo+E+opC6Vb2/HmGO1C ovGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yJRVUQ2kyaWOx3JEi8Im+MzLjEvaX1n7p61DzXVZlmM=; b=tgx/emOAQl7E4Y1DEez8sajjXLt5IdAPTPz+MXNNFJ8e+fjW/cQ5jwrKPy3bTS+RES 1NQCFJxkDjpwKsDFPU1ZGFf1Z3lnAlEecGIkhbGLiKP5VWgWBEaMicicqCWJVsl5b3Sb gMnvXKsM5XcygW0rnUr1SLP/4B5V+IqyqqnTZFlEMVt0mPd1iGGAL//4HMzyPOQiqmOb aBz/xLuz9zhXCtiatsfAfvVy2pFJmroZaE2FK0HuR/N0TKPlkhMJI+esXn8UGUC9fKPY C4ZuxBuKtXZXyp3nFrCXXPKGHiYdHcvmps7qknkrvlbCqL3p43jB5MgTOdekOsGAPJPq 2brA== X-Gm-Message-State: APjAAAU0jHlyAb8HqC5bg/0f6lt+BXATIbu+SqGKGvR/O8qhyKD6EWBH +MvsAbgFTlpvc5P9pO01KPO5gg== X-Google-Smtp-Source: APXvYqwCH1EJGj47o3L9xju6ndamTVq9A2qwD/kL6Apc2+tnmJeefRMJgkvKgWyZQjkwO36dj1GVIg== X-Received: by 2002:a5d:43c9:: with SMTP id v9mr5653118wrr.70.1565111241769; Tue, 06 Aug 2019 10:07:21 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v15sm79661431wrt.25.2019.08.06.10.07.20 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 06 Aug 2019 10:07:21 -0700 (PDT) Date: Tue, 6 Aug 2019 18:07:19 +0100 From: "Leif Lindholm" To: Michael Brown Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org, pete@akeo.ie Subject: Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Report device tree modification errors using Print() Message-ID: <20190806170719.GJ25813@bivouac.eciton.net> References: <20190724214342.GZ11541@bivouac.eciton.net> <20190725102715.572560-2-mbrown@fensystems.co.uk> MIME-Version: 1.0 In-Reply-To: <20190725102715.572560-2-mbrown@fensystems.co.uk> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Jul 25, 2019 at 11:27:15AM +0100, Michael Brown wrote: > Most functions in FdtDxe currently return VOID and report errors using > only DEBUG. Update functions to return EFI_STATUS and report errors > using Print() so that errors are at least visible in non-debug builds. > > Signed-off-by: Michael Brown Apologies for slow review. Happy with all of this, except for the use of EFI_DEVICE_ERROR. Generally, EFI_DEVICE_ERROR is used as an i/o error type status. Either EFI_NOT_FOUND, EFI_INVALID_PARAMETER or EFI_OUT_OF_RECOURCES would be more appropriate when referring to device-tree manipulation. Could you update these and spin a v2 please? Best Regards, Leif > --- > .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 102 ++++++++++++------ > 1 file changed, 70 insertions(+), 32 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c > index 57e4bee8da..17d3bbd82d 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -24,7 +25,7 @@ STATIC VOID *mFdtImage; > STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; > > STATIC > -VOID > +EFI_STATUS > FixEthernetAliases ( > VOID > ) > @@ -36,6 +37,7 @@ FixEthernetAliases ( > UINTN CopySize; > CHAR8 *Copy; > INTN Retval; > + EFI_STATUS Status; > > // > // Look up the 'ethernet[0]' aliases > @@ -43,14 +45,14 @@ FixEthernetAliases ( > Aliases = fdt_path_offset (mFdtImage, "/aliases"); > if (Aliases < 0) { > DEBUG ((DEBUG_ERROR, "%a: failed to locate '/aliases'\n", __FUNCTION__)); > - return; > + return EFI_NOT_FOUND; > } > Ethernet = fdt_getprop (mFdtImage, Aliases, "ethernet", NULL); > Ethernet0 = fdt_getprop (mFdtImage, Aliases, "ethernet0", NULL); > Alias = Ethernet ? Ethernet : Ethernet0; > if (!Alias) { > DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet[0]' alias\n", __FUNCTION__)); > - return; > + return EFI_NOT_FOUND; > } > > // > @@ -60,15 +62,17 @@ FixEthernetAliases ( > Copy = AllocateCopyPool (CopySize, Alias); > if (!Copy) { > DEBUG ((DEBUG_ERROR, "%a: failed to copy '%a'\n", __FUNCTION__, Alias)); > - return; > + return EFI_OUT_OF_RESOURCES; > } > > // > // Create missing aliases > // > + Status = EFI_SUCCESS; > if (!Ethernet) { > Retval = fdt_setprop (mFdtImage, Aliases, "ethernet", Copy, CopySize); > if (Retval != 0) { > + Status = EFI_DEVICE_ERROR; > DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet' alias (%d)\n", > __FUNCTION__, Retval)); > } > @@ -77,6 +81,7 @@ FixEthernetAliases ( > if (!Ethernet0) { > Retval = fdt_setprop (mFdtImage, Aliases, "ethernet0", Copy, CopySize); > if (Retval != 0) { > + Status = EFI_DEVICE_ERROR; > DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet0' alias (%d)\n", > __FUNCTION__, Retval)); > } > @@ -84,10 +89,11 @@ FixEthernetAliases ( > } > > FreePool (Copy); > + return Status; > } > > STATIC > -VOID > +EFI_STATUS > UpdateMacAddress ( > VOID > ) > @@ -103,7 +109,7 @@ UpdateMacAddress ( > Node = fdt_path_offset (mFdtImage, "ethernet"); > if (Node < 0) { > DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__)); > - return; > + return EFI_NOT_FOUND; > } > > // > @@ -112,7 +118,7 @@ UpdateMacAddress ( > Status = mFwProtocol->GetMacAddress (MacAddress); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: failed to retrieve MAC address\n", __FUNCTION__)); > - return; > + return Status; > } > > Retval = fdt_setprop (mFdtImage, Node, "mac-address", MacAddress, > @@ -120,16 +126,17 @@ UpdateMacAddress ( > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "%a: failed to create 'mac-address' property (%d)\n", > __FUNCTION__, Retval)); > - return; > + return EFI_DEVICE_ERROR; > } > > DEBUG ((DEBUG_INFO, "%a: setting MAC address to %02x:%02x:%02x:%02x:%02x:%02x\n", > __FUNCTION__, MacAddress[0], MacAddress[1], MacAddress[2], MacAddress[3], > MacAddress[4], MacAddress[5])); > + return EFI_SUCCESS; > } > > STATIC > -VOID > +EFI_STATUS > CleanMemoryNodes ( > VOID > ) > @@ -139,7 +146,7 @@ CleanMemoryNodes ( > > Node = fdt_path_offset (mFdtImage, "/memory"); > if (Node < 0) { > - return; > + return EFI_SUCCESS; > } > > /* > @@ -150,11 +157,14 @@ CleanMemoryNodes ( > Retval = fdt_del_node (mFdtImage, Node); > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "Failed to remove /memory\n")); > + return EFI_DEVICE_ERROR; > } > + > + return EFI_SUCCESS; > } > > STATIC > -VOID > +EFI_STATUS > SanitizePSCI ( > VOID > ) > @@ -166,7 +176,7 @@ SanitizePSCI ( > Root = fdt_path_offset (mFdtImage, "/"); > ASSERT (Root >= 0); > if (Root < 0) { > - return; > + return EFI_NOT_FOUND; > } > > Node = fdt_path_offset (mFdtImage, "/psci"); > @@ -177,41 +187,42 @@ SanitizePSCI ( > ASSERT (Node >= 0); > if (Node < 0) { > DEBUG ((DEBUG_ERROR, "Couldn't find/create /psci\n")); > - return; > + return EFI_DEVICE_ERROR; > } > > Retval = fdt_setprop_string (mFdtImage, Node, "compatible", "arm,psci-1.0"); > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "Couldn't set /psci compatible property\n")); > - return; > + return EFI_DEVICE_ERROR; > } > > Retval = fdt_setprop_string (mFdtImage, Node, "method", "smc"); > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "Couldn't set /psci method property\n")); > - return; > + return EFI_DEVICE_ERROR; > } > > Root = fdt_path_offset (mFdtImage, "/cpus"); > if (Root < 0) { > DEBUG ((DEBUG_ERROR, "No CPUs to update with PSCI enable-method?\n")); > - return; > + return EFI_NOT_FOUND; > } > > Node = fdt_first_subnode (mFdtImage, Root); > while (Node >= 0) { > if (fdt_setprop_string (mFdtImage, Node, "enable-method", "psci") != 0) { > DEBUG ((DEBUG_ERROR, "Failed to update enable-method for a CPU\n")); > - return; > + return EFI_DEVICE_ERROR; > } > > fdt_delprop (mFdtImage, Node, "cpu-release-addr"); > Node = fdt_next_subnode (mFdtImage, Node); > } > + return EFI_SUCCESS; > } > > STATIC > -VOID > +EFI_STATUS > CleanSimpleFramebuffer ( > VOID > ) > @@ -225,7 +236,7 @@ CleanSimpleFramebuffer ( > */ > Node = fdt_path_offset (mFdtImage, "display0"); > if (Node < 0) { > - return; > + return EFI_SUCCESS; > } > > /* > @@ -236,25 +247,28 @@ CleanSimpleFramebuffer ( > Retval = fdt_del_node (mFdtImage, Node); > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "Failed to remove display0\n")); > - return; > + return EFI_DEVICE_ERROR; > } > > Node = fdt_path_offset (mFdtImage, "/aliases"); > if (Node < 0) { > DEBUG ((DEBUG_ERROR, "Couldn't find /aliases to remove display0\n")); > - return; > + return EFI_NOT_FOUND; > } > > Retval = fdt_delprop (mFdtImage, Node, "display0"); > if (Retval != 0) { > DEBUG ((DEBUG_ERROR, "Failed to remove display0 alias\n")); > + return EFI_DEVICE_ERROR; > } > + > + return EFI_SUCCESS; > } > > #define MAX_CMDLINE_SIZE 512 > > STATIC > -VOID > +EFI_STATUS > UpdateBootArgs ( > VOID > ) > @@ -270,7 +284,7 @@ UpdateBootArgs ( > Node = fdt_path_offset (mFdtImage, "/chosen"); > if (Node < 0) { > DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__)); > - return; > + return EFI_NOT_FOUND; > } > > // > @@ -282,7 +296,7 @@ UpdateBootArgs ( > CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4; > if (!CommandLine) { > DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__)); > - return; > + return EFI_OUT_OF_RESOURCES; > } > > // > @@ -291,12 +305,12 @@ UpdateBootArgs ( > Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__)); > - return; > + return Status; > } > > if (AsciiStrLen (CommandLine + 4) == 0) { > DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__)); > - return; > + return EFI_SUCCESS; > } > > CommandLine[3] = ' '; > @@ -331,6 +345,7 @@ UpdateBootArgs ( > DEBUG_CODE_END (); > > FreePool (CommandLine - 4); > + return EFI_SUCCESS; > } > > > @@ -402,16 +417,39 @@ FdtDxeInitialize ( > Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize); > ASSERT (Retval == 0); > > - SanitizePSCI (); > - CleanMemoryNodes (); > - CleanSimpleFramebuffer (); > - FixEthernetAliases (); > - UpdateMacAddress (); > + Status = SanitizePSCI (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to sanitize PSCI (error %d)\n", Status); > + } > + > + Status = CleanMemoryNodes (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to clean memory nodes (error %d)\n", Status); > + } > + > + Status = CleanSimpleFramebuffer (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to clean frame buffer (error %d)\n", Status); > + } > + > + Status = FixEthernetAliases (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to fix ethernet aliases (error %d)\n", Status); > + } > + > + Status = UpdateMacAddress (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to update MAC address (error %d)\n", Status); > + } > + > if (Internal) { > /* > * A GPU-provided DTB already has the full command line. > */ > - UpdateBootArgs (); > + Status = UpdateBootArgs (); > + if (EFI_ERROR (Status)) { > + Print (L"Failed to update boot arguments (error %d)\n", Status); > + } > } > > DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage)); > -- > 2.21.0 >