[PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

[PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

[Ranbir Singh]

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

[PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

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

Re: [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Wu, Hao A]

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

[PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

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

Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Wu, Hao A]

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

Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

[-- 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 --]

Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Wu, Hao A]

[-- 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 --]

Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

[-- 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 --]

Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ard Biesheuvel]

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