From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x229.google.com (mail-wm0-x229.google.com [IPv6:2a00:1450:400c:c09::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C574581E91 for ; Thu, 19 Jan 2017 07:13:11 -0800 (PST) Received: by mail-wm0-x229.google.com with SMTP id c85so292767981wmi.1 for ; Thu, 19 Jan 2017 07:13:11 -0800 (PST) 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=u3MfcbXfIFih/J04LYrYZQmxdkHPLbOHdBB0xF1awLw=; b=R/+cPXcgWsjZwuMiZKOTxhjdI8OqrXeU255KzYDthralK4YcYFaHASKGqBYEL0xVpK 5sY0pSAiixPEPVmkg1ZMz1L180MWsq4KZURFL8L7RuS7zzvD14NMmydj0JtNHkmj5kZG IAAxojbtW3GT2QP51YfhxBSkLKuizpYzS/+wY= 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=u3MfcbXfIFih/J04LYrYZQmxdkHPLbOHdBB0xF1awLw=; b=Gg2WigyfjghL+HmdvQ/zo86HEHX72Lf/v9Sge6TrC0Y+fco+0CrcQ3T2exUPxcoL3v gDk94ORQz5pYam4q8ICEkfrBo/1qekQlvq72IJ5dfsnNhqq6760ljgkvHBQAiGAmGybr YFaxwqWra+IFsBFP602y/XZchECvudnMu/bn0pQdXdUTOKrAKkJ8KJpBCeimJ0ITmcdd Y2gz+/95xGtvm9uHuL+vccXEkUAyho0vyfzqH6O8mXv1klmIRwUvtIIDo2bvnQRkzxSR 3yda/3yLDWfXz996Id4QMrhrbOh4csSttXVS+QXkhFQsnoYINlBeu+o/J0Q7EZ29bmSA GaCw== X-Gm-Message-State: AIkVDXL3rVwcyQ8YOF8eJWGIb9cl04ivA4Yln9Ah+b8zu+gdo/j+Qq8E3MDJKrmGWWgFS9V9 X-Received: by 10.28.229.73 with SMTP id c70mr24041600wmh.82.1484838789879; Thu, 19 Jan 2017 07:13:09 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c81sm13403420wmf.22.2017.01.19.07.13.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Jan 2017 07:13:09 -0800 (PST) Date: Thu, 19 Jan 2017 15:13:07 +0000 From: Leif Lindholm To: Ryan Harkin , Daniil Egranov Cc: "edk2-devel@lists.01.org" Message-ID: <20170119151307.GZ25883@bivouac.eciton.net> References: <1484782032-82342-1-git-send-email-daniil.egranov@arm.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Jan 2017 15:13:12 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Jan 19, 2017 at 01:49:04PM +0000, Ryan Harkin wrote: > On 18 January 2017 at 23:27, Daniil Egranov wrote: > > The Marvell Yukon MAC address load supported only on Juno R1 and R2. > > It disabled for Juno R0 due to PCI issues on this board. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Daniil Egranov > > Tested-by: Ryan Harkin > > > --- > > ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > > index 47ff587..e9e6990 100644 > > --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > > +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c > > @@ -378,6 +378,7 @@ OnEndOfDxe ( > > EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath; > > EFI_HANDLE Handle; > > EFI_STATUS Status; > > + UINT32 JunoRevision; > > > > // > > // PCI Root Complex initialization > > @@ -393,8 +394,12 @@ OnEndOfDxe ( > > Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, FALSE); > > ASSERT_EFI_ERROR (Status); > > > > - Status = ArmJunoSetNicMacAddress (); > > - ASSERT_EFI_ERROR (Status); > > + GetJunoRevision (JunoRevision); > > + > > + if (JunoRevision != JUNO_REVISION_R0) { > > + Status = ArmJunoSetNicMacAddress (); > > + ASSERT_EFI_ERROR (Status); > > This is just an FYI, but I stacked your patch on top of mainline, like this: > > 5f81f61 2017-01-18 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: > Fixed crash on Juno R0 [Daniil Egranov] > 19ca06b 2017-01-19 OvmfPkg: Remove superfluous return statements. > [Thomas Huth] > > The first time I ran this, Juno R0 worked fine, but on R1 and R2, the > assert triggered: > > UEFI firmware (version 5f81f61 built at 11:56:52 on Jan 19 2017) > [snip] > ASSERT_EFI_ERROR (Status = Not Found) > ASSERT [ArmJunoDxe] > /linaro/platforms/uefi/edk2/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c(401): > !EFI_ERROR (Status) > > I worked out what is happening. And it's not to do with this patch. > It's another fall-out from the re-work you did to the previous patch. > It's also ultimately due to a bug the firmware. > > With the initial version of your "Set Marvell Yukon MAC address" > patch, this hang didn't happen. I suspect that was because your error > checking was weaker and certain PCIe failures didn't trigger the > assert. > > To reproduce the error with this commit: > 1) power on and boot R1 or R2 into Shell > I do this by interrupting the boot by pressing ESCAPE and using the boot menu > 2) At the Shell prompt, run "reset -s" to shutdown > 3) At the ARM Boot Loader "Cmd>" prompt, run "reboot" > 4) the board will hang while booting UEFI, assuming the board firmware > doesn't die with constant messages like this: > > ERROR: PCIe CSR read failed to respond > ERROR: SMBus transaction not claimed > > Assuming the problem is firmware, not EDK2, what should we do about it? OK, so instinctively, my reaction was that "the reset -s bug is a system controller firmware bug and we shouldn't work around it". However, since it is actually disrupting Ryan's workflow, which frequently doesn't touch PCI at all, I think downgrading the ASSERT to an error message is a good idea short-term. Daniil - could you make that change please? / Leif > Prior to your "Set Marvell Yukon MAC address" patch, or with the > earlier version, the board would boot anyway, but the Yukon device > would be missing. > > Now it dies. > > I don't know which is worse, but I think hanging is worse than an > ethernet port dropping out. Although hanging is a bit more obvious > that there's a problem... > > > > + } > > } > > > > STATIC > > -- > > 2.7.4 > >