* [PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
@ 2023-06-09 12:33 Ranbir Singh
2023-06-09 12:33 ` [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue Ranbir Singh
2023-06-09 12:33 ` [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE " Ranbir Singh
0 siblings, 2 replies; 9+ messages in thread
From: Ranbir Singh @ 2023-06-09 12:33 UTC (permalink / raw)
To: devel, rsingh
v1 -> v2:
- Retain outer cast
- Add error check instead of Status storage removal
Ranbir Singh (2):
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue
2023-06-09 12:33 [PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix Ranbir Singh
@ 2023-06-09 12:33 ` Ranbir Singh
2023-07-12 4:38 ` Wu, Hao A
2023-06-09 12:33 ` [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE " Ranbir Singh
1 sibling, 1 reply; 9+ messages in thread
From: Ranbir Singh @ 2023-06-09 12:33 UTC (permalink / raw)
To: devel, rsingh; +Cc: Hao A Wu, Ray Ni
From: Ranbir Singh <Ranbir.Singh3@Dell.com>
Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
Notes:
Retain outer cast
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..f39c909d0631 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
// Check logical block size
//
if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
- BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+ BlockSize = (UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-06-09 12:33 [PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix Ranbir Singh
2023-06-09 12:33 ` [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue Ranbir Singh
@ 2023-06-09 12:33 ` Ranbir Singh
2023-07-12 4:38 ` Wu, Hao A
1 sibling, 1 reply; 9+ messages in thread
From: Ranbir Singh @ 2023-06-09 12:33 UTC (permalink / raw)
To: devel, rsingh; +Cc: Hao A Wu, Ray Ni
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 <ray.ni@intel.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
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 == EfiIdeHarddisk) {
//
- // Init driver parameters
+ // Init drive parameters
//
DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->sectors_per_track;
DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->heads - 1);
DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n", Status));
+ continue;
+ }
}
//
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-06-09 12:33 ` [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE " Ranbir Singh
@ 2023-07-12 4:38 ` Wu, Hao A
2023-07-12 7:00 ` Ranbir Singh
0 siblings, 1 reply; 9+ messages in thread
From: Wu, Hao A @ 2023-07-12 4:38 UTC (permalink / raw)
To: Ranbir Singh, devel@edk2.groups.io; +Cc: Ni, Ray
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 MULTIPLE 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 = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n", Status));
}
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@edk2.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 <ray.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>
> 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 == EfiIdeHarddisk) {
>
> //
>
> - // Init driver parameters
>
> + // Init drive parameters
>
> //
>
> DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->sectors_per_track;
>
> DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
>
> DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
>
>
>
> Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
> +
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> Status));
>
> + continue;
>
> + }
>
> }
>
>
>
> //
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue
2023-06-09 12:33 ` [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue Ranbir Singh
@ 2023-07-12 4:38 ` Wu, Hao A
0 siblings, 0 replies; 9+ messages in thread
From: Wu, Hao A @ 2023-07-12 4:38 UTC (permalink / raw)
To: Ranbir Singh, devel@edk2.groups.io; +Cc: Ni, Ray
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Friday, June 9, 2023 8:33 PM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
> SIGN_EXTENSION Coverity issue
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> Line number 1348 does contain a typecast with UINT32, but it is after
> all the operations (16-bit left shift followed by OR'ing) are over.
> To avoid any SIGN_EXTENSION, typecast the intermediate result after
> 16-bit left shift operation immediately with UINT32.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>
> Notes:
> Retain outer cast
>
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> index 50406fe0270d..f39c909d0631 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
> // Check logical block size
>
> //
>
> if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
>
> - BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16)
> | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>
> + BlockSize = (UINT32)(((UINT32)(IdentifyData-
> >AtaData.logic_sector_size_hi << 16) | IdentifyData-
> >AtaData.logic_sector_size_lo) * sizeof (UINT16));
>
> }
>
> }
>
>
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-07-12 4:38 ` Wu, Hao A
@ 2023-07-12 7:00 ` Ranbir Singh
2023-07-12 7:05 ` Wu, Hao A
0 siblings, 1 reply; 9+ messages in thread
From: Ranbir Singh @ 2023-07-12 7:00 UTC (permalink / raw)
To: Wu, Hao A; +Cc: devel@edk2.groups.io, Ni, Ray
[-- Attachment #1: Type: text/plain, Size: 4364 bytes --]
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 = %r\n",
Status));
/* Ignore Warning and proceed normally */
Status = 0;
}
Best Regards,
Ranbir Singh
On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
> 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 MULTIPLE
> 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 = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
> Status));
> }
>
> 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@edk2.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 <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >
> > 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 == EfiIdeHarddisk) {
> >
> > //
> >
> > - // Init driver parameters
> >
> > + // Init drive parameters
> >
> > //
> >
> > DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->sectors_per_track;
> >
> > DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> >
> > DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> >
> >
> > Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> >
> > +
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> > Status));
> >
> > + continue;
> >
> > + }
> >
> > }
> >
> >
> >
> > //
> >
> > --
> > 2.34.1
>
>
[-- Attachment #2: Type: text/html, Size: 6108 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-07-12 7:00 ` Ranbir Singh
@ 2023-07-12 7:05 ` Wu, Hao A
2023-07-12 7:07 ` Ranbir Singh
2023-07-12 8:28 ` [edk2-devel] " Ard Biesheuvel
0 siblings, 2 replies; 9+ messages in thread
From: Wu, Hao A @ 2023-07-12 7:05 UTC (permalink / raw)
To: Ranbir Singh; +Cc: devel@edk2.groups.io, Ni, Ray
[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]
It works for me, better to override by:
Status = EFI_SUCCESS;
Best Regards,
Hao Wu
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: 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 = %r\n", Status));
/* Ignore Warning and proceed normally */
Status = 0;
}
Best Regards,
Ranbir Singh
On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
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 MULTIPLE 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 = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n", Status));
}
Will doing so still please Coverity?
Best Regards,
Hao Wu
> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> Sent: Friday, June 9, 2023 8:33 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto: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<mailto: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<mailto:hao.a.wu@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> ---
>
> 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 == EfiIdeHarddisk) {
>
> //
>
> - // Init driver parameters
>
> + // Init drive parameters
>
> //
>
> DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->sectors_per_track;
>
> DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
>
> DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
>
>
>
> Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
> +
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> Status));
>
> + continue;
>
> + }
>
> }
>
>
>
> //
>
> --
> 2.34.1
[-- Attachment #2: Type: text/html, Size: 9654 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-07-12 7:05 ` Wu, Hao A
@ 2023-07-12 7:07 ` Ranbir Singh
2023-07-12 8:28 ` [edk2-devel] " Ard Biesheuvel
1 sibling, 0 replies; 9+ messages in thread
From: Ranbir Singh @ 2023-07-12 7:07 UTC (permalink / raw)
To: Wu, Hao A; +Cc: devel@edk2.groups.io, Ni, Ray
[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]
Agreed! Will update accordingly.
On Wed, Jul 12, 2023 at 12:36 PM Wu, Hao A <hao.a.wu@intel.com> wrote:
> It works for me, better to override by:
>
> Status = EFI_SUCCESS;
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *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: 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 = %r\n",
> Status));
>
> /* Ignore Warning and proceed normally */
>
> Status = 0;
> }
>
>
>
> Best Regards,
>
> Ranbir Singh
>
>
>
> On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> 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 MULTIPLE
> 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 = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
> Status));
> }
>
> 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@edk2.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 <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >
> > 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 == EfiIdeHarddisk) {
> >
> > //
> >
> > - // Init driver parameters
> >
> > + // Init drive parameters
> >
> > //
> >
> > DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->sectors_per_track;
> >
> > DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> >
> > DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> >
> >
> > Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> >
> > +
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> > Status));
> >
> > + continue;
> >
> > + }
> >
> > }
> >
> >
> >
> > //
> >
> > --
> > 2.34.1
>
>
[-- Attachment #2: Type: text/html, Size: 8618 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
2023-07-12 7:05 ` Wu, Hao A
2023-07-12 7:07 ` Ranbir Singh
@ 2023-07-12 8:28 ` Ard Biesheuvel
1 sibling, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2023-07-12 8:28 UTC (permalink / raw)
To: devel, hao.a.wu; +Cc: Ranbir Singh, Ni, Ray
On Wed, 12 Jul 2023 at 09:06, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> It works for me, better to override by:
>
> Status = EFI_SUCCESS;
>
>
So now we're adding unnecessary assignments just to please coverity? I
don't think this is a good idea.
If Coverity does not understand that the source references Status
after the assignment (which it does but only in DEBUG mode), fix
coverity, not the code.
>
> 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: 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 = %r\n", Status));
>
> /* Ignore Warning and proceed normally */
>
> Status = 0;
> }
>
>
>
> Best Regards,
>
> Ranbir Singh
>
>
>
> On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> 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 MULTIPLE 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 = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
>
> if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n", Status));
> }
>
> 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@edk2.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 <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >
> > 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 == EfiIdeHarddisk) {
> >
> > //
> >
> > - // Init driver parameters
> >
> > + // Init drive parameters
> >
> > //
> >
> > DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->sectors_per_track;
> >
> > DriveParameters.Heads = (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> >
> > DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> >
> >
> > Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> >
> > +
> >
> > + if (EFI_ERROR (Status)) {
> >
> > + DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> > Status));
> >
> > + continue;
> >
> > + }
> >
> > }
> >
> >
> >
> > //
> >
> > --
> > 2.34.1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-07-12 8:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-09 12:33 [PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix Ranbir Singh
2023-06-09 12:33 ` [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue Ranbir Singh
2023-07-12 4:38 ` Wu, Hao A
2023-06-09 12:33 ` [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE " Ranbir Singh
2023-07-12 4:38 ` Wu, Hao A
2023-07-12 7:00 ` Ranbir Singh
2023-07-12 7:05 ` Wu, Hao A
2023-07-12 7:07 ` Ranbir Singh
2023-07-12 8:28 ` [edk2-devel] " Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox