5/7] EmbeddedPkg: Fix DwEmmc driver bugs

[edk2-devel] [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs

Posted by Loh, Tien Hock 4 years, 11 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#39939): https://edk2.groups.io/g/devel/message/39939
Mute This Topic: https://groups.io/mt/31480081/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Leif Lindholm 4 years, 11 months ago

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
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#39957): https://edk2.groups.io/g/devel/message/39957
Mute This Topic: https://groups.io/mt/31480081/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Loh, Tien Hock 4 years, 11 months ago

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Friday, May 3, 2019 8:11 PM
> To: Loh, Tien Hock <tien.hock.loh@intel.com>
> Cc: devel@edk2.groups.io; thloh85@gmail.com; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs
> 
> 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.
OK noted. 
> 
> >    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.

Yes I'll move it to after the buffer null check. 

> 
> /
>     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
> >

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#40241): https://edk2.groups.io/g/devel/message/40241
Mute This Topic: https://groups.io/mt/31480081/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-