From: Ranbir Singh <Ranbir.Singh3@Dell.com>
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=4239
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>
---
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 = PciBarTypeMem32;
+ //
+ // No break; here as this is an intentional fall through.
+ //
case 64:
PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
break;
@@ -1440,6 +1443,9 @@ UpdatePciInfo (
switch (Ptr->AddrSpaceGranularity) {
case 32:
PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
+ //
+ // No break; here as this is an intentional fall through.
+ //
case 64:
PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
break;
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110812): https://edk2.groups.io/g/devel/message/110812
Mute This Topic: https://groups.io/mt/102438299/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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 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=4239
>
> 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>
> ---
> 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 = PciBarTypeMem32;
> + //
> + // No break; here as this is an intentional fall through.
> + //
> case 64:
> PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
> break;
> @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> switch (Ptr->AddrSpaceGranularity) {
> case 32:
> PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
> + //
> + // No break; here as this is an intentional fall through.
> + //
> case 64:
> PciIoDevice->PciBar[BarIndex].BarTypeFixed = 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 <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110863): https://edk2.groups.io/g/devel/message/110863
Mute This Topic: https://groups.io/mt/102438299/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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@edk2.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 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=4239
> >
> > 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>
> > ---
> > 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 =
> PciBarTypeMem32;
> > + //
> > + // No break; here as this is an intentional fall
> through.
> > + //
> > case 64:
> > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> > break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > switch (Ptr->AddrSpaceGranularity) {
> > case 32:
> > PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypePMem32;
> > + //
> > + // No break; here as this is an intentional fall
> through.
> > + //
> > case 64:
> > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> 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 <lersek@redhat.com>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110870): https://edk2.groups.io/g/devel/message/110870
Mute This Topic: https://groups.io/mt/102438299/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 PM 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 <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 <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 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=4239
> > >
> > > 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>
> > > ---
> > > 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 =
> > PciBarTypeMem32;
> > > + //
> > > + // No break; here as this is an intentional fall
> > through.
> > > + //
> > > case 64:
> > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > > break;
> > > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > > switch (Ptr->AddrSpaceGranularity) {
> > > case 32:
> > > PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypePMem32;
> > > + //
> > > + // No break; here as this is an intentional fall
> > through.
> > > + //
> > > case 64:
> > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > 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 <lersek@redhat.com>
> >
> >
> >
> >
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 maintain.
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 <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@dellteam.com>
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 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 PM Kinney, Michael D <michael.d.kinney@intel.com<mailto: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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com<mailto: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<mailto:Ranbir.Singh3@Dell.com>>
> >
> > 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=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> > ---
> > 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 =
> PciBarTypeMem32;
> > + //
> > + // No break; here as this is an intentional fall
> through.
> > + //
> > case 64:
> > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> > break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > switch (Ptr->AddrSpaceGranularity) {
> > case 32:
> > PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypePMem32;
> > + //
> > + // No break; here as this is an intentional fall
> through.
> > + //
> > case 64:
> > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> 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 <lersek@redhat.com<mailto:lersek@redhat.com>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110892): https://edk2.groups.io/g/devel/message/110892
Mute This Topic: https://groups.io/mt/102438299/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 AM Kinney, Michael D <michael.d.kinney@intel.com>
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 maintain.
>
>
>
> 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 <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@dellteam.com>
> *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 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 PM 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 <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 <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 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=4239
> > >
> > > 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>
> > > ---
> > > 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 =
> > PciBarTypeMem32;
> > > + //
> > > + // No break; here as this is an intentional fall
> > through.
> > > + //
> > > case 64:
> > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > > break;
> > > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > > switch (Ptr->AddrSpaceGranularity) {
> > > case 32:
> > > PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypePMem32;
> > > + //
> > > + // No break; here as this is an intentional fall
> > through.
> > > + //
> > > case 64:
> > > PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > 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 <lersek@redhat.com>
> >
> >
> >
> >
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 11/8/23 05:29, Ranbir Singh wrote: > 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. This makes me a bit sad. I was happy to see that a company seriously invested in cleaning up Coverity issue reports all over edk2. If you don't have the environment and/or the assignment to continue doing that, then I agree that further tweaking these patches is unjustifed. (Sorry, I didn't realize the background when I reviewed your patches.) New versions would effectively mean "untested" code (untested as in not tested with Coverity specifically). In particular because the purported warning fixes will require real edk2 review (and occasionally regression testing even), which doesn't look good if we can't even be sure that the change actually placates coverity. I guess we should upstream those of your patches that are fine right as they are, and drop the rest for now. (A pity!) Accordingly, if you think some of my review comments are not especially important in this light (i.e., whenever it is better to take the patch as is, than to drop an updated version due to untestability), then please do comment so explicitly! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111142): https://edk2.groups.io/g/devel/message/111142 Mute This Topic: https://groups.io/mt/102438299/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Ray Ni <ray.ni@intel.com>
Thanks,
Ray
________________________________
From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Tuesday, November 7, 2023 2:19 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; rsingh@ventanamicro.com <rsingh@ventanamicro.com>
Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Subject: [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
From: Ranbir Singh <Ranbir.Singh3@Dell.com>
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=4239
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>
---
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 = PciBarTypeMem32;
+ //
+ // No break; here as this is an intentional fall through.
+ //
case 64:
PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
break;
@@ -1440,6 +1443,9 @@ UpdatePciInfo (
switch (Ptr->AddrSpaceGranularity) {
case 32:
PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
+ //
+ // No break; here as this is an intentional fall through.
+ //
case 64:
PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
break;
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110821): https://edk2.groups.io/g/devel/message/110821
Mute This Topic: https://groups.io/mt/102438299/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.