From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7247ED801DB for ; Wed, 8 Nov 2023 04:29:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KcW/gFjWZu13shCoCMzEDp+xy6kyu+1LNWHWl8zKei0=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1699417791; v=1; b=aDv9SNfrCwjwqQSXtVl0etbVZCvENlg0ImnFUPT7Wb0Cl16uSkhhjvcKfn6u5IaNS00dpSc4 PTvw8vGlaPqCEaDSaLcKnhNqM2Fqn17d9nNRW9MhXwVO7J9dsZosZaFOGYTSkaqR8K+nCwqrWYa hfucEvsTH1bVohl5MIQW09jU= X-Received: by 127.0.0.2 with SMTP id VdnjYY7687511x1ejt3MFgnu; Tue, 07 Nov 2023 20:29:51 -0800 X-Received: from mail-pg1-f181.google.com (mail-pg1-f181.google.com [209.85.215.181]) by mx.groups.io with SMTP id smtpd.web10.7197.1699417790470288477 for ; Tue, 07 Nov 2023 20:29:50 -0800 X-Received: by mail-pg1-f181.google.com with SMTP id 41be03b00d2f7-5b8c39a2dceso3878815a12.2 for ; Tue, 07 Nov 2023 20:29:50 -0800 (PST) X-Gm-Message-State: k3jZt7v6agh38xK2a4UCqY97x7686176AA= X-Google-Smtp-Source: AGHT+IGpdo5C339J49fn4N2OSOw2kYG8NJUloNmSqS+RPoiLGNOhS+IBlkG9dkWz2u5GMNAtnori6xhpJbZ1CdXt5Nw= X-Received: by 2002:a05:6a20:3d1a:b0:153:73ec:fe70 with SMTP id y26-20020a056a203d1a00b0015373ecfe70mr1099360pzi.13.1699417789684; Tue, 07 Nov 2023 20:29:49 -0800 (PST) MIME-Version: 1.0 References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-3-rsingh@ventanamicro.com> <88d1e976-505d-699e-013b-43891a94eb2b@redhat.com> In-Reply-To: From: "Ranbir Singh" Date: Wed, 8 Nov 2023 09:59:38 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "lersek@redhat.com" , "Ni, Ray" , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000adf69d06099c8978" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=aDv9SNfr; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none --000000000000adf69d06099c8978 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Mike, I agree that any manual inspection is sort of a burden, more so when it becomes repetitive in the long run. When I was doing these Coverity checks (Nov-Dec' 2022), I was working in Dell and had access to the real systems to check the execution flow as well as the Coverity status. I could never post these patches while being there, but happened to raise Bugzilla's and post them there instead hoping that they would be taken up by somebody further. I am no longer with Dell and later on when I found that those BZ / issues pointed out by Coverity still exist as there are no code changes in related contexts, I thought of taking them forward in whatever limited capacity I can. I am a bit hesitant to do any further code changes as now I do not have any systems to check the execution flow as well as the Coverity status. So, I do not guarantee, but will try to make the code changes wherever it is easy to ascertain that the functional flow would not be impacted and the same issue won't exist anymore. Ranbir Singh On Wed, Nov 8, 2023 at 9:35=E2=80=AFAM Kinney, Michael D wrote: > Hi Ranbir, > > > > Ignoring false positive in static analysis tools is typically a burden. > > > > It is better to avoid false positives with code changes as long as the > code changes do not make the implementation confusing and hard to maintai= n. > > > > I think depending on fall through case statements is confusing and > removing them will make the code easier to understand and maintain. > > > > Mike > > > > *From:* Ranbir Singh > *Sent:* Tuesday, November 7, 2023 7:51 PM > *To:* Kinney, Michael D > *Cc:* devel@edk2.groups.io; lersek@redhat.com; Ni, Ray = ; > Veeresh Sangolli > *Subject:* Re: [edk2-devel] [PATCH v2 2/5] > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues > > > > As mentioned in the commit message, the comment helps in making it > explicit and evident that the missing break is not a human miss, but > intentional. > > Hence, the comment should be considered as being added for the human code > readers / developers. > > > > So even if some static analysis tool flags such an issue, it can be fairl= y > easy now to ignore that on manual inspection. If desired this can also be > stated in the comment itself like - > > > > + // > + // No break here as this is an intentional fall throug= h. > > + // Ignore any static tool issue if pointed. > + // > > > > Yes, there can be other solutions (which may or may not be worth the > effort), but for now I went with the least code change approach. > > > > On Tue, Nov 7, 2023 at 11:29=E2=80=AFPM Kinney, Michael D < > michael.d.kinney@intel.com> wrote: > > This comment style only works with Coverity. > > Other static analysis tools may flag the same issue again. > > It is better to update the logic so no static analysis tool will > flag this issue. > > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Laszlo > > Ersek > > Sent: Tuesday, November 7, 2023 8:23 AM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Ni, Ray ; Veeresh Sangolli > > > > Subject: Re: [edk2-devel] [PATCH v2 2/5] > > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues > > > > On 11/7/23 07:19, Ranbir Singh wrote: > > > From: Ranbir Singh > > > > > > The function UpdatePciInfo has switch-case code in which there are > > fall > > > through from case 32: to case 64:. While this is seeemingly > > intentional, > > > it is not evident to any general code reader why there is no break; > > in > > > between. Adding > > > > > > // No break; here as this is an intentional fallthrough. > > > > > > as comment in between makes it explicit. Incidentally, the comment > > > satisfies Coverity as well. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4239 > > > > > > Cc: Ray Ni > > > Co-authored-by: Veeresh Sangolli > > > Signed-off-by: Ranbir Singh > > > Signed-off-by: Ranbir Singh > > > --- > > > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > index 6594b8eae83f..eda97285ee18 100644 > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > > @@ -1428,6 +1428,9 @@ UpdatePciInfo ( > > > switch (Ptr->AddrSpaceGranularity) { > > > case 32: > > > PciIoDevice->PciBar[BarIndex].BarType =3D > > PciBarTypeMem32; > > > + // > > > + // No break; here as this is an intentional fall > > through. > > > + // > > > case 64: > > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =3D > > TRUE; > > > break; > > > @@ -1440,6 +1443,9 @@ UpdatePciInfo ( > > > switch (Ptr->AddrSpaceGranularity) { > > > case 32: > > > PciIoDevice->PciBar[BarIndex].BarType =3D > > PciBarTypePMem32; > > > + // > > > + // No break; here as this is an intentional fall > > through. > > > + // > > > case 64: > > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =3D > > TRUE; > > > break; > > > > Agree, but the semicolon's placement is awkward. I propose > > > > No break here, as this is an intentional fall through. > > > > Reviewed-by: Laszlo Ersek > > > > > > > >=20 > > > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110896): https://edk2.groups.io/g/devel/message/110896 Mute This Topic: https://groups.io/mt/102438299/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000adf69d06099c8978 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Mike,

