From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f182.google.com (mail-qk1-f182.google.com [209.85.222.182]) by mx.groups.io with SMTP id smtpd.web10.1822.1689145252255751479 for ; Wed, 12 Jul 2023 00:00:52 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=YgnGmA9H; spf=pass (domain: ventanamicro.com, ip: 209.85.222.182, mailfrom: rsingh@ventanamicro.com) Received: by mail-qk1-f182.google.com with SMTP id af79cd13be357-7659924cd9bso733576985a.1 for ; Wed, 12 Jul 2023 00:00:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1689145251; x=1691737251; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=xyy1u/W/0lz8DManQr0dQlNAiqtMeNMwE//aEOtCAJE=; b=YgnGmA9HE1zoeFrSDw0iPHNqK9UWR6MSfSeHHC8nj/nWw+zs2jPK/akcgHrlwfZMie tUgme47rCK5BsaeF9dIBGgVR+nNxJWbeGsjeBv+h5YLrWSvzPgMaE/IRdt1h9wChPZ3h n444EEiz4R8F3BzQD48fS4LUkfU6CYFd9OoG0GM3b93AFIKTuo5MW0lKsYKlns/ZfLg3 21nlJYIIYblpPlxdQEnGB1x9cTSGElmiXrQnLa07to2MrLf03iMlt0OCNnS3TrwJ56P3 F8KClb4NdEBWCDHH6z6nCrRWQ4wMazLLnIk4ttN+odaJiBrwH0GvPNRAx5XwFJxEMzp0 vP8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689145251; x=1691737251; 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=xyy1u/W/0lz8DManQr0dQlNAiqtMeNMwE//aEOtCAJE=; b=Sfo5GwKabhHhT/IHUh0rU1dKmGLVCCFyDYn0ws2ITP0kMGBxD8DY02ISAtErUSPYsR sZgxj4OoKcjfbneqLlPS/YVWLi8DTJ+MCLHEuZeK/cWoZ2d+JDZUdLhs2KJw3QFgdPxL PNVlsdgwf4k8/lBSUrC4hd7V+CND689fj8wozrqRAOwJGGVOFnvI0DYRZ2UjfdQGWdIE YWUC7kHh/qJtO9ynFQ380GnySrY51Yk8qivgzFJaOAd/+Gr+x1F0Om27Tvs/92y8IrBd 8jY3cqz4kAnmNLUOf2lzRunObdJ36eneawDXb6zsehtrzv4Ct5cYUZD2bDjJg2pm6RNE G93A== X-Gm-Message-State: ABy/qLZg8qi44RMHd8AzG0Vm23YhRJciC3QnRiK1vpGHEv1PSjhfe/hL uFAJQsfZkGzgL+vt+LslQEoR2Cp821i+ZHMXvkY8Wg== X-Google-Smtp-Source: APBJJlHIqIYIgPbe8+wDJ+kvhLQ9qwy/ol1cH4XdG39Ku1brgXZRp4gHgUCNG0zHjZjtGF+xwo20r5/W0bHh6etsf+g= X-Received: by 2002:a05:620a:4001:b0:767:6710:9636 with SMTP id h1-20020a05620a400100b0076767109636mr22374465qko.77.1689145251169; Wed, 12 Jul 2023 00:00:51 -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:30:40 +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="000000000000ab99b4060044c671" --000000000000ab99b4060044c671 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 wro= te: > 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 > > --000000000000ab99b4060044c671 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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 C= overity 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 thi= s 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 pro= ceed normally */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Status =3D 0;
=C2= =A0 =C2=A0 =C2=A0 }

Best Regards,
Ranbir= Singh

On Wed, Jul 12, 2023 at 10:08=E2=80=AFAM Wu, Hao A <hao.a.wu@intel.com> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">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@ventanamicro.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_b= ug.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

--000000000000ab99b4060044c671--