Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs

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

On Fri, May 03, 2019 at 11:27:01AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Send command when MMC ask for response in DwEmmcReceiveResponse, and
> command is a pending command (eg. DMA needs to be set up first)
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index 32572a9..a69d9ab 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -398,8 +398,11 @@ DwEmmcSendCommand (
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
>    } else {
> +    mDwEmmcCommand = Cmd;
> +    mDwEmmcArgument = Argument;
>      Status = SendCommand (Cmd, Argument);
>    }
> +

I agree a space looks better here, but please don't add unrelated
whitespace as part of a functional change.

>    return Status;
>  }
>  
> @@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
>    IN UINT32*                    Buffer
>    )
>  {
> +  EFI_STATUS Status = EFI_SUCCESS;
> +
> +  if(IsPendingReadCommand (mDwEmmcCommand) || IsPendingWriteCommand(mDwEmmcCommand))

  {

> +    Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);

  }

> +
>    if (Buffer == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }

Should this test not come first in the function?
If the code is relying on the side effect of the SendCommand () being
performed even if Buffer is invalid, that needs a very detailed
comment.

/
    Leif


> @@ -427,7 +435,7 @@ DwEmmcReceiveResponse (
>      Buffer[2] = MmioRead32 (DWEMMC_RESP2);
>      Buffer[3] = MmioRead32 (DWEMMC_RESP3);
>    }
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  VOID
> -- 
> 2.2.2
>