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

["Wu, Hao A" <hao.a.wu@intel.com>]

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

Hello,

Is it possible for you to verify the proposed change on a hard disk working under IDE mode to see:
  a) If it can still be successfully recognized;
  b) The SetDriveParameters call has been reached and returns with no error.

My opinion is that the change needs to be tested at least to ensure it will not bring issue to previously working devices.

Best Regards,
Hao Wu

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Thursday, June 8, 2023 6:17 PM
To: Wu, Hao A <hao.a.wu@intel.com>
Cc: devel@edk2.groups.io; ardb@kernel.org; pedro.falcato@gmail.com; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

I mentioned similar approach in https://bugzilla.tianocore.org/show_bug.cgi?id=4204#c1

Let me know if I should update the patch as Hao proposed -

if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n", Status));
    continue;
  }

On Thu, Jun 8, 2023 at 12:25 PM Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 5, 2023 4:32 PM
> To: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>; 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: Re: [edk2-devel] [PATCH 2/2]
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity
> issue
>
> On Mon, 5 Jun 2023 at 10:10, Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> wrote:
> >
> > I counted myself as not the right person to decide what all to do if Status is
> not successful. Adding the DEBUG statement from the Coverity aspect
> doesn't count as a fix as RELEASE mode behavior remains the same.
> >
> > In the comments / description, I already mentioned - Assuming, this non-
> usage is deliberate, so as such I did not intend to hide anything - and left it in
> the status quo.
> >
> > The patch proposed may not be appropriate, but should now give a
> thinking point to active module developers / owners / maintainers if they
> indeed feel that there is a bug here and it needs to be fixed.
> >
>
> Thanks - I agree that it is a good thing that people are aware of this now.


Judging from the context in DetectAndConfigIdeDevice(), I think a fix can be:

  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, NULL);
  if (EFI_ERROR (Status)) {
    DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n", Status));
    continue;
  }

But I do not have the device and platform environment to verify the change.
Really sorry for this. I am not sure on the potential risk of making this change without at least some kind of 'no-break' tests.

Best Regards,
Hao Wu


>
> > On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel <ardb@kernel.org<mailto:ardb@kernel.org>> wrote:
> >>
> >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>
> wrote:
> >> >
> >> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> wrote:
> >> > >
> >> > > 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.
> >> > > Assuming, this non-usage is deliberate, the storage in Status can be
> >> > > done away with.
> >> > >
> >> > > 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>>
> >> > > ---
> >> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > index 75403886e44a..c6d637afa989 100644
> >> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> >> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> >> > >        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);
> >> > > +      SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> >
> >> > I'm /fairly/ sure this is wrong and that you need to use Status.
> >> >
> >>
> >> Yeah, removing the assignment fixes the coverity warning, but now you
> >> are hiding a bug instead of fixing it.
> >>
> >> SetDriveParameters () can apparently fail, and this is being ignored.
> >> At the very least, we should emit a diagnostic here in DEBUG mode to
> >> log this. E.g.,
> >>
> >> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> >> __func__, Status));
>
>
> 
>

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