From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: tien.hock.loh@intel.com)
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
 by groups.io with SMTP; Wed, 08 May 2019 20:40:54 -0700
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
  by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 May 2019 20:40:53 -0700
X-ExtLoop1: 1
Received: from kmsmsx152.gar.corp.intel.com ([172.21.73.87])
  by orsmga004.jf.intel.com with ESMTP; 08 May 2019 20:40:52 -0700
Received: from pgsmsx110.gar.corp.intel.com ([169.254.13.159]) by
 KMSMSX152.gar.corp.intel.com ([169.254.11.16]) with mapi id 14.03.0415.000;
 Thu, 9 May 2019 11:40:51 +0800
From: "Loh, Tien Hock" <tien.hock.loh@intel.com>
To: Leif Lindholm <leif.lindholm@linaro.org>
CC: "devel@edk2.groups.io" <devel@edk2.groups.io>, "thloh85@gmail.com"
	<thloh85@gmail.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs
Thread-Topic: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs
Thread-Index: AQHVAWBaxuIRHXyTMkC2lVBT3ALTIKZYycGAgAllJ/A=
Date: Thu, 9 May 2019 03:40:50 +0000
Message-ID: <EF88013823EA1B42AC7142BA7DA05E0634C9065A@PGSMSX110.gar.corp.intel.com>
References: <1556854023-5486-1-git-send-email-tien.hock.loh@intel.com>
 <1556854023-5486-6-git-send-email-tien.hock.loh@intel.com>
 <20190503121117.eursvy5tozgxz2sc@bivouac.eciton.net>
In-Reply-To: <20190503121117.eursvy5tozgxz2sc@bivouac.eciton.net>
Accept-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
dlp-product: dlpe-windows
dlp-version: 11.0.600.7
dlp-reaction: no-action
x-ctpclassification: CTP_NT
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjk1NmJkYWQtYWU2Yy00NmU1LThiMDYtOWM2MjMxZTkzMzhjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiWUU5SGNrb0lVVFBkbnRuZTdnYWdIaE9odzNYT3JNUHNHZ29Hc3FadUhjcHhpQlIyUzVySGN1R2Z4M01ycXd3ZCJ9
x-originating-ip: [172.30.20.205]
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

> -----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
>=20
> 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 =3D Cmd;
> >      mDwEmmcArgument =3D Argument;
> >    } else {
> > +    mDwEmmcCommand =3D Cmd;
> > +    mDwEmmcArgument =3D Argument;
> >      Status =3D SendCommand (Cmd, Argument);
> >    }
> > +
>=20
> I agree a space looks better here, but please don't add unrelated whitesp=
ace
> as part of a functional change.
OK noted.=20
>=20
> >    return Status;
> >  }
> >
> > @@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
> >    IN UINT32*                    Buffer
> >    )
> >  {
> > +  EFI_STATUS Status =3D EFI_SUCCESS;
> > +
> > +  if(IsPendingReadCommand (mDwEmmcCommand) ||
> > + IsPendingWriteCommand(mDwEmmcCommand))
>=20
>   {
>=20
> > +    Status =3D SendCommand (mDwEmmcCommand, mDwEmmcArgument);
>=20
>   }
>=20
> > +
> >    if (Buffer =3D=3D NULL) {
> >      return EFI_INVALID_PARAMETER;
> >    }
>=20
> 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.=20

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