From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by mx.groups.io with SMTP id smtpd.web11.1960.1689145646943633190 for ; Wed, 12 Jul 2023 00:07:27 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=dbXGP8lA; spf=pass (domain: ventanamicro.com, ip: 209.85.214.171, mailfrom: rsingh@ventanamicro.com) Received: by mail-pl1-f171.google.com with SMTP id d9443c01a7336-1b89b75dc1cso2889745ad.1 for ; Wed, 12 Jul 2023 00:07:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1689145646; x=1691737646; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=9kLZYp7mTofTLUANPDZSRVqnX3SwH/Aml878mwfBgJg=; b=dbXGP8lAZrnOyAsKCeiaROZm2rlL0ATwZLb6O0iWRuqTjO+rVlh0Cj2IONGTa28kdB 3q0Tf4OCDE9LGPxoIJrt8/PhsEoypLuvGGjKaKySUhnplo7ehp9lcjb/3qHLcR3m07/f 9onU+EkD7c6eTSqkdfk78SrfBJjTXx3NjjqzAn7fc5XeLPXMTsi6mlHh1qhq3GXCLDzb xp5n66xff0yIbuJdxNE3Frd3f+/eIOcW1xyiTZvL4VvlQFR2cDB8CUZ5KVUmL1/53M0S nFUemn3Efco8jvIsQY+MgezWfXh290zLI7NKgMNDXaVyrIhBBJJEZYUGZ1Ft5FUZNbIi RLCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689145646; x=1691737646; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=9kLZYp7mTofTLUANPDZSRVqnX3SwH/Aml878mwfBgJg=; b=RxnLn/CO5q+8YvfJ/1IJhFgpofOiyN3T8aci7WSVeW2Ikz+X8suMjj8mER69uduzKE 7GBs3jpk05yLHZhp3PrW7EP/vPtM+R+FVMF/lsDlDllRfV6ZrqTPUhhH7TSdRZf6qs+g PnL29PyfCjWPVv7kpzI9mqifeaBmz4AyP9Sa8PEC7DBxo9S6KGbYX78GEFPKIRqPxqzs jaQ/A6aY946FH58i0Rg7gZtJHrD2FNes8NJFjuNGmUoU18sVuFHwu/Pbq6rAivTHe32q SV3yjL+Q0mjfVP55ijIHMwEUP0Afe3IuPXxfjkmqVF2MRoWFtx5giuD5E+sw4lIxXKgi LJpA== X-Gm-Message-State: ABy/qLaNkdd5pZxyoIpR/S4mpOJpJRvOnpbO1pugKkNiL682wceLiI/T AaZf2i7N5UyGYF3l6Gc+LwTsKBjcQ6gf6VMbm2H5UQ== X-Google-Smtp-Source: APBJJlF0tSWZerEoixw4SkL8wYxNY26toatXtQEAeAS1w6N5BzqghTP/7q/75LW9i6uDNiah7yIByqKlyySd5HRqm/w= X-Received: by 2002:a17:902:e843:b0:1b7:de50:7d9c with SMTP id t3-20020a170902e84300b001b7de507d9cmr1459796plg.15.1689145646277; Wed, 12 Jul 2023 00:07:26 -0700 (PDT) MIME-Version: 1.0 References: <20230609123322.191390-1-rsingh@ventanamicro.com> <20230609123322.191390-3-rsingh@ventanamicro.com> In-Reply-To: From: Ranbir Singh Date: Wed, 12 Jul 2023 12:37:15 +0530 Message-ID: Subject: Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , "Ni, Ray" Content-Type: multipart/alternative; boundary="000000000000388105060044de90" --000000000000388105060044de90 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Agreed! Will update accordingly. On Wed, Jul 12, 2023 at 12:36=E2=80=AFPM Wu, Hao A wro= te: > It works for me, better to override by: > > Status =3D EFI_SUCCESS; > > > > Best Regards, > > Hao Wu > > > > *From:* Ranbir Singh > *Sent:* Wednesday, July 12, 2023 3:01 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray > *Subject:* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix > UNUSED_VALUE Coverity issue > > > > Thanks Hao for digging deeper into this. > > > > The if block itself might get knocked off in Release mode when there is > only a DEBUG statement inside it and hence Coverity might still complain. > So, we can override the Status value in this scenario inside the if block > and then proceed normally - let me know if this is acceptable and I will > update the patch as below then > > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status =3D %r\n", > Status)); > > /* Ignore Warning and proceed normally */ > > Status =3D 0; > } > > > > Best Regards, > > Ranbir Singh > > > > On Wed, Jul 12, 2023 at 10:08=E2=80=AFAM Wu, Hao A w= rote: > > Really sorry, > > After referring to the Information Technology - AT Attachment with Packet > Interface (ATA/ATAPI) Specification, > It seems to me that the commands being executed in function > SetDriveParameters() are not mandatory during device initialization. > > 1. INITIALIZE DEVICE PARAMETERS command (ID 0x91h): > This command got obsoleted since ATA/ATAPI-6 spec version. > Also, the return status of SetDriveParameters() is irrelevant with the > execution result of this command. > > 2. SET MULTIPLE MODE command (ID 0xC6h): > My take is that this command is necessary if there is subsequent usage of > command READ MULTIPLE, READ MULTIPLE EXT, WRITE MULTIPLE, or WRITE MULTIP= LE > EXT. > I do not find usage of the above 4 commands within edk2, so I think the > successful execution of SET MULTIPLE MODE command is not mandatory for > detecting hard disk device. > > Based on the above findings, could you help to update the patch to: > Status =3D SetDriveParameters (Instance, IdeChannel, IdeDevice, > &DriveParameters, NULL); > > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status =3D %r\n", > Status)); > } > > Will doing so still please Coverity? > > Best Regards, > Hao Wu > > > -----Original Message----- > > From: Ranbir Singh > > Sent: Friday, June 9, 2023 8:33 PM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Wu, Hao A ; Ni, Ray > > Subject: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix > > UNUSED_VALUE Coverity issue > > > > From: Ranbir Singh > > > > The return value stored in Status after call to SetDriveParameters > > is not made of any use thereafter and hence it remains as UNUSED. > > > > Add error check as is done after calls to SetDeviceTransferMode. > > > > Cc: Hao A Wu > > Cc: Ray Ni > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4204 > > Signed-off-by: Ranbir Singh > > Signed-off-by: Ranbir Singh > > --- > > > > Notes: > > Add error check instead of Status storage removal > > > > MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > > index 75403886e44a..d04b1d95a7f5 100644 > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c > > @@ -2549,13 +2549,18 @@ DetectAndConfigIdeDevice ( > > // > > > > if (DeviceType =3D=3D EfiIdeHarddisk) { > > > > // > > > > - // Init driver parameters > > > > + // Init drive parameters > > > > // > > > > DriveParameters.Sector =3D (UINT8)((ATA5_IDENTIFY_DATA > > *)(&Buffer.AtaData))->sectors_per_track; > > > > DriveParameters.Heads =3D (UINT8)(((ATA5_IDENTIFY_DATA > > *)(&Buffer.AtaData))->heads - 1); > > > > DriveParameters.MultipleSector =3D (UINT8)((ATA5_IDENTIFY_DATA > > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt; > > > > > > > > Status =3D SetDriveParameters (Instance, IdeChannel, IdeDevice, > > &DriveParameters, NULL); > > > > + > > > > + if (EFI_ERROR (Status)) { > > > > + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status =3D %r= \n", > > Status)); > > > > + continue; > > > > + } > > > > } > > > > > > > > // > > > > -- > > 2.34.1 > > --000000000000388105060044de90 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Agreed! Will update accordingly.

On Wed, Jul 12, 2023= at 12:36=E2=80=AFPM Wu, Hao A <ha= o.a.wu@intel.com> wrote:

It works for me, better to override by:

=C2=A0 Status =3D EFI_SUCCESS;

=C2=A0

Best Regards,

Hao Wu

=C2=A0

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Wednesday, July 12, 2023 3:01 PM
To: Wu, Hao A <hao.a.wu@intel.com>
Cc: devel@= edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Subject: Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: F= ix UNUSED_VALUE Coverity issue

=C2=A0

Thanks Hao for digging deeper into this.

=C2=A0

The if block itself might get knocked off in = Release mode when there is only a DEBUG statement inside it and hence Cover= ity might still complain. So, we can override the Status value in this scenario inside the if block and then proceed normally - let me know = if this is acceptable and I will update the patch as below then=C2=A0

=C2=A0 =C2=A0 =C2=A0 if (EFI_ERROR (Status)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((DEBUG_WARN, "Set Drive Parameters = Fail, Status =3D %r\n", Status));

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Ignore Warning and pr= oceed normally */

=C2=A0 =C2=A0 =C2=A0 =C2=A0 Status =3D 0;
=C2=A0 =C2=A0 =C2=A0 }

=C2=A0

Best Regards,

Ranbir Singh

=C2=A0

On Wed, Jul 12, 2023 at 10:08=E2=80=AFAM Wu, Hao A &= lt;hao.a.wu@intel.c= om> wrote:

Really sorry,

After referring to the Information Technology - AT Attachment with Packet I= nterface (ATA/ATAPI) Specification,
It seems to me that the commands being executed in function SetDriveParamet= ers() are not mandatory during device initialization.

1. INITIALIZE DEVICE PARAMETERS command (ID 0x91h):
This command got obsoleted since ATA/ATAPI-6 spec version.
Also, the return status of SetDriveParameters() is irrelevant with the exec= ution result of this command.

2. SET MULTIPLE MODE command (ID 0xC6h):
My take is that this command is necessary if there is subsequent usage of c= ommand READ MULTIPLE, READ MULTIPLE EXT, WRITE MULTIPLE, or WRITE MULTIPLE = EXT.
I do not find usage of the above 4 commands within edk2, so I think the suc= cessful execution of SET MULTIPLE MODE command is not mandatory for detecti= ng hard disk device.

Based on the above findings, could you help to update the patch to:
=C2=A0 =C2=A0 =C2=A0 Status =3D SetDriveParameters (Instance, IdeChannel, I= deDevice, &DriveParameters, NULL);

=C2=A0 =C2=A0 =C2=A0 if (EFI_ERROR (Status)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((DEBUG_WARN, "Set Drive Parameters = Fail, Status =3D %r\n", Status));
=C2=A0 =C2=A0 =C2=A0 }

Will doing so still please Coverity?

Best Regards,
Hao Wu

> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Friday, June 9, 2023 8:33 PM
> To: devel@ed= k2.groups.io; rsingh@ventana= micro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
> UNUSED_VALUE Coverity issue
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The return value stored in Status after call to SetDriveParameters
> is not made of any use thereafter and hence it remains as UNUSED.
>
> Add error check as is done after calls to SetDeviceTransferMode.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <r= ay.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>
> Notes:
>=C2=A0 =C2=A0 =C2=A0Add error check instead of Status storage removal >
>=C2=A0 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++++++-
>=C2=A0 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> index 75403886e44a..d04b1d95a7f5 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> @@ -2549,13 +2549,18 @@ DetectAndConfigIdeDevice (
>=C2=A0 =C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 =C2=A0 if (DeviceType =3D=3D EfiIdeHarddisk) {
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
> -=C2=A0 =C2=A0 =C2=A0 // Init driver parameters
>
> +=C2=A0 =C2=A0 =C2=A0 // Init drive parameters
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveParameters.Sector=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0=3D (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->sectors_per_track;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveParameters.Heads=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =3D (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveParameters.MultipleSector =3D (UINT8)(= (ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
>
>
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Status =3D SetDriveParameters (Instance, Id= eChannel, IdeDevice,
> &DriveParameters, NULL);
>
> +
>
> +=C2=A0 =C2=A0 =C2=A0 if (EFI_ERROR (Status)) {
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "Set Drive Para= meters Fail, Status =3D %r\n",
> Status));
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 continue;
>
> +=C2=A0 =C2=A0 =C2=A0 }
>
>=C2=A0 =C2=A0 =C2=A0 }
>
>
>
>=C2=A0 =C2=A0 =C2=A0 //
>
> --
> 2.34.1

--000000000000388105060044de90--