[v2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

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

Posted by Ranbir Singh 2 years, 8 months ago

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105982): https://edk2.groups.io/g/devel/message/105982
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Wu, Hao A 2 years, 7 months ago

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106844): https://edk2.groups.io/g/devel/message/106844
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ranbir Singh 2 years, 7 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106870): https://edk2.groups.io/g/devel/message/106870
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Wu, Hao A 2 years, 7 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106849): https://edk2.groups.io/g/devel/message/106849
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ranbir Singh 2 years, 7 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106871): https://edk2.groups.io/g/devel/message/106871
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ard Biesheuvel 2 years, 7 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106851): https://edk2.groups.io/g/devel/message/106851
Mute This Topic: https://groups.io/mt/99432080/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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