Re: [patch] MdeModulePkg/SdMmc: Add break to avoid dead loop when polling OCR Reg

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Tian, Feng
> Sent: Monday, March 13, 2017 11:24 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [patch] MdeModulePkg/SdMmc: Add break to avoid dead loop when
> polling OCR Reg
> 
> At worst case, OCR register may always not set BIT31. It will cause
> original code enter to dead loop. Adding a break for such case.
> 
> Cc: Hao Wu <hao.a.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Feng Tian <feng.tian@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 ++++++++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 8 ++++++++
>  MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcHci.c    | 9 ++++++++-
>  MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHci.c        | 9 ++++++++-
>  4 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 9dbec10..6653a87 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -1109,6 +1109,7 @@ EmmcIdentification (
>    EFI_SD_MMC_PASS_THRU_PROTOCOL  *PassThru;
>    UINT32                         Ocr;
>    UINT16                         Rca;
> +  UINTN                          Retry;
> 
>    PciIo    = Private->PciIo;
>    PassThru = &Private->PassThru;
> @@ -1119,7 +1120,8 @@ EmmcIdentification (
>      return Status;
>    }
> 
> -  Ocr = 0;
> +  Ocr   = 0;
> +  Retry = 0;
>    do {
>      Status = EmmcGetOcr (PassThru, Slot, &Ocr);
>      if (EFI_ERROR (Status)) {
> @@ -1127,6 +1129,11 @@ EmmcIdentification (
>        return Status;
>      }
>      Ocr |= BIT30;
> +
> +    if (Retry++ == 100) {

Hi Feng,

Maybe a debug message can be added here to indicate the type of error,
like other error handling logics in this function.

Apart from that, the patch is good to me.
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> +      return EFI_DEVICE_ERROR;
> +    }
> +    gBS->Stall(10 * 1000);
>    } while ((Ocr & BIT31) == 0);
> 
>    Status = EmmcGetAllCid (PassThru, Slot);
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 9122848..1da0849 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -1017,6 +1017,7 @@ SdCardIdentification (
>    UINT8                          PowerCtrl;
>    UINT32                         PresentState;
>    UINT8                          HostCtrl2;
> +  UINTN                          Retry;
> 
>    PciIo    = Private->PciIo;
>    PassThru = &Private->PassThru;
> @@ -1097,12 +1098,19 @@ SdCardIdentification (
>    //    Note here we only support the cards complied with SD physical
>    //    layer simplified spec version 2.0 and version 3.0 and above.
>    //
> +  Ocr   = 0;
> +  Retry = 0;
>    do {
>      Status = SdCardSendOpCond (PassThru, Slot, 0, Ocr, S18r, Xpc, TRUE, &Ocr);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((DEBUG_ERROR, "SdCardIdentification: SdCardSendOpCond fails
> with %r Ocr %x, S18r %x, Xpc %x\n", Status, Ocr, S18r, Xpc));
>        return EFI_DEVICE_ERROR;
>      }
> +
> +    if (Retry++ == 100) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +    gBS->Stall(10 * 1000);
>    } while ((Ocr & BIT31) == 0);
> 
>    //
> diff --git a/MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcHci.c
> b/MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcHci.c
> index 2c0baca..5cb20a3 100644
> --- a/MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcHci.c
> +++ b/MdeModulePkg/Bus/Sd/EmmcBlockIoPei/EmmcHci.c
> @@ -2827,6 +2827,7 @@ EmmcPeimIdentification (
>    EFI_STATUS                     Status;
>    UINT32                         Ocr;
>    UINT32                         Rca;
> +  UINTN                          Retry;
> 
>    Status = EmmcPeimReset (Slot);
>    if (EFI_ERROR (Status)) {
> @@ -2834,13 +2835,19 @@ EmmcPeimIdentification (
>      return Status;
>    }
> 
> -  Ocr = 0;
> +  Ocr   = 0;
> +  Retry = 0;
>    do {
>      Status = EmmcPeimGetOcr (Slot, &Ocr);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((EFI_D_ERROR, "EmmcPeimIdentification: EmmcPeimGetOcr fails
> with %r\n", Status));
>        return Status;
>      }
> +
> +    if (Retry++ == 100) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +    MicroSecondDelay (10 * 1000);
>    } while ((Ocr & BIT31) == 0);
> 
>    Status = EmmcPeimGetAllCid (Slot);
> diff --git a/MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHci.c
> b/MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHci.c
> index 23e6563..81076ba 100644
> --- a/MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHci.c
> +++ b/MdeModulePkg/Bus/Sd/SdBlockIoPei/SdHci.c
> @@ -2754,7 +2754,7 @@ SdPeimIdentification (
>    UINT32                         PresentState;
>    UINT8                          HostCtrl2;
>    SD_HC_SLOT_CAP                 Capability;
> -
> +  UINTN                          Retry;
>    //
>    // 1. Send Cmd0 to the device
>    //
> @@ -2842,12 +2842,19 @@ SdPeimIdentification (
>    //    Note here we only support the cards complied with SD physical
>    //    layer simplified spec version 2.0 and version 3.0 and above.
>    //
> +  Ocr   = 0;
> +  Retry = 0;
>    do {
>      Status = SdPeimSendOpCond (Slot, 0, Ocr, S18r, Xpc, TRUE, &Ocr);
>      if (EFI_ERROR (Status)) {
>        DEBUG ((EFI_D_ERROR, "SdPeimIdentification: SdPeimSendOpCond fails
> with %r Ocr %x, S18r %x, Xpc %x\n", Status, Ocr, S18r, Xpc));
>        return EFI_DEVICE_ERROR;
>      }
> +
> +    if (Retry++ == 100) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +    MicroSecondDelay (10 * 1000);
>    } while ((Ocr & BIT31) == 0);
> 
>    //
> --
> 2.7.1.windows.2