From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f51.google.com (mail-ot1-f51.google.com [209.85.210.51]) by mx.groups.io with SMTP id smtpd.web10.2369.1685952654861530728 for ; Mon, 05 Jun 2023 01:10:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ventanamicro.com header.s=google header.b=M3gtYLKP; spf=pass (domain: ventanamicro.com, ip: 209.85.210.51, mailfrom: rsingh@ventanamicro.com) Received: by mail-ot1-f51.google.com with SMTP id 46e09a7af769-6b16cbe4fb6so688441a34.1 for ; Mon, 05 Jun 2023 01:10:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1685952654; x=1688544654; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Ira209Ytam+g8czyww3Ka672SPwenGkhQsv1WZ33XlI=; b=M3gtYLKPyi43FXTf+zSD7pUhX5z3HkfMx5brEymnodOYu51dJeuGG3vmIpiC0iJIIc Y9Wr3CvygND676dZpi21+/XVoQONRNmTdodoamGQZ1Q4kixrSvwBx8e9o+uFO4B4PLtw tRrJMhxJKv+1e/tkWWhwDCsZGO/1Zngt+jnG+6pFZdcsaKhPM8nk5RytupWWnNW6tS/F it44fq4t3WZa8ur8O/EEhw3vy2Nxu+vPSCBl9voviyJ2DbOnBY7bcjgUmLvYgg2puQRi edy5uG6CT+qjz4RfgYH+DhjH+Vry/cgjm92Fxr4Z7+HrHfuWs0e1L1RmxKeUhYwztlAg Rq8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685952654; x=1688544654; 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=Ira209Ytam+g8czyww3Ka672SPwenGkhQsv1WZ33XlI=; b=K3bM8VLwgkHhMYzpOvTIIV34E+/64HyP0nGypQ4HcLutr1PPrbGFu29k5X6tZvjghw xjfwAC3aLRJ6E8B2OZgNpyWDN+o1UmM0pvSvDY8O/wdq535r65SVwIBSochjc7ges5g7 qKzmZsFqAXnnQkQoA+pi+aGAfMYqtewubm2DEUolF+dZdqeNgYf25SJi0GF6ApJuSJPM nrVuNeS2lVtSdqs+5iU3R0+yG8fImHeyylCadqXUGvGD5WGqILPkEZuN7BqR+vPdiNvY ghzr0Xg+bjAhu4FM0rgXJATMEQnMbHyQaKO13UfsHIAKKLEm0wwSdnURQQFX1WUXlvbn EsFg== X-Gm-Message-State: AC+VfDwX3r1T4a9S91I150zYBzlqzRKy9QJ3LcGihh46j0f6id5iALQ+ 3bfTHalCFXeE/w5/0MihFHRCwYIJM53KioqDPG0/XQ== X-Google-Smtp-Source: ACHHUZ6cT+/VEDO0xhT4JWC59AqQtxjJ9Cuq03PYENsdDtQIWdLyxmny0G+N9l+dBmLvGmiTDGW+xPVekn+E0MWlr3c= X-Received: by 2002:a05:6358:cb1a:b0:129:c9c9:7c33 with SMTP id gr26-20020a056358cb1a00b00129c9c97c33mr1955561rwb.11.1685952654023; Mon, 05 Jun 2023 01:10:54 -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: Mon, 5 Jun 2023 13:40:43 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue To: Ard Biesheuvel Cc: devel@edk2.groups.io, pedro.falcato@gmail.com, Hao A Wu , Ray Ni Content-Type: multipart/alternative; boundary="0000000000000d367705fd5d718a" --0000000000000d367705fd5d718a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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. On Mon, Jun 5, 2023 at 4:33=E2=80=AFAM Ard Biesheuvel wro= te: > On Sat, 3 Jun 2023 at 18:04, Pedro Falcato > wrote: > > > > On Fri, Jun 2, 2023 at 8:42=E2=80=AFPM Ranbir Singh > wrote: > > > > > > From: Ranbir Singh > > > > > > 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 > > > 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_DAT= A > *)(&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 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)); > --0000000000000d367705fd5d718a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I counted myself as not the right person to decide wh= at 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 behavi= or remains the same.

In the comme= nts / description, I already mentioned -=C2=A0Assuming, this non-usage is deliberate, so as such I did not i= ntend 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.=C2=A0 =C2= =A0=C2=A0

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> wrote:
>
> On Fri, Jun 2, 2023 at 8:42=E2=80=AFPM Ranbir Singh <rsingh@ventanamicro.com&= gt; wrote:
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The return value stored in Status after call to SetDriveParameter= s
> > 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>
> > Cc: Ray Ni <ray.ni@intel.com>
> > REF: https://bugzilla.tianocore.org/s= how_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/Md= eModulePkg/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.MultipleSector =3D (UI= NT8)((ATA5_IDENTIFY_DATA *)(&Buffer.AtaData))->multi_sector_cmd_max_= sct_cnt;
> >
> > -=C2=A0 =C2=A0 =C2=A0 Status =3D SetDriveParameters (Instance, Id= eChannel, IdeDevice, &DriveParameters, NULL);
> > +=C2=A0 =C2=A0 =C2=A0 SetDriveParameters (Instance, IdeChannel, I= deDevice, &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));
--0000000000000d367705fd5d718a--