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 A07E8D806CB for ; Wed, 8 Nov 2023 03:51:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=mNkBKkHHwxkTbj4LI2J7LFIYMlY9mZ+w4vQd6sp/BDc=; 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=1699415499; v=1; b=WD3eL4wsohjdvH9qd1tujUD6+PWT2cCH+87pGc/bvSGeu9DGVFd2REb77X7/dmHPzXHcZ7Lq 8vxOJlDEsgk5IACZN6h/B7d6WRwihNE0JVl36YCw84Usf+DL3UHU5IM7Gx6DJUcyZtJGGuy5xrj amL9t7ZWBizvzdGQWoQOcxL8= X-Received: by 127.0.0.2 with SMTP id vM2iYY7687511xo99UEU0Tvu; Tue, 07 Nov 2023 19:51:39 -0800 X-Received: from mail-pg1-f174.google.com (mail-pg1-f174.google.com [209.85.215.174]) by mx.groups.io with SMTP id smtpd.web10.6754.1699415498762048146 for ; Tue, 07 Nov 2023 19:51:38 -0800 X-Received: by mail-pg1-f174.google.com with SMTP id 41be03b00d2f7-5b980391d70so4803046a12.0 for ; Tue, 07 Nov 2023 19:51:38 -0800 (PST) X-Gm-Message-State: gZFC3v7vf5yBtdn1vqo8IupBx7686176AA= X-Google-Smtp-Source: AGHT+IG6XBZ8O8tAbtlRTE/bmsMoTXnRy2T4RyyhWE5EUclQG3aGv6Gdw4O4lkQXmGwVvjXVBkSvnr11HdNffUydVe4= X-Received: by 2002:a05:6a21:4886:b0:179:fbd6:95f1 with SMTP id av6-20020a056a21488600b00179fbd695f1mr1135907pzc.26.1699415498017; Tue, 07 Nov 2023 19:51:38 -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:21:24 +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="00000000000015dae306099c0134" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=WD3eL4ws; 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 --00000000000015dae306099c0134 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 fairly 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 through. + // 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 (#110890): https://edk2.groups.io/g/devel/message/110890 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- --00000000000015dae306099c0134 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
As mentioned in the commit message, the comment helps in m= aking it explicit=C2=A0and evident that the missing break is not a human mi= ss, but intentional.
Hence, the comment should be considered as being a= dded for the human code readers / developers.

So even if= some static analysis tool flags such an issue, it can be fairly easy now t= o ignore that on manual=C2=A0inspection. If 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 // No break here as this is an in= tentional 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 //
<= br>
Yes, there can be other solutions (which may or may not be wo= rth 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 &= lt;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 <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@ed= k2.groups.io; rsingh@ventanamicro.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/s= how_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 (#110890) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--00000000000015dae306099c0134--