From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f174.google.com (mail-pg1-f174.google.com [209.85.215.174]) by mx.groups.io with SMTP id smtpd.web11.5484.1686219454402701328 for ; Thu, 08 Jun 2023 03:17:34 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=PUJbyIOA; spf=pass (domain: ventanamicro.com, ip: 209.85.215.174, mailfrom: rsingh@ventanamicro.com) Received: by mail-pg1-f174.google.com with SMTP id 41be03b00d2f7-5440e98616cso1136277a12.0 for ; Thu, 08 Jun 2023 03:17:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1686219454; x=1688811454; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=TswacnCORbMakVE/CeD6JZABjRk31YMiKwDSzT/S0AE=; b=PUJbyIOA3oV0kS64NUg6MlBx6bOg0GQCFNQAVMSeTw2HKa4xh+7UvAVFrYqOwlIa3P rocBxPPG//Lpvm9Q/86v4ZUZ1/AUIXEUQHsguxxeerbgpys3RII+hQnVu1o23DkI0699 WCiNL7/yf97epMjXWXdqCAgOfSBfd8gVS48X5S0ju6MoNo6Oup1CnJQgMaoo+qrSPwgG SyeFFTbzpc7zFPgDb9NK12b56XIliNb6+BDaPhMOBgnNGtc2rm0FFLH0sFR1GoU1qkBg CX3OLypCqT4onplo0JZAYDz4kIE+BhYP0dSrnu9QokEkEDcwIA5CvHrvEm9a1ChlTnok KLdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686219454; x=1688811454; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=TswacnCORbMakVE/CeD6JZABjRk31YMiKwDSzT/S0AE=; b=JSllHnygWQ4ErxRFX2GCqP1lYJ4BROabdH4DSClQdQfaAoYCHQtyn4gTvC6GIoCJkl jc95FhXwXIWWM2jvXkfBcs95T9sDSFpNPd1Z1iePhq4t9w2I7M4eACTIOm80CZf/90B4 VmH4FAVsFeXpI4ICjdJsEGDmwJpEldI1dAZrfHBtv+suJwYLQlhYEDck6v6uzjO0+2xz Uig2zod3pxQsRNM46hzRCKSH6GTVmqf4yaQYpu3s6KmdXoaGqEwV2w4dPZL+4vq119Wp oPEZFWjTCO596sA6K+z5O7knurFNTOxTCS6Ti6tVvu7TK/fobKlshvP6XBD+R3NhNIAb zKmQ== X-Gm-Message-State: AC+VfDxBro4WWO4NuiA728WULy7cElmO1Oq/FnC+nvGdLsMPkGu2jt57 uv+jrjEoRlfwW1bKu6LRrLhCxGJIWYNagYOTuW1pUQ== X-Google-Smtp-Source: ACHHUZ7d51poHqbtmc1Q+fDZfaCmoDL2jjR6erVeBDVh9GlsxltURPIZ09YBgW9z1vBgt1vRuquI/YFrzlB0tOBYwLM= X-Received: by 2002:a17:90a:1959:b0:259:b828:19b6 with SMTP id 25-20020a17090a195900b00259b82819b6mr2106697pjh.16.1686219453749; Thu, 08 Jun 2023 03:17:33 -0700 (PDT) MIME-Version: 1.0 References: <20230602070946.83730-1-rsingh@ventanamicro.com> <20230602070946.83730-2-rsingh@ventanamicro.com> In-Reply-To: From: Ranbir Singh Date: Thu, 8 Jun 2023 15:47:22 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , "ardb@kernel.org" , "pedro.falcato@gmail.com" , "Ni, Ray" Content-Type: multipart/alternative; boundary="0000000000008df6cb05fd9b8fc7" --0000000000008df6cb05fd9b8fc7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I mentioned similar approach in https://bugzilla.tianocore.org/show_bug.cgi?id=3D4204#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 =3D %r\n", Status)); continue; } On Thu, Jun 8, 2023 at 12:25=E2=80=AFPM Wu, Hao A wrot= e: > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Ard > > Biesheuvel > > Sent: Monday, June 5, 2023 4:32 PM > > To: Ranbir Singh > > Cc: devel@edk2.groups.io; pedro.falcato@gmail.com; Wu, Hao A > > ; Ni, Ray > > 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 > > 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 th= ey > > 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 =3D SetDriveParameters (Instance, IdeChannel, IdeDevice, > &DriveParameters, NULL); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status =3D %r\n", > Status)); > continue; > } > > But I do not have the device and platform environment to verify the chang= e. > 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=E2=80=AFAM Ard Biesheuvel wrote: > > >> > > >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato > > wrote: > > >> > > > >> > On Fri, Jun 2, 2023 at 8:42=E2=80=AFPM Ranbir Singh < > rsingh@ventanamicro.com> > > wrote: > > >> > > > > >> > > From: Ranbir Singh > > >> > > > > >> > > The return value stored in Status after call to SetDriveParamete= rs > > >> > > is not made of any use thereafter and hence it remains as UNUSED= . > > >> > > Assuming, this non-usage is deliberate, the storage in Status ca= n > be > > >> > > done away with. > > >> > > > > >> > > Cc: Hao A Wu > > >> > > Cc: Ray Ni > > >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4204 > > >> > > Signed-off-by: Ranbir Singh > > >> > > --- > > >> > > 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 =3D > (UINT8)(((ATA5_IDENTIFY_DATA > > *)(&Buffer.AtaData))->heads - 1); > > >> > > DriveParameters.MultipleSector =3D > (UINT8)((ATA5_IDENTIFY_DATA > > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt; > > >> > > > > >> > > - Status =3D 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 yo= u > > >> 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)); > > > > > >=20 > > > > --0000000000008df6cb05fd9b8fc7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

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

