Re: [PATCH] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Jan 19, 2017 at 01:49:04PM +0000, Ryan Harkin wrote:
> On 18 January 2017 at 23:27, Daniil Egranov <daniil.egranov@arm.com> 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 <daniil.egranov@arm.com>
> 
> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
> 
> > ---
> >  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
> >