I agree that any manual inspec= tion is sort of a burden, more so when it becomes=C2=A0repetitive in the lo= ng run.

When I was doing these Coverity checks (Nov-Dec&= #39; 2022), I was working in Dell and had access to the real systems to che= ck the execution flow as well as the Coverity status. I could never post th= ese patches while being there, but happened to raise Bugzilla's and pos= t them there instead hoping that they would be taken up by somebody further= .

I am no longer with Dell and later on when I fou= nd that those BZ / issues pointed out by Coverity still exist as there are = no code changes in related contexts, I thought of taking them forward in wh= atever limited capacity=C2=A0I can. I am a bit hesitant to do any further c= ode changes as now I do not have any systems to check the execution flow as= well as the Coverity status. So, I do not guarantee, but will try to make = the code changes wherever it is easy to ascertain that the functional flow = would not be impacted and the same issue won't exist anymore.

Ranbir Singh

On Wed, Nov 8, 2023 at 9:35=E2=80= =AFAM Kinney, Michael D <michael.d.kinney@intel.com> wrote:

Hi Ranbir,

=C2=A0

Ignoring false positive in static analysis tools is = typically a burden.

=C2=A0

It is better to avoid false positives with code chan= ges as long as the code changes do not make the implementation confusing an= d hard to maintain.

=C2=A0

I think depending on fall through case statements is= confusing and removing them will make the code easier to understand and ma= intain.

=C2=A0

Mike

=C2=A0

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Tuesday, November 7, 2023 7:51 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@= edk2.groups.io; = lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellt= eam.com>
Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBus= Dxe: Fix MISSING_BREAK Coverity issues

=C2=A0

As mentioned in the commit message, the comment help= s in making it explicit=C2=A0and evident that the missing break is not a hu= man miss, but intentional.

Hence, the comment should be considered as being add= ed for the human code readers / developers.

=C2=A0

So even if some static analysis tool flags such an i= ssue, it can be fairly easy now to ignore that on manual=C2=A0inspection. I= f desired this can also be stated in the comment itself like -

=C2=A0

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 //
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // No break= here as this is an intentional fall through.

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 // Ignore any static tool issue if pointed.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //

=C2=A0

Yes, there can be other solutions (which may or may = not be worth the effort), but for now I went with the least code change app= roach.

=C2=A0

On Tue, Nov 7, 2023 at 11:29=E2=80=AFPM Kinney, Mich= ael D <m= ichael.d.kinney@intel.com> wrote:

This comment style only= works with Coverity.

Other static analysis tools may flag the same issue again.

It is better to update the logic so no static analysis tool will
flag this issue.

Mike

> -----Original Message-----
> From: devel@= edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@ed= k2.groups.io; rsingh@ventana= micro.com
> Cc: Ni, Ray <= ray.ni@intel.com>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com>
> Subject: Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
>
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function UpdatePciInfo has switch-case code in which there ar= e
> fall
> > through from case 32: to case 64:. While this is seeemingly
> intentional,
> > it is not evident to any general code reader why there is no brea= k;
> in
> > between. Adding
> >
> >=C2=A0 =C2=A0 =C2=A0// No break; here as this is an intentional fa= llthrough.
> >
> > as comment in between makes it explicit. Incidentally, the commen= t
> > satisfies Coverity as well.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4239
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>= ;
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >=C2=A0 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 += +++++
> >=C2=A0 1 file changed, 6 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.= c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > index 6594b8eae83f..eda97285ee18 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (Pt= r->AddrSpaceGranularity) {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cas= e 32:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 PciIoDevice->PciBar[BarIndex].BarType =3D
> PciBarTypeMem32;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= /
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= / No break; here as this is an intentional fall
> through.
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= /
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cas= e 64:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 PciIoDevice->PciBar[BarIndex].BarTypeFixed =3D
> TRUE;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 switch (Pt= r->AddrSpaceGranularity) {
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cas= e 32:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 PciIoDevice->PciBar[BarIndex].BarType =3D
> PciBarTypePMem32;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= /
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= / No break; here as this is an intentional fall
> through.
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /= /
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cas= e 64:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 PciIoDevice->PciBar[BarIndex].BarTypeFixed =3D
> TRUE;
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 break;
>
> Agree, but the semicolon's placement is awkward. I propose
>
>=C2=A0 =C2=A0No break here, as this is an intentional fall through.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>
>
>
>

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#110896) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000adf69d06099c8978--