From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.2597.1685953915847364597 for ; Mon, 05 Jun 2023 01:31:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=N1h0XcRI; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 474A060FC6 for ; Mon, 5 Jun 2023 08:31:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AD16BC4339B for ; Mon, 5 Jun 2023 08:31:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685953914; bh=1zlfMcCSvDSENAFEmfTBELhjt8cXAuNVgizPJY01Mf0=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=N1h0XcRIdeUOPTixV2KAwgmeFxrWRuRe7x/SPxaM2MwhKnCSmdUPUxT8p+nIZAVJq Md0NxoV0DwmWL8FStXNuANlXqPYHx8BpFMrMm5sxjUtjFcv5eroySqK+i4nBvBZm6j I82nQbeDyXP0Y7dn8VcJgW82HVQfN6frBYgJz+4gPtpWjtReKCUSoAugmLEUuS8hpL DhoLmTywdVV9bifk4VjCMmS2Pwl660TT2mtSqOFiQD8LaOYMPsAJfY53G5mxq4I2u/ DU/lOgGcwkurJ41/zFdS9m1OppXc8QFnjCdzi8UzbIcXctOf1HEhC6z66ADdwiMdbd giUUc07gNJDmA== Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-2b1b1635661so34426441fa.0 for ; Mon, 05 Jun 2023 01:31:54 -0700 (PDT) X-Gm-Message-State: AC+VfDzRt0wiH0MXE3A3MGZIbK1955i4iaunnEXI/CTCEkJaygCTzb4i 5YmGDR5yFVixNPjifYIeUxpJwBVTijbHIVIK84M= X-Google-Smtp-Source: ACHHUZ4qn/zoSX6Y+0dPTWbGEW4P6FN31s/Qw+rrEdWKTkvrKmwWXgGfHnvPn6932hCI1lpYb68cWk2SYLi35isnPJE= X-Received: by 2002:a2e:8705:0:b0:2b0:259d:f670 with SMTP id m5-20020a2e8705000000b002b0259df670mr3564735lji.4.1685953911581; Mon, 05 Jun 2023 01:31:51 -0700 (PDT) MIME-Version: 1.0 References: <20230602070946.83730-1-rsingh@ventanamicro.com> <20230602070946.83730-2-rsingh@ventanamicro.com> In-Reply-To: From: "Ard Biesheuvel" Date: Mon, 5 Jun 2023 10:31:40 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue To: Ranbir Singh Cc: devel@edk2.groups.io, pedro.falcato@gmail.com, Hao A Wu , Ray Ni Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Stat= us is not successful. Adding the DEBUG statement from the Coverity aspect d= oesn't count as a fix as RELEASE mode behavior remains the same. > > In the comments / description, I already mentioned - Assuming, this non-u= sage 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 fe= el 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. > On Mon, Jun 5, 2023 at 4:33=E2=80=AFAM Ard Biesheuvel w= rote: >> >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato wro= te: >> > >> > 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/MdeMo= dulePkg/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_DA= TA *)(&Buffer.AtaData))->heads - 1); >> > > DriveParameters.MultipleSector =3D (UINT8)((ATA5_IDENTIFY_DAT= A *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt; >> > > >> > > - Status =3D SetDriveParameters (Instance, IdeChannel, IdeDevic= e, &DriveParameters, NULL); >> > > + SetDriveParameters (Instance, IdeChannel, IdeDevice, &DrivePa= rameters, 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));