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

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

[Ranbir Singh]

v2 -> v3:
  - [Patch 2] Update as per review comments

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          | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v3 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>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---

 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 v3 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>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..af022139cf02 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -2549,13 +2549,21 @@ 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_WARN, "Set Drive Parameters Fail, Status = %r\n", Status));
+        //
+        // Ignore warning and proceed normally
+        //
+        Status = EFI_SUCCESS;
+      }
     }
 
     //
-- 
2.34.1

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

[Wu, Hao A]

> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Friday, July 14, 2023 12:47 AM
> 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 v3 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>
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> index 75403886e44a..af022139cf02 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> @@ -2549,13 +2549,21 @@ 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_WARN, "Set Drive Parameters Fail, Status = %r\n",
> + Status));
> 
> +        //
> 
> +        // Ignore warning and proceed normally
> 
> +        //
> 
> +        Status = EFI_SUCCESS;


To please both sides, how about:
1. Remove the 'Status' assignment of the return value from SetDriveParameters()
Based on my findings (https://edk2.groups.io/g/devel/message/106844), the
successful execution of SetDriveParameters() is not mandatory for initializing
IDE mode hard disk device.

2. Add DEBUG_WARN level debug message within SetDriveParameters() function
In function SetDriveParameters, for the 2 calls of AtaNonDataCommandIn (one
for the INITIALIZE DEVICE PARAMETERS command and the other for SET MULTIPLE
MODE command), if the return status is not EFI_SUCCESS, add debug message to
display the information.

Best Regards,
Hao Wu


> 
> +      }
> 
>      }
> 
> 
> 
>      //
> 
> --
> 2.34.1

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

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

>
> To please both sides, how about:
> 1. Remove the 'Status' assignment of the return value from
> SetDriveParameters()
> Based on my findings (https://edk2.groups.io/g/devel/message/106844), the
> successful execution of SetDriveParameters() is not mandatory for
> initializing
> IDE mode hard disk device.
>

Interestingly, my very first patch for this began with this approach only.


> 2. Add DEBUG_WARN level debug message within SetDriveParameters() function
> In function SetDriveParameters, for the 2 calls of AtaNonDataCommandIn (one
> for the INITIALIZE DEVICE PARAMETERS command and the other for SET MULTIPLE
> MODE command), if the return status is not EFI_SUCCESS, add debug message
> to
> display the information.
>

These can be added if desired so.

[-- Attachment #2: Type: text/html, Size: 1276 bytes --]