if= (EFI_ERROR (Status)) {
=C2=A0 =C2=A0 DEBUG ((DEBUG_ERROR, "Set dri= ve parameters Fail, Status =3D %r\n", Status));
=C2=A0 =C2=A0 conti= nue;
=C2=A0 }

On Thu, Jun 8, 2023 at 12:25=E2=80=AFPM Wu, Hao A <<= a href=3D"mailto:hao.a.wu@intel.com">hao.a.wu@intel.com> wrote:
<= /div>
> -----Original M= essage-----
> From: devel@= edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Monday, June 5, 2023 4:32 PM
> To: Ranbir Singh <rsingh@ventanamicro.com>
> Cc: devel@ed= k2.groups.io; pedro.falcato@gmail.com; Wu, Hao A
> <hao.a.wu@i= ntel.com>; Ni, Ray <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>
> 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.<= br> > >
> > In the comments / description, I already mentioned - Assuming, th= is non-
> usage is deliberate, so as such I did not intend to hide anything - an= d left it in
> the status quo.
> >
> > The patch proposed may not be appropriate, but should now give a<= br> > thinking point to active module developers / owners / maintainers if t= hey
> 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 b= e:

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

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 c= hange without at least some kind of 'no-break' tests.

Best Regards,
Hao Wu


>
> > On Mon, Jun 5, 2023 at 4:33=E2=80=AFAM Ard Biesheuvel <ardb@kernel.org> wrote= :
> >>
> >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato <pedro.falcato@gmail.com&= gt;
> wrote:
> >> >
> >> > On Fri, Jun 2, 2023 at 8:42=E2=80=AFPM Ranbir Singh <= rsingh@ventana= micro.com>
> wrote:
> >> > >
> >> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > >> > >
> >> > > The return value stored in Status after call to Set= DriveParameters
> >> > > is not made of any use thereafter and hence it rema= ins 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>
> >> > > Cc: Ray Ni <ray.ni@intel.com>
> >> > > REF: https://bugzilla.t= ianocore.org/show_bug.cgi?id=3D4204
> >> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.= com>
> >> > > ---
> >> > >=C2=A0 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode= .c | 2 +-
> >> > >=C2=A0 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 ( > >> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveParameters.Heads=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
> >> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveParameters.Multiple= Sector =3D (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >> > >
> >> > > -=C2=A0 =C2=A0 =C2=A0 Status =3D SetDriveParameters= (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >> > > +=C2=A0 =C2=A0 =C2=A0 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));
>
>
>
>

--0000000000008df6cb05fd9b8fc7--