* [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity
@ 2023-11-07 6:19 Ranbir Singh
2023-11-07 6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh
` (4 more replies)
0 siblings, 5 replies; 32+ messages in thread
From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw)
To: devel, rsingh
v1 -> v2:
- Rebased to latest master HEAD
- Updated Cc list
- Updated the commit message wrt MISSING_BREAK issues
- Removed the ASSERT wrt NULL_RETURNS issue
Ranbir Singh (5):
MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue
MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues
MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues
MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 73 +++++++++++---------
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 48 +++++++++----
MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 45 ++++++++++--
4 files changed, 113 insertions(+), 54 deletions(-)
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110810): https://edk2.groups.io/g/devel/message/110810
Mute This Topic: https://groups.io/mt/102438297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 32+ messages in thread* [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh @ 2023-11-07 6:19 ` Ranbir Singh 2023-11-07 16:21 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh ` (3 subsequent siblings) 4 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli From: Ranbir Singh <Ranbir.Singh3@Dell.com> The function PciHotPlugRequestNotify has two if blocks towards the end of function both containing return. Due to the parameter checks ensured at the beginning of the function, one of the two if blocks is bound to come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is redundant/deadcode. To fix it, either of line 2109 or 2112 can be deleted. 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/PciEnumerator.c | 1 - 1 file changed, 1 deletion(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c index 3f8c6e6da7dc..5b71e152f3ea 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify ( // // End for // - return EFI_SUCCESS; } return EFI_SUCCESS; -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110811): https://edk2.groups.io/g/devel/message/110811 Mute This Topic: https://groups.io/mt/102438298/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh @ 2023-11-07 16:21 ` Laszlo Ersek 2023-11-10 6:14 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-07 16:21 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli On 11/7/23 07:19, Ranbir Singh wrote: > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > The function PciHotPlugRequestNotify has two if blocks towards the end > of function both containing return. Due to the parameter checks ensured > at the beginning of the function, one of the two if blocks is bound to > come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is > redundant/deadcode. Agree with the analysis. > > To fix it, either of line 2109 or 2112 can be deleted. > > 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/PciEnumerator.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > index 3f8c6e6da7dc..5b71e152f3ea 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify ( > // > // End for > // > - return EFI_SUCCESS; > } > > return EFI_SUCCESS; Disagree with the fix. Given the checks on "Operation" at the top of the function, the condition (near the end of the function) if (Operation == EfiPciHotplugRequestRemove) { will always evaluate to TRUE. (Operation can be only Add or Remove, and if it is Add, then we don't reach this location.) Therefore, we should remove this condition, and *unnest* the dependent logic. As a result of *that*, you'll have return EFI_SUCCESS; return EFI_SUCCESS; at the end of the function, and *then* you should remove either one of them. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110862): https://edk2.groups.io/g/devel/message/110862 Mute This Topic: https://groups.io/mt/102438298/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue 2023-11-07 16:21 ` Laszlo Ersek @ 2023-11-10 6:14 ` Ranbir Singh 0 siblings, 0 replies; 32+ messages in thread From: Ranbir Singh @ 2023-11-10 6:14 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel, Ray Ni, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 2635 bytes --] I considered the least code change approach without considering any optimization which also works as such. However, yes, the duplicated /unnecessary subsequent checks can be removed. On Tue, Nov 7, 2023 at 9:51 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > The function PciHotPlugRequestNotify has two if blocks towards the end > > of function both containing return. Due to the parameter checks ensured > > at the beginning of the function, one of the two if blocks is bound to > > come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is > > redundant/deadcode. > > Agree with the analysis. > > > > > To fix it, either of line 2109 or 2112 can be deleted. > > > > 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/PciEnumerator.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > > index 3f8c6e6da7dc..5b71e152f3ea 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > > @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify ( > > // > > // End for > > // > > - return EFI_SUCCESS; > > } > > > > return EFI_SUCCESS; > > Disagree with the fix. > > Given the checks on "Operation" at the top of the function, the > condition (near the end of the function) > > if (Operation == EfiPciHotplugRequestRemove) { > > will always evaluate to TRUE. (Operation can be only Add or Remove, and > if it is Add, then we don't reach this location.) > > Therefore, we should remove this condition, and *unnest* the dependent > logic. > > As a result of *that*, you'll have > > return EFI_SUCCESS; > return EFI_SUCCESS; > > at the end of the function, and *then* you should remove either one of > them. > > Thanks > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111018): https://edk2.groups.io/g/devel/message/111018 Mute This Topic: https://groups.io/mt/102438298/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 3917 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh @ 2023-11-07 6:19 ` Ranbir Singh 2023-11-07 8:15 ` Ni, Ray 2023-11-07 16:23 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh ` (2 subsequent siblings) 4 siblings, 2 replies; 32+ messages in thread From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh @ 2023-11-07 8:15 ` Ni, Ray 2023-11-07 16:23 ` Laszlo Ersek 1 sibling, 0 replies; 32+ messages in thread From: Ni, Ray @ 2023-11-07 8:15 UTC (permalink / raw) To: Ranbir Singh, devel@edk2.groups.io; +Cc: Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 2902 bytes --] 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 6582 bytes --] ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh 2023-11-07 8:15 ` Ni, Ray @ 2023-11-07 16:23 ` Laszlo Ersek 2023-11-07 17:59 ` Michael D Kinney 1 sibling, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-07 16:23 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-07 16:23 ` Laszlo Ersek @ 2023-11-07 17:59 ` Michael D Kinney 2023-11-08 3:51 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Michael D Kinney @ 2023-11-07 17:59 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, rsingh@ventanamicro.com Cc: Ni, Ray, Veeresh Sangolli, Kinney, Michael D 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-07 17:59 ` Michael D Kinney @ 2023-11-08 3:51 ` Ranbir Singh 2023-11-08 4:05 ` Michael D Kinney 0 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-08 3:51 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 4464 bytes --] 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 6779 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-08 3:51 ` Ranbir Singh @ 2023-11-08 4:05 ` Michael D Kinney 2023-11-08 4:29 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Michael D Kinney @ 2023-11-08 4:05 UTC (permalink / raw) To: Ranbir Singh Cc: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, Veeresh Sangolli, Kinney, Michael D [-- Attachment #1: Type: text/plain, Size: 5460 bytes --] 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 11149 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-08 4:05 ` Michael D Kinney @ 2023-11-08 4:29 ` Ranbir Singh 2023-11-13 11:24 ` Laszlo Ersek 0 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-08 4:29 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, lersek@redhat.com, Ni, Ray, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 6517 bytes --] 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 10984 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues 2023-11-08 4:29 ` Ranbir Singh @ 2023-11-13 11:24 ` Laszlo Ersek 0 siblings, 0 replies; 32+ messages in thread From: Laszlo Ersek @ 2023-11-13 11:24 UTC (permalink / raw) To: Ranbir Singh, Kinney, Michael D Cc: devel@edk2.groups.io, Ni, Ray, Veeresh Sangolli 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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh @ 2023-11-07 6:19 ` Ranbir Singh 2023-11-07 16:41 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh 4 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli From: Ranbir Singh <Ranbir.Singh3@Dell.com> The function PciHostBridgeResourceAllocator is not making use of the generic approach as is used in one of the other function namely - DumpResourceMap. As a result, the following warnings can be seen as reported by Coverity e.g. (30) Event address_of: Taking address with "&IoBridge" yields a singleton pointer. (31) Event callee_ptr_arith: Passing "&IoBridge" to function "FindResourceNode" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. Hence, adopt the generic approach to fix the issues at relevant points. 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/PciLib.c | 37 ++++++++++++++++---- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c index 84fc0161a19c..71767d3793d4 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator ( UINT64 Mem64ResStatus; UINT64 PMem64ResStatus; UINT32 MaxOptionRomSize; + PCI_RESOURCE_NODE **ChildResources; + UINTN ChildResourceCount; PCI_RESOURCE_NODE *IoBridge; PCI_RESOURCE_NODE *Mem32Bridge; PCI_RESOURCE_NODE *PMem32Bridge; @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator ( // Create the entire system resource map from the information collected by // enumerator. Several resource tree was created // - FindResourceNode (RootBridgeDev, &IoPool, &IoBridge); - FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge); - FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge); - FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge); - FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge); - + ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL); + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); + ASSERT (ChildResources != NULL); + FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]); + IoBridge = ChildResources[0]; ASSERT (IoBridge != NULL); + + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL); + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); + ASSERT (ChildResources != NULL); + FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]); + Mem32Bridge = ChildResources[0]; ASSERT (Mem32Bridge != NULL); + + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL); + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); + ASSERT (ChildResources != NULL); + FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]); + PMem32Bridge = ChildResources[0]; ASSERT (PMem32Bridge != NULL); + + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL); + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); + ASSERT (ChildResources != NULL); + FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]); + Mem64Bridge = ChildResources[0]; ASSERT (Mem64Bridge != NULL); + + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL); + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); + ASSERT (ChildResources != NULL); + FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]); + PMem64Bridge = ChildResources[0]; ASSERT (PMem64Bridge != NULL); // -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110813): https://edk2.groups.io/g/devel/message/110813 Mute This Topic: https://groups.io/mt/102438300/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh @ 2023-11-07 16:41 ` Laszlo Ersek 2023-11-10 5:59 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-07 16:41 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli On 11/7/23 07:19, Ranbir Singh wrote: > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > The function PciHostBridgeResourceAllocator is not making use of the > generic approach as is used in one of the other function namely - > DumpResourceMap. As a result, the following warnings can be seen as > reported by Coverity e.g. > > (30) Event address_of: Taking address with "&IoBridge" yields a > singleton pointer. > (31) Event callee_ptr_arith: Passing "&IoBridge" to function > "FindResourceNode" which uses it as an array. This might corrupt > or misinterpret adjacent memory locations. > > Hence, adopt the generic approach to fix the issues at relevant points. > > 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/PciLib.c | 37 ++++++++++++++++---- > 1 file changed, 31 insertions(+), 6 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > index 84fc0161a19c..71767d3793d4 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator ( > UINT64 Mem64ResStatus; > UINT64 PMem64ResStatus; > UINT32 MaxOptionRomSize; > + PCI_RESOURCE_NODE **ChildResources; > + UINTN ChildResourceCount; > PCI_RESOURCE_NODE *IoBridge; > PCI_RESOURCE_NODE *Mem32Bridge; > PCI_RESOURCE_NODE *PMem32Bridge; > @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator ( > // Create the entire system resource map from the information collected by > // enumerator. Several resource tree was created > // > - FindResourceNode (RootBridgeDev, &IoPool, &IoBridge); > - FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge); > - FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge); > - FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge); > - FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge); > - > + ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL); > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); > + ASSERT (ChildResources != NULL); > + FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]); > + IoBridge = ChildResources[0]; > ASSERT (IoBridge != NULL); > + > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL); > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); > + ASSERT (ChildResources != NULL); > + FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]); > + Mem32Bridge = ChildResources[0]; > ASSERT (Mem32Bridge != NULL); > + > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL); > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); > + ASSERT (ChildResources != NULL); > + FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]); > + PMem32Bridge = ChildResources[0]; > ASSERT (PMem32Bridge != NULL); > + > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL); > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); > + ASSERT (ChildResources != NULL); > + FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]); > + Mem64Bridge = ChildResources[0]; > ASSERT (Mem64Bridge != NULL); > + > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL); > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount); > + ASSERT (ChildResources != NULL); > + FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]); > + PMem64Bridge = ChildResources[0]; > ASSERT (PMem64Bridge != NULL); > > // Sorry, but this is terrible. * First of all, Coverity is *wrong*. The C spec clearly states that, for the purposes of pointer arithmetic, a singleton object behaves identically to the first element of an array. So, immediately, the idea arises that we should just use PCI_RESOURCE_NODE *IoBridgeArray[1]; FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray) to shut up Coverity. * Unfortunately, I expect that would only create a different warning: a warning about potentially overflowing this single-element array. Which is in fact a deeper problem in FindResourceNode() -- it happily overwrites an array that is too small. * Finally, this generic approach is both ugly (lots of code duplication!), and worse, it allocates memory without proper error checking (ASSERT() is not error checking), and then it leaks ChildResources *multiple times*! I suggest the following, for solving all of these issues: - create a function called FindFirstResourceNode(). It should have the same function prototype as FindResourceNode(), except the last parameter should be mandatory, not OPTIONAL. - the internal logic should be the same, except we shouldn't be counting, the return value should be a BOOLEAN. If we find a match, then output the first match and return TRUE. Otherwise, set the output to NULL and return FALSE. - replace the FindResourceNode calls with FindFirstResourceNode calls - Those call sites are already followed by ASSERT()s, saying that all search attempts will succeed. If coverity is happy with them, that's good; otherwise, we'll have to find a solution for them as well. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110864): https://edk2.groups.io/g/devel/message/110864 Mute This Topic: https://groups.io/mt/102438300/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues 2023-11-07 16:41 ` Laszlo Ersek @ 2023-11-10 5:59 ` Ranbir Singh 0 siblings, 0 replies; 32+ messages in thread From: Ranbir Singh @ 2023-11-10 5:59 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel, Ray Ni, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 6889 bytes --] I was apprehensive about this from the very beginning. Anyway, for now I will be dropping this issue from the series. On Tue, Nov 7, 2023 at 10:11 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > The function PciHostBridgeResourceAllocator is not making use of the > > generic approach as is used in one of the other function namely - > > DumpResourceMap. As a result, the following warnings can be seen as > > reported by Coverity e.g. > > > > (30) Event address_of: Taking address with "&IoBridge" yields a > > singleton pointer. > > (31) Event callee_ptr_arith: Passing "&IoBridge" to function > > "FindResourceNode" which uses it as an array. This might corrupt > > or misinterpret adjacent memory locations. > > > > Hence, adopt the generic approach to fix the issues at relevant points. > > > > 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/PciLib.c | 37 ++++++++++++++++---- > > 1 file changed, 31 insertions(+), 6 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > index 84fc0161a19c..71767d3793d4 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator ( > > UINT64 Mem64ResStatus; > > UINT64 PMem64ResStatus; > > UINT32 MaxOptionRomSize; > > + PCI_RESOURCE_NODE **ChildResources; > > + UINTN ChildResourceCount; > > PCI_RESOURCE_NODE *IoBridge; > > PCI_RESOURCE_NODE *Mem32Bridge; > > PCI_RESOURCE_NODE *PMem32Bridge; > > @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator ( > > // Create the entire system resource map from the information > collected by > > // enumerator. Several resource tree was created > > // > > - FindResourceNode (RootBridgeDev, &IoPool, &IoBridge); > > - FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge); > > - FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge); > > - FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge); > > - FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge); > > - > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]); > > + IoBridge = ChildResources[0]; > > ASSERT (IoBridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]); > > + Mem32Bridge = ChildResources[0]; > > ASSERT (Mem32Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]); > > + PMem32Bridge = ChildResources[0]; > > ASSERT (PMem32Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]); > > + Mem64Bridge = ChildResources[0]; > > ASSERT (Mem64Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]); > > + PMem64Bridge = ChildResources[0]; > > ASSERT (PMem64Bridge != NULL); > > > > // > > Sorry, but this is terrible. > > * First of all, Coverity is *wrong*. The C spec clearly states that, for > the purposes of pointer arithmetic, a singleton object behaves > identically to the first element of an array. > > So, immediately, the idea arises that we should just use > > PCI_RESOURCE_NODE *IoBridgeArray[1]; > > FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray) > > to shut up Coverity. > > * Unfortunately, I expect that would only create a different warning: a > warning about potentially overflowing this single-element array. Which > is in fact a deeper problem in FindResourceNode() -- it happily > overwrites an array that is too small. > > * Finally, this generic approach is both ugly (lots of code > duplication!), and worse, it allocates memory without proper error > checking (ASSERT() is not error checking), and then it leaks > ChildResources *multiple times*! > > I suggest the following, for solving all of these issues: > > - create a function called FindFirstResourceNode(). It should have the > same function prototype as FindResourceNode(), except the last parameter > should be mandatory, not OPTIONAL. > > - the internal logic should be the same, except we shouldn't be > counting, the return value should be a BOOLEAN. If we find a match, then > output the first match and return TRUE. Otherwise, set the output to > NULL and return FALSE. > > - replace the FindResourceNode calls with FindFirstResourceNode calls > > - Those call sites are already followed by ASSERT()s, saying that all > search attempts will succeed. If coverity is happy with them, that's > good; otherwise, we'll have to find a solution for them as well. > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111016): https://edk2.groups.io/g/devel/message/111016 Mute This Topic: https://groups.io/mt/102438300/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 8824 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh ` (2 preceding siblings ...) 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh @ 2023-11-07 6:19 ` Ranbir Singh 2023-11-07 16:48 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh 4 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli From: Ranbir Singh <Ranbir.Singh3@Dell.com> The function StartPciDevices has a check ASSERT (RootBridge != NULL); but this comes into play only in DEBUG mode. In Release mode, there is no handling if the RootBridge value is NULL and the code proceeds to unconditionally dereference "RootBridge" which will lead to CRASH. Hence, for safety add NULL pointer checks always and return EFI_NOT_READY if RootBridge value is NULL which is one of the return values as mentioned in the function description header. 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/PciDeviceSupport.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c index 581e9075ad41..3de80d98370e 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c @@ -772,7 +772,10 @@ StartPciDevices ( LIST_ENTRY *CurrentLink; RootBridge = GetRootBridgeByHandle (Controller); - ASSERT (RootBridge != NULL); + if (RootBridge == NULL) { + return EFI_NOT_READY; + } + ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; CurrentLink = mPciDevicePool.ForwardLink; -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110814): https://edk2.groups.io/g/devel/message/110814 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh @ 2023-11-07 16:48 ` Laszlo Ersek 2023-11-10 6:11 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-07 16:48 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli On 11/7/23 07:19, Ranbir Singh wrote: > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > The function StartPciDevices has a check > > ASSERT (RootBridge != NULL); > > but this comes into play only in DEBUG mode. In Release mode, there > is no handling if the RootBridge value is NULL and the code proceeds > to unconditionally dereference "RootBridge" which will lead to CRASH. > > Hence, for safety add NULL pointer checks always and return > EFI_NOT_READY if RootBridge value is NULL which is one of the return > values as mentioned in the function description header. > > 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/PciDeviceSupport.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > index 581e9075ad41..3de80d98370e 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > @@ -772,7 +772,10 @@ StartPciDevices ( > LIST_ENTRY *CurrentLink; > > RootBridge = GetRootBridgeByHandle (Controller); > - ASSERT (RootBridge != NULL); > + if (RootBridge == NULL) { > + return EFI_NOT_READY; > + } > + > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > CurrentLink = mPciDevicePool.ForwardLink; I don't think this is a good fix. There is one call site, namely in PciBusDriverBindingStart(). That call site does not check the return value. (Of course /s) I think that this ASSERT() can indeed never fail. Therefore I suggest CpuDeadLoop() instead. If you insist that CpuDeadLoop() is "too risky" here, then the patch is acceptable, but then the StartPciDevices() call site in PciBusDriverBindingStart() must check the error properly: we must not install "gEfiPciEnumerationCompleteProtocolGuid", and the function must propagate the error outwards. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110865): https://edk2.groups.io/g/devel/message/110865 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-07 16:48 ` Laszlo Ersek @ 2023-11-10 6:11 ` Ranbir Singh 2023-11-13 11:28 ` Laszlo Ersek 0 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-10 6:11 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel, Ray Ni, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 3077 bytes --] EFI_NOT_READY was listed as one of the error return values in the function header of StartPciDevices(). So, I considered it here. Maybe we can go by a dual approach, including CpuDeadLoop() in StartPciDevices() as well as add return value check at the call site in PciBusDriverBindingStart(). On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > The function StartPciDevices has a check > > > > ASSERT (RootBridge != NULL); > > > > but this comes into play only in DEBUG mode. In Release mode, there > > is no handling if the RootBridge value is NULL and the code proceeds > > to unconditionally dereference "RootBridge" which will lead to CRASH. > > > > Hence, for safety add NULL pointer checks always and return > > EFI_NOT_READY if RootBridge value is NULL which is one of the return > > values as mentioned in the function description header. > > > > 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/PciDeviceSupport.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > index 581e9075ad41..3de80d98370e 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > @@ -772,7 +772,10 @@ StartPciDevices ( > > LIST_ENTRY *CurrentLink; > > > > RootBridge = GetRootBridgeByHandle (Controller); > > - ASSERT (RootBridge != NULL); > > + if (RootBridge == NULL) { > > + return EFI_NOT_READY; > > + } > > + > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > CurrentLink = mPciDevicePool.ForwardLink; > > I don't think this is a good fix. > > There is one call site, namely in PciBusDriverBindingStart(). That call > site does not check the return value. (Of course /s) > > I think that this ASSERT() can indeed never fail. Therefore I suggest > CpuDeadLoop() instead. > > If you insist that CpuDeadLoop() is "too risky" here, then the patch is > acceptable, but then the StartPciDevices() call site in > PciBusDriverBindingStart() must check the error properly: we must not > install "gEfiPciEnumerationCompleteProtocolGuid", and the function must > propagate the error outwards. > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111017): https://edk2.groups.io/g/devel/message/111017 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 4406 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-10 6:11 ` Ranbir Singh @ 2023-11-13 11:28 ` Laszlo Ersek 2023-11-14 15:08 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-13 11:28 UTC (permalink / raw) To: Ranbir Singh; +Cc: devel, Ray Ni, Veeresh Sangolli On 11/10/23 07:11, Ranbir Singh wrote: > EFI_NOT_READY was listed as one of the error return values in the > function header of StartPciDevices(). So, I considered it here. > > Maybe we can go by a dual approach, including CpuDeadLoop() in > StartPciDevices() as well as add return value check at the call site in > PciBusDriverBindingStart(). I don't think this makes much sense, given that execution is not supposed to continue past CpuDeadLoop(), so the new error check would not be reached. I think two options are realistic: - replace the assert() with a CpuDeadLoop() -- this is my preference - keep the assert, add the "if", and in the caller, handle the EFI_NOT_READY error. This is workable too. (Keeping the assert is goog because it shows that we don't expect the "if" to fire.) Anyway, now that we have no way to verify the change against Coverity, I don't know if this patch is worth the churn. :( As I said, this ASSERT() is one of those few justified ones in edk2 core that can indeed never fail, IMO. Laszlo > > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > The function StartPciDevices has a check > > > > ASSERT (RootBridge != NULL); > > > > but this comes into play only in DEBUG mode. In Release mode, there > > is no handling if the RootBridge value is NULL and the code proceeds > > to unconditionally dereference "RootBridge" which will lead to CRASH. > > > > Hence, for safety add NULL pointer checks always and return > > EFI_NOT_READY if RootBridge value is NULL which is one of the return > > values as mentioned in the function description header. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > <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> > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com > <mailto:rsingh@ventanamicro.com>> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > index 581e9075ad41..3de80d98370e 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > @@ -772,7 +772,10 @@ StartPciDevices ( > > LIST_ENTRY *CurrentLink; > > > > RootBridge = GetRootBridgeByHandle (Controller); > > - ASSERT (RootBridge != NULL); > > + if (RootBridge == NULL) { > > + return EFI_NOT_READY; > > + } > > + > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > CurrentLink = mPciDevicePool.ForwardLink; > > I don't think this is a good fix. > > There is one call site, namely in PciBusDriverBindingStart(). That call > site does not check the return value. (Of course /s) > > I think that this ASSERT() can indeed never fail. Therefore I suggest > CpuDeadLoop() instead. > > If you insist that CpuDeadLoop() is "too risky" here, then the patch is > acceptable, but then the StartPciDevices() call site in > PciBusDriverBindingStart() must check the error properly: we must not > install "gEfiPciEnumerationCompleteProtocolGuid", and the function must > propagate the error outwards. > > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111143): https://edk2.groups.io/g/devel/message/111143 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-13 11:28 ` Laszlo Ersek @ 2023-11-14 15:08 ` Ranbir Singh 2023-11-14 16:21 ` Michael D Kinney 0 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-14 15:08 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel, Ray Ni, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 5096 bytes --] On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/10/23 07:11, Ranbir Singh wrote: > > EFI_NOT_READY was listed as one of the error return values in the > > function header of StartPciDevices(). So, I considered it here. > > > > Maybe we can go by a dual approach, including CpuDeadLoop() in > > StartPciDevices() as well as add return value check at the call site in > > PciBusDriverBindingStart(). > > I don't think this makes much sense, given that execution is not > supposed to continue past CpuDeadLoop(), so the new error check would > not be reached. > > I think two options are realistic: > > - replace the assert() with a CpuDeadLoop() -- this is my preference > > - keep the assert, add the "if", and in the caller, handle the > EFI_NOT_READY error. This is workable too. (Keeping the assert is goog > because it shows that we don't expect the "if" to fire.) > > Anyway, now that we have no way to verify the change against Coverity, I > don't know if this patch is worth the churn. :( As I said, this ASSERT() > is one of those few justified ones in edk2 core that can indeed never > fail, IMO. > > Laszlo > > See, Does the following change look acceptable to you ? ASSERT (RootBridge != NULL); + if (RootBridge == NULL) { + CpuDeadLoop (); + return EFI_NOT_READY; + } + which retains the existing assert, adds the NULL pointer check and includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the return statement afterwards will never reach execution (=> no need to handle this return value at the call sites), but will satisfy static analysis tools as the "RootBridge" dereference scenario doesn't arise thereafter. > > > > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com > > <mailto:lersek@redhat.com>> wrote: > > > > On 11/7/23 07:19, Ranbir Singh wrote: > > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > > > The function StartPciDevices has a check > > > > > > ASSERT (RootBridge != NULL); > > > > > > but this comes into play only in DEBUG mode. In Release mode, there > > > is no handling if the RootBridge value is NULL and the code > proceeds > > > to unconditionally dereference "RootBridge" which will lead to > CRASH. > > > > > > Hence, for safety add NULL pointer checks always and return > > > EFI_NOT_READY if RootBridge value is NULL which is one of the > return > > > values as mentioned in the function description header. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > <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> > > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com > > <mailto:rsingh@ventanamicro.com>> > > > --- > > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > index 581e9075ad41..3de80d98370e 100644 > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > @@ -772,7 +772,10 @@ StartPciDevices ( > > > LIST_ENTRY *CurrentLink; > > > > > > RootBridge = GetRootBridgeByHandle (Controller); > > > - ASSERT (RootBridge != NULL); > > > + if (RootBridge == NULL) { > > > + return EFI_NOT_READY; > > > + } > > > + > > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > > > CurrentLink = mPciDevicePool.ForwardLink; > > > > I don't think this is a good fix. > > > > There is one call site, namely in PciBusDriverBindingStart(). That > call > > site does not check the return value. (Of course /s) > > > > I think that this ASSERT() can indeed never fail. Therefore I suggest > > CpuDeadLoop() instead. > > > > If you insist that CpuDeadLoop() is "too risky" here, then the patch > is > > acceptable, but then the StartPciDevices() call site in > > PciBusDriverBindingStart() must check the error properly: we must not > > install "gEfiPciEnumerationCompleteProtocolGuid", and the function > must > > propagate the error outwards. > > > > Laszlo > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111202): https://edk2.groups.io/g/devel/message/111202 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 7728 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 15:08 ` Ranbir Singh @ 2023-11-14 16:21 ` Michael D Kinney 2023-11-14 16:53 ` Ranbir Singh ` (2 more replies) 0 siblings, 3 replies; 32+ messages in thread From: Michael D Kinney @ 2023-11-14 16:21 UTC (permalink / raw) To: devel@edk2.groups.io, rsingh@ventanamicro.com, Laszlo Ersek, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki Cc: Ni, Ray, Veeresh Sangolli, Kinney, Michael D [-- Attachment #1: Type: text/plain, Size: 6674 bytes --] Hi Ranbir, First I want to recognize your efforts to collect Coverity issues and propose changes to address them. I still disagree with adding CpuDealLoop() for any static analysis issues. There have been previous discussions about adding a PANIC() or FATAL() macro that would perform platform specific actions if a condition is detected where the boot of the platform can not continue. A platform get to make the choice to log or reboot or hang, etc. Not the code that detected the condition. https://edk2.groups.io/g/devel/message/86597 Unfortunately, in order to fix some of these static analysis issues correctly, we are going to have to identify the use of ASSERT() that really is a fatal condition that can not continue and introduce an implementation approach that provides a platform handler and satisfies the static analysis tools. We also have to evaluate if a return error status with a DEBUG_ERROR message would be a better choice than an ASSERT() that can be filtered out by build options. Best regards, Mike From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir Singh Sent: Tuesday, November 14, 2023 7:08 AM To: Laszlo Ersek <lersek@redhat.com> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com> Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote: On 11/10/23 07:11, Ranbir Singh wrote: > EFI_NOT_READY was listed as one of the error return values in the > function header of StartPciDevices(). So, I considered it here. > > Maybe we can go by a dual approach, including CpuDeadLoop() in > StartPciDevices() as well as add return value check at the call site in > PciBusDriverBindingStart(). I don't think this makes much sense, given that execution is not supposed to continue past CpuDeadLoop(), so the new error check would not be reached. I think two options are realistic: - replace the assert() with a CpuDeadLoop() -- this is my preference - keep the assert, add the "if", and in the caller, handle the EFI_NOT_READY error. This is workable too. (Keeping the assert is goog because it shows that we don't expect the "if" to fire.) Anyway, now that we have no way to verify the change against Coverity, I don't know if this patch is worth the churn. :( As I said, this ASSERT() is one of those few justified ones in edk2 core that can indeed never fail, IMO. Laszlo See, Does the following change look acceptable to you ? ASSERT (RootBridge != NULL); + if (RootBridge == NULL) { + CpuDeadLoop (); + return EFI_NOT_READY; + } + which retains the existing assert, adds the NULL pointer check and includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the return statement afterwards will never reach execution (=> no need to handle this return value at the call sites), but will satisfy static analysis tools as the "RootBridge" dereference scenario doesn't arise thereafter. > > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com> > <mailto:lersek@redhat.com<mailto:lersek@redhat.com>>> wrote: > > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>> > > > > The function StartPciDevices has a check > > > > ASSERT (RootBridge != NULL); > > > > but this comes into play only in DEBUG mode. In Release mode, there > > is no handling if the RootBridge value is NULL and the code proceeds > > to unconditionally dereference "RootBridge" which will lead to CRASH. > > > > Hence, for safety add NULL pointer checks always and return > > EFI_NOT_READY if RootBridge value is NULL which is one of the return > > values as mentioned in the function description header. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239> > > > > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com<mailto:ray.ni@intel.com>>> > > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com> > <mailto: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> > <mailto:rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > index 581e9075ad41..3de80d98370e 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > @@ -772,7 +772,10 @@ StartPciDevices ( > > LIST_ENTRY *CurrentLink; > > > > RootBridge = GetRootBridgeByHandle (Controller); > > - ASSERT (RootBridge != NULL); > > + if (RootBridge == NULL) { > > + return EFI_NOT_READY; > > + } > > + > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > CurrentLink = mPciDevicePool.ForwardLink; > > I don't think this is a good fix. > > There is one call site, namely in PciBusDriverBindingStart(). That call > site does not check the return value. (Of course /s) > > I think that this ASSERT() can indeed never fail. Therefore I suggest > CpuDeadLoop() instead. > > If you insist that CpuDeadLoop() is "too risky" here, then the patch is > acceptable, but then the StartPciDevices() call site in > PciBusDriverBindingStart() must check the error properly: we must not > install "gEfiPciEnumerationCompleteProtocolGuid", and the function must > propagate the error outwards. > > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111207): https://edk2.groups.io/g/devel/message/111207 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 13770 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 16:21 ` Michael D Kinney @ 2023-11-14 16:53 ` Ranbir Singh 2023-11-15 10:02 ` Laszlo Ersek 2023-11-14 19:37 ` Michael Kubacki 2023-11-15 9:50 ` Laszlo Ersek 2 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-14 16:53 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, Laszlo Ersek, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki, Ni, Ray, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 7650 bytes --] On Tue, Nov 14, 2023 at 9:51 PM Kinney, Michael D < michael.d.kinney@intel.com> wrote: > Hi Ranbir, > > > > First I want to recognize your efforts to collect Coverity issues and > propose changes to address > > them. > Thanks Mike. > > > I still disagree with adding CpuDealLoop() for any static analysis issues. > A bit of correction here. CpuDeadLoop() is not exactly an addition for static analysis issues. For that, the NULL pointer check and return statement in the if block are sufficient. However, CpuDeadLoop() is added as suggested by Laszlo to introduce a hang behaviour in RELEASE builds as well when the situation is anyway not safe to progress normally. That said, it may not still be required at every point and hence needs to be assessed on a case to case basis. > > There have been previous discussions about adding a PANIC() or FATAL() > macro that would > > perform platform specific actions if a condition is detected where the > boot of the platform > > can not continue. A platform get to make the choice to log or reboot or > hang, etc. Not the > > code that detected the condition. > > > > https://edk2.groups.io/g/devel/message/86597 > > > > Unfortunately, in order to fix some of these static analysis issues > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal condition > that can not continue > > and introduce an implementation approach that provides a platform handler > and > > satisfies the static analysis tools. > > > > We also have to evaluate if a return error status with a DEBUG_ERROR > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > Note that the existing ASSERT will give a DEBUG message even when CpuDeadLoop is added in the code later. > > > Best regards, > > > > Mike > > > Generally speaking, there now seems to be different views coming from you and Laszlo. We might have to wait for some sort of agreement to be reached. > > > *From:* devel@edk2.groups.io <devel@edk2.groups.io> * On Behalf Of *Ranbir > Singh > *Sent:* Tuesday, November 14, 2023 7:08 AM > *To:* Laszlo Ersek <lersek@redhat.com> > *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli < > veeresh.sangolli@dellteam.com> > *Subject:* Re: [edk2-devel] [PATCH v2 4/5] > MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue > > > > > > > > On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote: > > On 11/10/23 07:11, Ranbir Singh wrote: > > EFI_NOT_READY was listed as one of the error return values in the > > function header of StartPciDevices(). So, I considered it here. > > > > Maybe we can go by a dual approach, including CpuDeadLoop() in > > StartPciDevices() as well as add return value check at the call site in > > PciBusDriverBindingStart(). > > I don't think this makes much sense, given that execution is not > supposed to continue past CpuDeadLoop(), so the new error check would > not be reached. > > I think two options are realistic: > > - replace the assert() with a CpuDeadLoop() -- this is my preference > > - keep the assert, add the "if", and in the caller, handle the > EFI_NOT_READY error. This is workable too. (Keeping the assert is goog > because it shows that we don't expect the "if" to fire.) > > Anyway, now that we have no way to verify the change against Coverity, I > don't know if this patch is worth the churn. :( As I said, this ASSERT() > is one of those few justified ones in edk2 core that can indeed never > fail, IMO. > > Laszlo > > > > See, Does the following change look acceptable to you ? > > > > ASSERT (RootBridge != NULL); > + if (RootBridge == NULL) { > > + CpuDeadLoop (); > + return EFI_NOT_READY; > + } > > + > > > > which retains the existing assert, adds the NULL pointer check and > includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), > the return statement afterwards will never reach execution (=> no need to > handle this return value at the call sites), but will satisfy static > analysis tools as the "RootBridge" dereference scenario doesn't arise > thereafter. > > > > > > > > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com > > <mailto:lersek@redhat.com>> wrote: > > > > On 11/7/23 07:19, Ranbir Singh wrote: > > > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > > > > > The function StartPciDevices has a check > > > > > > ASSERT (RootBridge != NULL); > > > > > > but this comes into play only in DEBUG mode. In Release mode, there > > > is no handling if the RootBridge value is NULL and the code > proceeds > > > to unconditionally dereference "RootBridge" which will lead to > CRASH. > > > > > > Hence, for safety add NULL pointer checks always and return > > > EFI_NOT_READY if RootBridge value is NULL which is one of the > return > > > values as mentioned in the function description header. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > <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> > > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com > > <mailto:rsingh@ventanamicro.com>> > > > --- > > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > index 581e9075ad41..3de80d98370e 100644 > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > @@ -772,7 +772,10 @@ StartPciDevices ( > > > LIST_ENTRY *CurrentLink; > > > > > > RootBridge = GetRootBridgeByHandle (Controller); > > > - ASSERT (RootBridge != NULL); > > > + if (RootBridge == NULL) { > > > + return EFI_NOT_READY; > > > + } > > > + > > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > > > CurrentLink = mPciDevicePool.ForwardLink; > > > > I don't think this is a good fix. > > > > There is one call site, namely in PciBusDriverBindingStart(). That > call > > site does not check the return value. (Of course /s) > > > > I think that this ASSERT() can indeed never fail. Therefore I suggest > > CpuDeadLoop() instead. > > > > If you insist that CpuDeadLoop() is "too risky" here, then the patch > is > > acceptable, but then the StartPciDevices() call site in > > PciBusDriverBindingStart() must check the error properly: we must not > > install "gEfiPciEnumerationCompleteProtocolGuid", and the function > must > > propagate the error outwards. > > > > Laszlo > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111209): https://edk2.groups.io/g/devel/message/111209 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 14653 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 16:53 ` Ranbir Singh @ 2023-11-15 10:02 ` Laszlo Ersek 0 siblings, 0 replies; 32+ messages in thread From: Laszlo Ersek @ 2023-11-15 10:02 UTC (permalink / raw) To: Ranbir Singh, Kinney, Michael D Cc: devel@edk2.groups.io, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki, Ni, Ray, Veeresh Sangolli On 11/14/23 17:53, Ranbir Singh wrote: > Generally speaking, there now seems to be different views coming from > you and Laszlo. Yes. > We might have to wait for some sort of agreement to be > reached. I don't insist on CpuDeadLoop() *specifically*. Only the following two generic points matter to me: (1) Stop abusing ASSERT (both because it is compiled out of RELEASE builds, and because it is conceptually unsuitable for catching data- and environment-dependent error conditions). ASSERT must only be used for stating (well, "asserting") algorithmic invariants. (2) Upon detecting an algorithmic invariant failure, call *some* API that, at the same time: (2.a) prevents execution from continuing, (2.b) *cannot* be removed from RELEASE builds, (2.c) informs all static analysis tools we use that execution cannot continue past that point. For (2), Mike seems to have an additional requirement: (2.d) make the implementation customizable by the platform, including any information shown to, or logged for, the user (or supervisor software). I have nothing against that additional requirement. My concern is that "perfect" is going to get in the way of "good enough" once again. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111248): https://edk2.groups.io/g/devel/message/111248 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 16:21 ` Michael D Kinney 2023-11-14 16:53 ` Ranbir Singh @ 2023-11-14 19:37 ` Michael Kubacki 2023-11-15 10:10 ` Laszlo Ersek 2023-11-15 9:50 ` Laszlo Ersek 2 siblings, 1 reply; 32+ messages in thread From: Michael Kubacki @ 2023-11-14 19:37 UTC (permalink / raw) To: devel, michael.d.kinney, rsingh@ventanamicro.com, Laszlo Ersek, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com Cc: Ni, Ray, Veeresh Sangolli On 11/14/2023 11:21 AM, Michael D Kinney wrote: > Hi Ranbir, > > First I want to recognize your efforts to collect Coverity issues and > propose changes to address > > them. > > I still disagree with adding CpuDealLoop() for any static analysis issues. > > There have been previous discussions about adding a PANIC() or FATAL() > macro that would > > perform platform specific actions if a condition is detected where the > boot of the platform > > can not continue. A platform get to make the choice to log or reboot or > hang, etc. Not the > > code that detected the condition. > > https://edk2.groups.io/g/devel/message/86597 > <https://edk2.groups.io/g/devel/message/86597> > After going through hundreds of edk2 static analysis findings, we found a small number of cases where an interface such as PanicLib was useful and recently added an implementation https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h. For example, the return value from calls to MpInitLibWhoAmI() in exception related code often goes unchecked and it's been used there. Being able to separate the library instance implementation linked to a given module from a more broad library class like DebugLib for these cases has also been helpful. > Unfortunately, in order to fix some of these static analysis issues > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal condition > that can not continue > > and introduce an implementation approach that provides a platform > handler and > > satisfies the static analysis tools. > > We also have to evaluate if a return error status with a DEBUG_ERROR > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > > Best regards, > > Mike > > *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of > *Ranbir Singh > *Sent:* Tuesday, November 14, 2023 7:08 AM > *To:* Laszlo Ersek <lersek@redhat.com> > *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli > <veeresh.sangolli@dellteam.com> > *Subject:* Re: [edk2-devel] [PATCH v2 4/5] > MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue > > On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > > On 11/10/23 07:11, Ranbir Singh wrote: > > EFI_NOT_READY was listed as one of the error return values in the > > function header of StartPciDevices(). So, I considered it here. > > > > Maybe we can go by a dual approach, including CpuDeadLoop() in > > StartPciDevices() as well as add return value check at the call > site in > > PciBusDriverBindingStart(). > > I don't think this makes much sense, given that execution is not > supposed to continue past CpuDeadLoop(), so the new error check would > not be reached. > > I think two options are realistic: > > - replace the assert() with a CpuDeadLoop() -- this is my preference > > - keep the assert, add the "if", and in the caller, handle the > EFI_NOT_READY error. This is workable too. (Keeping the assert is goog > because it shows that we don't expect the "if" to fire.) > > Anyway, now that we have no way to verify the change against Coverity, I > don't know if this patch is worth the churn. :( As I said, this ASSERT() > is one of those few justified ones in edk2 core that can indeed never > fail, IMO. > > Laszlo > > See, Does the following change look acceptable to you ? > > ASSERT (RootBridge != NULL); > + if (RootBridge == NULL) { > > + CpuDeadLoop (); > + return EFI_NOT_READY; > + } > > + > > which retains the existing assert, adds the NULL pointer check and > includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop > (), the return statement afterwards will never reach execution (=> no > need to handle this return value at the call sites), but will satisfy > static analysis tools as the "RootBridge" dereference scenario doesn't > arise thereafter. > > > > > > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com> > > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: > > > > On 11/7/23 07:19, Ranbir Singh wrote: > > > From: Ranbir Singh <Ranbir.Singh3@Dell.com > <mailto:Ranbir.Singh3@Dell.com>> > > > > > > The function StartPciDevices has a check > > > > > > ASSERT (RootBridge != NULL); > > > > > > but this comes into play only in DEBUG mode. In Release > mode, there > > > is no handling if the RootBridge value is NULL and the code > proceeds > > > to unconditionally dereference "RootBridge" which will lead > to CRASH. > > > > > > Hence, for safety add NULL pointer checks always and return > > > EFI_NOT_READY if RootBridge value is NULL which is one of > the return > > > values as mentioned in the function description header. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239> > > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>> > > > > > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> > <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>> > > > Co-authored-by: Veeresh Sangolli > <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com> > > <mailto: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> > > <mailto:rsingh@ventanamicro.com > <mailto:rsingh@ventanamicro.com>>> > > > --- > > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > index 581e9075ad41..3de80d98370e 100644 > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > > @@ -772,7 +772,10 @@ StartPciDevices ( > > > LIST_ENTRY *CurrentLink; > > > > > > RootBridge = GetRootBridgeByHandle (Controller); > > > - ASSERT (RootBridge != NULL); > > > + if (RootBridge == NULL) { > > > + return EFI_NOT_READY; > > > + } > > > + > > > ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle; > > > > > > CurrentLink = mPciDevicePool.ForwardLink; > > > > I don't think this is a good fix. > > > > There is one call site, namely in PciBusDriverBindingStart(). > That call > > site does not check the return value. (Of course /s) > > > > I think that this ASSERT() can indeed never fail. Therefore I > suggest > > CpuDeadLoop() instead. > > > > If you insist that CpuDeadLoop() is "too risky" here, then > the patch is > > acceptable, but then the StartPciDevices() call site in > > PciBusDriverBindingStart() must check the error properly: we > must not > > install "gEfiPciEnumerationCompleteProtocolGuid", and the > function must > > propagate the error outwards. > > > > Laszlo > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111214): https://edk2.groups.io/g/devel/message/111214 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 19:37 ` Michael Kubacki @ 2023-11-15 10:10 ` Laszlo Ersek 2023-11-20 14:05 ` Michael Kubacki 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-15 10:10 UTC (permalink / raw) To: Michael Kubacki, devel, michael.d.kinney, rsingh@ventanamicro.com, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com Cc: Ni, Ray, Veeresh Sangolli On 11/14/23 20:37, Michael Kubacki wrote: > On 11/14/2023 11:21 AM, Michael D Kinney wrote: >> Hi Ranbir, >> >> First I want to recognize your efforts to collect Coverity issues and >> propose changes to address >> >> them. >> >> I still disagree with adding CpuDealLoop() for any static analysis >> issues. >> >> There have been previous discussions about adding a PANIC() or FATAL() >> macro that would >> >> perform platform specific actions if a condition is detected where the >> boot of the platform >> >> can not continue. A platform get to make the choice to log or reboot >> or hang, etc. Not the >> >> code that detected the condition. >> >> https://edk2.groups.io/g/devel/message/86597 >> <https://edk2.groups.io/g/devel/message/86597> >> > After going through hundreds of edk2 static analysis findings, we found > a small number of cases where an interface such as PanicLib was useful > and recently added an implementation > https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h. > > For example, the return value from calls to MpInitLibWhoAmI() in > exception related code often goes unchecked and it's been used there. > Being able to separate the library instance implementation linked to a > given module from a more broad library class like DebugLib for these > cases has also been helpful. Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally forgotten about that; my apologies. ... I initially thought that a plain "CONST CHAR8 *Description" was not too flexible, but on second thought, it should be exactly right. Reason being, it's very easy to print. Format specifiers and variable arguments (PrintLib style) may be too complex to implement safely within PanicReport(). Arguably, no PanicReport() implementation should be obligated (by the interface) to depend on, or to reimplement, PrintLib. If the calling context permits, the caller can just use PrintLib to format the message to a local buffer (on the stack), and pass that to PanicReport. So this looks very useful to me; can you upstream it? Laszlo > >> Unfortunately, in order to fix some of these static analysis issues >> correctly, we are going >> >> to have to identify the use of ASSERT() that really is a fatal >> condition that can not continue >> >> and introduce an implementation approach that provides a platform >> handler and >> >> satisfies the static analysis tools. >> >> We also have to evaluate if a return error status with a DEBUG_ERROR >> message would be a better >> >> choice than an ASSERT() that can be filtered out by build options. >> >> Best regards, >> >> Mike >> >> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of >> *Ranbir Singh >> *Sent:* Tuesday, November 14, 2023 7:08 AM >> *To:* Laszlo Ersek <lersek@redhat.com> >> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh >> Sangolli <veeresh.sangolli@dellteam.com> >> *Subject:* Re: [edk2-devel] [PATCH v2 4/5] >> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue >> >> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com >> <mailto:lersek@redhat.com>> wrote: >> >> On 11/10/23 07:11, Ranbir Singh wrote: >> > EFI_NOT_READY was listed as one of the error return values in the >> > function header of StartPciDevices(). So, I considered it here. >> > >> > Maybe we can go by a dual approach, including CpuDeadLoop() in >> > StartPciDevices() as well as add return value check at the call >> site in >> > PciBusDriverBindingStart(). >> >> I don't think this makes much sense, given that execution is not >> supposed to continue past CpuDeadLoop(), so the new error check would >> not be reached. >> >> I think two options are realistic: >> >> - replace the assert() with a CpuDeadLoop() -- this is my preference >> >> - keep the assert, add the "if", and in the caller, handle the >> EFI_NOT_READY error. This is workable too. (Keeping the assert is >> goog >> because it shows that we don't expect the "if" to fire.) >> >> Anyway, now that we have no way to verify the change against >> Coverity, I >> don't know if this patch is worth the churn. :( As I said, this >> ASSERT() >> is one of those few justified ones in edk2 core that can indeed never >> fail, IMO. >> >> Laszlo >> >> See, Does the following change look acceptable to you ? >> >> ASSERT (RootBridge != NULL); >> + if (RootBridge == NULL) { >> >> + CpuDeadLoop (); >> + return EFI_NOT_READY; >> + } >> >> + >> >> which retains the existing assert, adds the NULL pointer check and >> includes CpuDeadLoop () within the if block. As a result of >> CpuDeadLoop (), the return statement afterwards will never reach >> execution (=> no need to handle this return value at the call sites), >> but will satisfy static analysis tools as the "RootBridge" dereference >> scenario doesn't arise thereafter. >> >> >> > >> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com >> <mailto:lersek@redhat.com> >> > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: >> > >> > On 11/7/23 07:19, Ranbir Singh wrote: >> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com >> <mailto:Ranbir.Singh3@Dell.com>> >> > > >> > > The function StartPciDevices has a check >> > > >> > > ASSERT (RootBridge != NULL); >> > > >> > > but this comes into play only in DEBUG mode. In Release >> mode, there >> > > is no handling if the RootBridge value is NULL and the code >> proceeds >> > > to unconditionally dereference "RootBridge" which will lead >> to CRASH. >> > > >> > > Hence, for safety add NULL pointer checks always and return >> > > EFI_NOT_READY if RootBridge value is NULL which is one of >> the return >> > > values as mentioned in the function description header. >> > > >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 >> <https://bugzilla.tianocore.org/show_bug.cgi?id=4239> >> > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239 >> <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>> >> > > >> > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> >> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>> >> > > Co-authored-by: Veeresh Sangolli >> <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com> >> > <mailto: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> >> > <mailto:rsingh@ventanamicro.com >> <mailto:rsingh@ventanamicro.com>>> >> > > --- >> > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- >> > > 1 file changed, 4 insertions(+), 1 deletion(-) >> > > >> > > diff --git >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >> > > index 581e9075ad41..3de80d98370e 100644 >> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >> > > @@ -772,7 +772,10 @@ StartPciDevices ( >> > > LIST_ENTRY *CurrentLink; >> > > >> > > RootBridge = GetRootBridgeByHandle (Controller); >> > > - ASSERT (RootBridge != NULL); >> > > + if (RootBridge == NULL) { >> > > + return EFI_NOT_READY; >> > > + } >> > > + >> > > ThisHostBridge = >> RootBridge->PciRootBridgeIo->ParentHandle; >> > > >> > > CurrentLink = mPciDevicePool.ForwardLink; >> > >> > I don't think this is a good fix. >> > >> > There is one call site, namely in PciBusDriverBindingStart(). >> That call >> > site does not check the return value. (Of course /s) >> > >> > I think that this ASSERT() can indeed never fail. Therefore I >> suggest >> > CpuDeadLoop() instead. >> > >> > If you insist that CpuDeadLoop() is "too risky" here, then >> the patch is >> > acceptable, but then the StartPciDevices() call site in >> > PciBusDriverBindingStart() must check the error properly: we >> must not >> > install "gEfiPciEnumerationCompleteProtocolGuid", and the >> function must >> > propagate the error outwards. >> > >> > Laszlo >> > >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111249): https://edk2.groups.io/g/devel/message/111249 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-15 10:10 ` Laszlo Ersek @ 2023-11-20 14:05 ` Michael Kubacki 0 siblings, 0 replies; 32+ messages in thread From: Michael Kubacki @ 2023-11-20 14:05 UTC (permalink / raw) To: devel, lersek, michael.d.kinney, rsingh@ventanamicro.com, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com Cc: Ni, Ray, Veeresh Sangolli On 11/15/2023 5:10 AM, Laszlo Ersek wrote: > On 11/14/23 20:37, Michael Kubacki wrote: >> On 11/14/2023 11:21 AM, Michael D Kinney wrote: >>> Hi Ranbir, >>> >>> First I want to recognize your efforts to collect Coverity issues and >>> propose changes to address >>> >>> them. >>> >>> I still disagree with adding CpuDealLoop() for any static analysis >>> issues. >>> >>> There have been previous discussions about adding a PANIC() or FATAL() >>> macro that would >>> >>> perform platform specific actions if a condition is detected where the >>> boot of the platform >>> >>> can not continue. A platform get to make the choice to log or reboot >>> or hang, etc. Not the >>> >>> code that detected the condition. >>> >>> https://edk2.groups.io/g/devel/message/86597 >>> <https://edk2.groups.io/g/devel/message/86597> >>> >> After going through hundreds of edk2 static analysis findings, we found >> a small number of cases where an interface such as PanicLib was useful >> and recently added an implementation >> https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h. >> >> For example, the return value from calls to MpInitLibWhoAmI() in >> exception related code often goes unchecked and it's been used there. >> Being able to separate the library instance implementation linked to a >> given module from a more broad library class like DebugLib for these >> cases has also been helpful. > > Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally > forgotten about that; my apologies. > > ... I initially thought that a plain "CONST CHAR8 *Description" was not > too flexible, but on second thought, it should be exactly right. Reason > being, it's very easy to print. Format specifiers and variable arguments > (PrintLib style) may be too complex to implement safely within > PanicReport(). Arguably, no PanicReport() implementation should be > obligated (by the interface) to depend on, or to reimplement, PrintLib. > If the calling context permits, the caller can just use PrintLib to > format the message to a local buffer (on the stack), and pass that to > PanicReport. > > So this looks very useful to me; can you upstream it? > Thanks for taking a look. We definitely want to align with edk2 on a consistent way to handle these scenarios. We'll send a patch for further discussion and review. We've waited to do so to allow the original author (Ken Lautner) to return from his time out of office to send that patch. - Michael > Laszlo > > >> >>> Unfortunately, in order to fix some of these static analysis issues >>> correctly, we are going >>> >>> to have to identify the use of ASSERT() that really is a fatal >>> condition that can not continue >>> >>> and introduce an implementation approach that provides a platform >>> handler and >>> >>> satisfies the static analysis tools. >>> >>> We also have to evaluate if a return error status with a DEBUG_ERROR >>> message would be a better >>> >>> choice than an ASSERT() that can be filtered out by build options. >>> >>> Best regards, >>> >>> Mike >>> >>> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of >>> *Ranbir Singh >>> *Sent:* Tuesday, November 14, 2023 7:08 AM >>> *To:* Laszlo Ersek <lersek@redhat.com> >>> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh >>> Sangolli <veeresh.sangolli@dellteam.com> >>> *Subject:* Re: [edk2-devel] [PATCH v2 4/5] >>> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue >>> >>> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com >>> <mailto:lersek@redhat.com>> wrote: >>> >>> On 11/10/23 07:11, Ranbir Singh wrote: >>> > EFI_NOT_READY was listed as one of the error return values in the >>> > function header of StartPciDevices(). So, I considered it here. >>> > >>> > Maybe we can go by a dual approach, including CpuDeadLoop() in >>> > StartPciDevices() as well as add return value check at the call >>> site in >>> > PciBusDriverBindingStart(). >>> >>> I don't think this makes much sense, given that execution is not >>> supposed to continue past CpuDeadLoop(), so the new error check would >>> not be reached. >>> >>> I think two options are realistic: >>> >>> - replace the assert() with a CpuDeadLoop() -- this is my preference >>> >>> - keep the assert, add the "if", and in the caller, handle the >>> EFI_NOT_READY error. This is workable too. (Keeping the assert is >>> goog >>> because it shows that we don't expect the "if" to fire.) >>> >>> Anyway, now that we have no way to verify the change against >>> Coverity, I >>> don't know if this patch is worth the churn. :( As I said, this >>> ASSERT() >>> is one of those few justified ones in edk2 core that can indeed never >>> fail, IMO. >>> >>> Laszlo >>> >>> See, Does the following change look acceptable to you ? >>> >>> ASSERT (RootBridge != NULL); >>> + if (RootBridge == NULL) { >>> >>> + CpuDeadLoop (); >>> + return EFI_NOT_READY; >>> + } >>> >>> + >>> >>> which retains the existing assert, adds the NULL pointer check and >>> includes CpuDeadLoop () within the if block. As a result of >>> CpuDeadLoop (), the return statement afterwards will never reach >>> execution (=> no need to handle this return value at the call sites), >>> but will satisfy static analysis tools as the "RootBridge" dereference >>> scenario doesn't arise thereafter. >>> >>> >>> > >>> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com >>> <mailto:lersek@redhat.com> >>> > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote: >>> > >>> > On 11/7/23 07:19, Ranbir Singh wrote: >>> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com >>> <mailto:Ranbir.Singh3@Dell.com>> >>> > > >>> > > The function StartPciDevices has a check >>> > > >>> > > ASSERT (RootBridge != NULL); >>> > > >>> > > but this comes into play only in DEBUG mode. In Release >>> mode, there >>> > > is no handling if the RootBridge value is NULL and the code >>> proceeds >>> > > to unconditionally dereference "RootBridge" which will lead >>> to CRASH. >>> > > >>> > > Hence, for safety add NULL pointer checks always and return >>> > > EFI_NOT_READY if RootBridge value is NULL which is one of >>> the return >>> > > values as mentioned in the function description header. >>> > > >>> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=4239> >>> > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239 >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>> >>> > > >>> > > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> >>> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>> >>> > > Co-authored-by: Veeresh Sangolli >>> <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com> >>> > <mailto: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> >>> > <mailto:rsingh@ventanamicro.com >>> <mailto:rsingh@ventanamicro.com>>> >>> > > --- >>> > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++- >>> > > 1 file changed, 4 insertions(+), 1 deletion(-) >>> > > >>> > > diff --git >>> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >>> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >>> > > index 581e9075ad41..3de80d98370e 100644 >>> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >>> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c >>> > > @@ -772,7 +772,10 @@ StartPciDevices ( >>> > > LIST_ENTRY *CurrentLink; >>> > > >>> > > RootBridge = GetRootBridgeByHandle (Controller); >>> > > - ASSERT (RootBridge != NULL); >>> > > + if (RootBridge == NULL) { >>> > > + return EFI_NOT_READY; >>> > > + } >>> > > + >>> > > ThisHostBridge = >>> RootBridge->PciRootBridgeIo->ParentHandle; >>> > > >>> > > CurrentLink = mPciDevicePool.ForwardLink; >>> > >>> > I don't think this is a good fix. >>> > >>> > There is one call site, namely in PciBusDriverBindingStart(). >>> That call >>> > site does not check the return value. (Of course /s) >>> > >>> > I think that this ASSERT() can indeed never fail. Therefore I >>> suggest >>> > CpuDeadLoop() instead. >>> > >>> > If you insist that CpuDeadLoop() is "too risky" here, then >>> the patch is >>> > acceptable, but then the StartPciDevices() call site in >>> > PciBusDriverBindingStart() must check the error properly: we >>> must not >>> > install "gEfiPciEnumerationCompleteProtocolGuid", and the >>> function must >>> > propagate the error outwards. >>> > >>> > Laszlo >>> > >>> >>> >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111482): https://edk2.groups.io/g/devel/message/111482 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-14 16:21 ` Michael D Kinney 2023-11-14 16:53 ` Ranbir Singh 2023-11-14 19:37 ` Michael Kubacki @ 2023-11-15 9:50 ` Laszlo Ersek 2023-11-20 3:57 ` Ranbir Singh 2 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-15 9:50 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, rsingh@ventanamicro.com, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki Cc: Ni, Ray, Veeresh Sangolli On 11/14/23 17:21, Kinney, Michael D wrote: > Hi Ranbir, > > > > First I want to recognize your efforts to collect Coverity issues and > propose changes to address > them. > > I still disagree with adding CpuDealLoop() for any static analysis issues. > > There have been previous discussions about adding a PANIC() or FATAL() > macro that would > perform platform specific actions if a condition is detected where the > boot of the platform > can not continue. A platform get to make the choice to log or reboot or > hang, etc. Not the > code that detected the condition. > > https://edk2.groups.io/g/devel/message/86597 > <https://edk2.groups.io/g/devel/message/86597> This is indeed a great idea. I didn't know about that discussion. Perhaps a new DebugLib API would handle this (i.e., "panic"). I've been certainly proposing CpuDeadLoop() as a means to panic. Did the thread conclude anything tangible? > Unfortunately, in order to fix some of these static analysis issues > correctly, we are going > to have to identify the use of ASSERT() that really is a fatal condition > that can not continue Absolutely. > and introduce an implementation approach that provides a platform > handler and > satisfies the static analysis tools. The "platform handler" is the difficult part. If the above-noted discussion from 2022 didn't produce an agreement, should we really block the static analyzer fixes on an agreed-upon panic API? I'm concerned that would just cause these fixes to get stuck. And I don't consider CpuDeadLoop()s added for this purpose serious technical debt. They are easy to find and update later, assuming we also add comments. > We also have to evaluate if a return error status with a DEBUG_ERROR > message would be a better > choice than an ASSERT() that can be filtered out by build options. I agree 100% that this would be better for the codebase, but the work needed for this is (IMO) impossible to contain. ASSERT() has been abused for a long time *because* it seemed to allow the programmer to ignore any related error paths. If we now want to implement those error paths retroactively (which would be absolutely the right thing to do from a software engineering perspective), then immense amounts of work are going to be needed (patch review and regression testing), and I think it's just not practical to dump all that on someone that wants to improve the status quo. Replacing an invalid ASSERT() with a panic is honest about the current situation, is safer regarding RELEASE builds, and its work demand (regression testing, patch review) is tolerable. I do agree that, if the error path mostly exists already, then returning errors for data/environment-based error conditions (i.e., not for algorithmic invariant failures) is best. Where we need to be extremely vigilant is new patches. We must uncompromisingly reject *new* abuses of ASSERT(), in new patches. Anyway, it seems that we've been trying to steer Ranbir in opposite directions. I'll let you take the lead on this; for one, I've not been aware of the panic api discussion for 2022! (I don't feel especially pushy about fixing coverity issues, it's just that Ranbir has been contributing such patches, which I've found very welcome, and I wanted to help out with reviews.) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111247): https://edk2.groups.io/g/devel/message/111247 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-15 9:50 ` Laszlo Ersek @ 2023-11-20 3:57 ` Ranbir Singh 2023-11-21 17:07 ` Laszlo Ersek 0 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-20 3:57 UTC (permalink / raw) To: Laszlo Ersek Cc: Kinney, Michael D, devel@edk2.groups.io, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki, Ni, Ray, Veeresh Sangolli [-- Attachment #1: Type: text/plain, Size: 4405 bytes --] On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/14/23 17:21, Kinney, Michael D wrote: > > Hi Ranbir, > > > > > > > > First I want to recognize your efforts to collect Coverity issues and > > propose changes to address > > them. > > > > I still disagree with adding CpuDealLoop() for any static analysis > issues. > > > > There have been previous discussions about adding a PANIC() or FATAL() > > macro that would > > perform platform specific actions if a condition is detected where the > > boot of the platform > > can not continue. A platform get to make the choice to log or reboot or > > hang, etc. Not the > > code that detected the condition. > > > > https://edk2.groups.io/g/devel/message/86597 > > <https://edk2.groups.io/g/devel/message/86597> > > This is indeed a great idea. > > I didn't know about that discussion. Perhaps a new DebugLib API would > handle this (i.e., "panic"). > > I've been certainly proposing CpuDeadLoop() as a means to panic. > > Did the thread conclude anything tangible? > > > Unfortunately, in order to fix some of these static analysis issues > > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal condition > > that can not continue > > Absolutely. > > > and introduce an implementation approach that provides a platform > > handler and > > satisfies the static analysis tools. > > The "platform handler" is the difficult part. If the above-noted > discussion from 2022 didn't produce an agreement, should we really block > the static analyzer fixes on an agreed-upon panic API? I'm concerned > that would just cause these fixes to get stuck. And I don't consider > CpuDeadLoop()s added for this purpose serious technical debt. They are > easy to find and update later, assuming we also add comments. > > I agree with the approach to not gate current fixes adding CpuDeadLoop(). Later on, it can be updated with the desired panic API and I can contribute for those required changes related to patches submitted by me. I can update current patches to carry additional comment in suffix form to ease later search like CpuDeadLoop (); // TBD: replace with Panic API in future Laszlo, Mike - Let me know if that works for now. > > We also have to evaluate if a return error status with a DEBUG_ERROR > > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > > I agree 100% that this would be better for the codebase, but the work > needed for this is (IMO) impossible to contain. ASSERT() has been abused > for a long time *because* it seemed to allow the programmer to ignore > any related error paths. If we now want to implement those error paths > retroactively (which would be absolutely the right thing to do from a > software engineering perspective), then immense amounts of work are > going to be needed (patch review and regression testing), and I think > it's just not practical to dump all that on someone that wants to > improve the status quo. Replacing an invalid ASSERT() with a panic is > honest about the current situation, is safer regarding RELEASE builds, > and its work demand (regression testing, patch review) is tolerable. > > I do agree that, if the error path mostly exists already, then returning > errors for data/environment-based error conditions (i.e., not for > algorithmic invariant failures) is best. > > Where we need to be extremely vigilant is new patches. We must > uncompromisingly reject *new* abuses of ASSERT(), in new patches. > > Anyway, it seems that we've been trying to steer Ranbir in opposite > directions. I'll let you take the lead on this; for one, I've not been > aware of the panic api discussion for 2022! > > (I don't feel especially pushy about fixing coverity issues, it's just > that Ranbir has been contributing such patches, which I've found very > welcome, and I wanted to help out with reviews.) > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111444): https://edk2.groups.io/g/devel/message/111444 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 5872 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue 2023-11-20 3:57 ` Ranbir Singh @ 2023-11-21 17:07 ` Laszlo Ersek 0 siblings, 0 replies; 32+ messages in thread From: Laszlo Ersek @ 2023-11-21 17:07 UTC (permalink / raw) To: devel, rsingh Cc: Kinney, Michael D, Andrew Fish (afish@apple.com), quic_llindhol@quicinc.com, Michael Kubacki, Ni, Ray, Veeresh Sangolli On 11/20/23 04:57, Ranbir Singh wrote: > > > On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <lersek@redhat.com > <mailto:lersek@redhat.com>> wrote: > > On 11/14/23 17:21, Kinney, Michael D wrote: > > Hi Ranbir, > > > > > > > > First I want to recognize your efforts to collect Coverity issues and > > propose changes to address > > them. > > > > I still disagree with adding CpuDealLoop() for any static analysis > issues. > > > > There have been previous discussions about adding a PANIC() or FATAL() > > macro that would > > perform platform specific actions if a condition is detected where the > > boot of the platform > > can not continue. A platform get to make the choice to log or > reboot or > > hang, etc. Not the > > code that detected the condition. > > > > https://edk2.groups.io/g/devel/message/86597 > <https://edk2.groups.io/g/devel/message/86597> > > <https://edk2.groups.io/g/devel/message/86597 > <https://edk2.groups.io/g/devel/message/86597>> > > This is indeed a great idea. > > I didn't know about that discussion. Perhaps a new DebugLib API would > handle this (i.e., "panic"). > > I've been certainly proposing CpuDeadLoop() as a means to panic. > > Did the thread conclude anything tangible? > > > Unfortunately, in order to fix some of these static analysis issues > > correctly, we are going > > to have to identify the use of ASSERT() that really is a fatal > condition > > that can not continue > > Absolutely. > > > and introduce an implementation approach that provides a platform > > handler and > > satisfies the static analysis tools. > > The "platform handler" is the difficult part. If the above-noted > discussion from 2022 didn't produce an agreement, should we really block > the static analyzer fixes on an agreed-upon panic API? I'm concerned > that would just cause these fixes to get stuck. And I don't consider > CpuDeadLoop()s added for this purpose serious technical debt. They are > easy to find and update later, assuming we also add comments. > > > I agree with the approach to not gate current fixes adding > CpuDeadLoop(). Later on, it can be updated with the desired panic API > and I can contribute for those required changes related to patches > submitted by me. > > I can update current patches to carry additional comment in suffix form > to ease later search like > CpuDeadLoop (); // TBD: replace with Panic API in future > > Laszlo, Mike - Let me know if that works for now. It works for me. Of course the risk is always that the proper panic API might never materialize, and then we'll be stuck with these comments forever as yet another piece of technical debt. From that perspective minimally, it would be reasonable for Mike not to accept these reminder comments. (A reminder BZ, with the commit hash and exact code locations listed, could be a stronger reminder, but we've seen such BZs too fall through the cracks over time.) So, for me, I'm OK; but if Mike doesn't like this approach, I'll certainly accept that (and then we can't fix the coverity warnings until the panic API arrives). Thanks Laszlo > > > > We also have to evaluate if a return error status with a DEBUG_ERROR > > message would be a better > > choice than an ASSERT() that can be filtered out by build options. > > I agree 100% that this would be better for the codebase, but the work > needed for this is (IMO) impossible to contain. ASSERT() has been abused > for a long time *because* it seemed to allow the programmer to ignore > any related error paths. If we now want to implement those error paths > retroactively (which would be absolutely the right thing to do from a > software engineering perspective), then immense amounts of work are > going to be needed (patch review and regression testing), and I think > it's just not practical to dump all that on someone that wants to > improve the status quo. Replacing an invalid ASSERT() with a panic is > honest about the current situation, is safer regarding RELEASE builds, > and its work demand (regression testing, patch review) is tolerable. > > I do agree that, if the error path mostly exists already, then returning > errors for data/environment-based error conditions (i.e., not for > algorithmic invariant failures) is best. > > Where we need to be extremely vigilant is new patches. We must > uncompromisingly reject *new* abuses of ASSERT(), in new patches. > > Anyway, it seems that we've been trying to steer Ranbir in opposite > directions. I'll let you take the lead on this; for one, I've not been > aware of the panic api discussion for 2022! > > (I don't feel especially pushy about fixing coverity issues, it's just > that Ranbir has been contributing such patches, which I've found very > welcome, and I wanted to help out with reviews.) > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111587): https://edk2.groups.io/g/devel/message/111587 Mute This Topic: https://groups.io/mt/102438320/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh ` (3 preceding siblings ...) 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh @ 2023-11-07 6:19 ` Ranbir Singh 2023-11-07 17:20 ` Laszlo Ersek 4 siblings, 1 reply; 32+ messages in thread From: Ranbir Singh @ 2023-11-07 6:19 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni The return value after calls to functions gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge, PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write, gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is stored in Status, but it is not made of any use and later Status gets overridden. Remove the return value storage in Status or add Status check as seems appropriate at a particular point. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> --- MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 +++++++++++--------- MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 ++++++++---- MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 8 +++ 3 files changed, 72 insertions(+), 46 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c index 3de80d98370e..9b0587c94d05 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c @@ -544,12 +544,12 @@ DeRegisterPciDevice ( EFI_OPEN_PROTOCOL_TEST_PROTOCOL ); if (!EFI_ERROR (Status)) { - Status = gBS->UninstallMultipleProtocolInterfaces ( - Handle, - &gEfiLoadFile2ProtocolGuid, - &PciIoDevice->LoadFile2, - NULL - ); + gBS->UninstallMultipleProtocolInterfaces ( + Handle, + &gEfiLoadFile2ProtocolGuid, + &PciIoDevice->LoadFile2, + NULL + ); } // @@ -678,19 +678,21 @@ StartPciDevicesOnBridge ( ChildHandleBuffer ); - PciIoDevice->PciIo.Attributes ( - &(PciIoDevice->PciIo), - EfiPciIoAttributeOperationSupported, - 0, - &Supports - ); - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; - PciIoDevice->PciIo.Attributes ( - &(PciIoDevice->PciIo), - EfiPciIoAttributeOperationEnable, - Supports, - NULL - ); + if (!EFI_ERROR (Status)) { + PciIoDevice->PciIo.Attributes ( + &(PciIoDevice->PciIo), + EfiPciIoAttributeOperationSupported, + 0, + &Supports + ); + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; + PciIoDevice->PciIo.Attributes ( + &(PciIoDevice->PciIo), + EfiPciIoAttributeOperationEnable, + Supports, + NULL + ); + } return Status; } else { @@ -726,19 +728,21 @@ StartPciDevicesOnBridge ( ChildHandleBuffer ); - PciIoDevice->PciIo.Attributes ( - &(PciIoDevice->PciIo), - EfiPciIoAttributeOperationSupported, - 0, - &Supports - ); - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; - PciIoDevice->PciIo.Attributes ( - &(PciIoDevice->PciIo), - EfiPciIoAttributeOperationEnable, - Supports, - NULL - ); + if (!EFI_ERROR (Status)) { + PciIoDevice->PciIo.Attributes ( + &(PciIoDevice->PciIo), + EfiPciIoAttributeOperationSupported, + 0, + &Supports + ); + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; + PciIoDevice->PciIo.Attributes ( + &(PciIoDevice->PciIo), + EfiPciIoAttributeOperationEnable, + Supports, + NULL + ); + } } CurrentLink = CurrentLink->ForwardLink; diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c index eda97285ee18..636885dd189d 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c @@ -2796,6 +2796,20 @@ IsPciDeviceRejected ( // Test its high 32-Bit BAR // Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); + if (EFI_ERROR (Status)) { + // + // Not sure if it is correct to skip the below if (TestValue == OldValue) check in this special scenario. + // If correct, then remove these 11 comment lines eventually. + // If not correct, then replace "continue;" with blank "; // Nothing to do" and also remove these 11 comment lines eventually + // OR + // Remove the newly added if (EFI_ERROR (Status)) { ... } block completely and change + // Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); + // => + // BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); + // i.e., no return value storage in Status + // + continue; + } if (TestValue == OldValue) { return TRUE; } @@ -2861,13 +2875,13 @@ ResetAllPpbBusNumber ( if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { Register = 0; Address = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0x18); - Status = PciRootBridgeIo->Pci.Read ( - PciRootBridgeIo, - EfiPciWidthUint32, - Address, - 1, - &Register - ); + PciRootBridgeIo->Pci.Read ( + PciRootBridgeIo, + EfiPciWidthUint32, + Address, + 1, + &Register + ); SecondaryBus = (UINT8)(Register >> 8); if (SecondaryBus != 0) { @@ -2878,13 +2892,13 @@ ResetAllPpbBusNumber ( // Reset register 18h, 19h, 1Ah on PCI Bridge // Register &= 0xFF000000; - Status = PciRootBridgeIo->Pci.Write ( - PciRootBridgeIo, - EfiPciWidthUint32, - Address, - 1, - &Register - ); + PciRootBridgeIo->Pci.Write ( + PciRootBridgeIo, + EfiPciWidthUint32, + Address, + 1, + &Register + ); } if ((Func == 0) && !IS_PCI_MULTI_FUNC (&Pci)) { diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c index 71767d3793d4..087fe563c0bc 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c @@ -1250,6 +1250,10 @@ PciScanBus ( &State ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, "Failed to initialize Hotplug PCI Controller, Status %r\n", Status)); + } + PreprocessController ( PciDevice, PciDevice->BusNumber, @@ -1501,6 +1505,10 @@ PciRootBridgeP2CProcess ( if (!IsListEmpty (&Temp->ChildList)) { Status = PciRootBridgeP2CProcess (Temp); + + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, "Failed to process Option Rom on PCI root bridge %p, Status %r\n", Temp, Status)); + } } CurrentLink = CurrentLink->ForwardLink; -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110815): https://edk2.groups.io/g/devel/message/110815 Mute This Topic: https://groups.io/mt/102438321/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh @ 2023-11-07 17:20 ` Laszlo Ersek 2023-11-10 6:31 ` Ranbir Singh 0 siblings, 1 reply; 32+ messages in thread From: Laszlo Ersek @ 2023-11-07 17:20 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni On 11/7/23 07:19, Ranbir Singh wrote: > The return value after calls to functions > gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge, > PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write, > gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is > stored in Status, but it is not made of any use and later Status > gets overridden. > > Remove the return value storage in Status or add Status check as > seems appropriate at a particular point. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > Cc: Ray Ni <ray.ni@intel.com> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 +++++++++++--------- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 ++++++++---- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 8 +++ > 3 files changed, 72 insertions(+), 46 deletions(-) First of all, please split up this patch. It's hard to review it like this, with unrelated pieces of logic lumped together. > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > index 3de80d98370e..9b0587c94d05 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > @@ -544,12 +544,12 @@ DeRegisterPciDevice ( > EFI_OPEN_PROTOCOL_TEST_PROTOCOL > ); > if (!EFI_ERROR (Status)) { > - Status = gBS->UninstallMultipleProtocolInterfaces ( > - Handle, > - &gEfiLoadFile2ProtocolGuid, > - &PciIoDevice->LoadFile2, > - NULL > - ); > + gBS->UninstallMultipleProtocolInterfaces ( > + Handle, > + &gEfiLoadFile2ProtocolGuid, > + &PciIoDevice->LoadFile2, > + NULL > + ); > } > > // OK > @@ -678,19 +678,21 @@ StartPciDevicesOnBridge ( > ChildHandleBuffer > ); > > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationSupported, > - 0, > - &Supports > - ); > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationEnable, > - Supports, > - NULL > - ); > + if (!EFI_ERROR (Status)) { > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationSupported, > + 0, > + &Supports > + ); > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationEnable, > + Supports, > + NULL > + ); > + } > > return Status; > } else { OK > @@ -726,19 +728,21 @@ StartPciDevicesOnBridge ( > ChildHandleBuffer > ); > > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationSupported, > - 0, > - &Supports > - ); > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationEnable, > - Supports, > - NULL > - ); > + if (!EFI_ERROR (Status)) { > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationSupported, > + 0, > + &Supports > + ); > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationEnable, > + Supports, > + NULL > + ); > + } > } > > CurrentLink = CurrentLink->ForwardLink; I don't really like this; the original code is inconsistent. In the first branch, StartPciDevicesOnBridge() failure is fatal, here it isn't. :/ Anyway, I agree we can at least restrict the enablement. OK. > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > index eda97285ee18..636885dd189d 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > @@ -2796,6 +2796,20 @@ IsPciDeviceRejected ( > // Test its high 32-Bit BAR > // > Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + if (EFI_ERROR (Status)) { > + // > + // Not sure if it is correct to skip the below if (TestValue == OldValue) check in this special scenario. > + // If correct, then remove these 11 comment lines eventually. > + // If not correct, then replace "continue;" with blank "; // Nothing to do" and also remove these 11 comment lines eventually > + // OR > + // Remove the newly added if (EFI_ERROR (Status)) { ... } block completely and change > + // Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + // => > + // BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + // i.e., no return value storage in Status > + // > + continue; > + } > if (TestValue == OldValue) { > return TRUE; > } "continue" is not right here. We have already determined (from the least significant dword of the BAR) that the BAR exists. Continue seems only right when the BAR doesn't exist. In my opinion (but Ray should correct me if I'm wrong), we should not assign Status here, as we don't care whether BarExisted() finds that the response Value from the device is zero or not. That only matters if we're testing the low qword. So just remove "Status =". > @@ -2861,13 +2875,13 @@ ResetAllPpbBusNumber ( > if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { > Register = 0; > Address = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0x18); > - Status = PciRootBridgeIo->Pci.Read ( > - PciRootBridgeIo, > - EfiPciWidthUint32, > - Address, > - 1, > - &Register > - ); > + PciRootBridgeIo->Pci.Read ( > + PciRootBridgeIo, > + EfiPciWidthUint32, > + Address, > + 1, > + &Register > + ); > SecondaryBus = (UINT8)(Register >> 8); > > if (SecondaryBus != 0) { > @@ -2878,13 +2892,13 @@ ResetAllPpbBusNumber ( > // Reset register 18h, 19h, 1Ah on PCI Bridge > // > Register &= 0xFF000000; > - Status = PciRootBridgeIo->Pci.Write ( > - PciRootBridgeIo, > - EfiPciWidthUint32, > - Address, > - 1, > - &Register > - ); > + PciRootBridgeIo->Pci.Write ( > + PciRootBridgeIo, > + EfiPciWidthUint32, > + Address, > + 1, > + &Register > + ); > } > > if ((Func == 0) && !IS_PCI_MULTI_FUNC (&Pci)) { Wow, the original code is so sloppy. :( I guess itś best if we just don't assign Status here. If these accesses don't work, then nothing will. > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > index 71767d3793d4..087fe563c0bc 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > @@ -1250,6 +1250,10 @@ PciScanBus ( > &State > ); > > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "Failed to initialize Hotplug PCI Controller, Status %r\n", Status)); > + } > + > PreprocessController ( > PciDevice, > PciDevice->BusNumber, Honestly, I don't have the slightest idea. The original code is utterly broken. We have a PCI device that seems like a root hotplug controller, we fail to initialzie the root hotplug controller, we capture the Status there, and then happily go on to "pre-process" the controller. What the heck is this. If the root HPC init is not a pre-requisite for preprocessing, then why capture the status??? Well, I guess your approach is the safest. Log it, but otherwise, preserve the current behavior. Jesus. > @@ -1501,6 +1505,10 @@ PciRootBridgeP2CProcess ( > > if (!IsListEmpty (&Temp->ChildList)) { > Status = PciRootBridgeP2CProcess (Temp); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "Failed to process Option Rom on PCI root bridge %p, Status %r\n", Temp, Status)); > + } > } > > CurrentLink = CurrentLink->ForwardLink; Yeah, I guess so. On a tangent, best cast Temp to (VOID*)Temp, for logging with %p. I didn't expect that the original code was this terrible. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110867): https://edk2.groups.io/g/devel/message/110867 Mute This Topic: https://groups.io/mt/102438321/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues 2023-11-07 17:20 ` Laszlo Ersek @ 2023-11-10 6:31 ` Ranbir Singh 0 siblings, 0 replies; 32+ messages in thread From: Ranbir Singh @ 2023-11-10 6:31 UTC (permalink / raw) To: Laszlo Ersek; +Cc: devel, Ray Ni [-- Attachment #1: Type: text/plain, Size: 12573 bytes --] It was hard to conclude at my end as well what to do where. So I just threw it open ... - Status assignment can be ignored or - Maintain the existing behaviour and just log the error given the original code existence for quite a long now Various files were clubbed together being part of the same module and all having the same UNUSED_VALUE issue. If you say so, I will split, but many lines are just cosmetic changes (indentation level) - after impact of inclusion / removal of Status storage / Status value check. Will need to figure out what is the best split though :-) On Tue, Nov 7, 2023 at 10:50 PM Laszlo Ersek <lersek@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > The return value after calls to functions > > gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge, > > PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write, > > gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is > > stored in Status, but it is not made of any use and later Status > > gets overridden. > > > > Remove the return value storage in Status or add Status check as > > seems appropriate at a particular point. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > > > Cc: Ray Ni <ray.ni@intel.com> > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 > +++++++++++--------- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 ++++++++---- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 8 +++ > > 3 files changed, 72 insertions(+), 46 deletions(-) > > First of all, please split up this patch. It's hard to review it like > this, with unrelated pieces of logic lumped together. > > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > index 3de80d98370e..9b0587c94d05 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > > @@ -544,12 +544,12 @@ DeRegisterPciDevice ( > > EFI_OPEN_PROTOCOL_TEST_PROTOCOL > > ); > > if (!EFI_ERROR (Status)) { > > - Status = gBS->UninstallMultipleProtocolInterfaces ( > > - Handle, > > - &gEfiLoadFile2ProtocolGuid, > > - &PciIoDevice->LoadFile2, > > - NULL > > - ); > > + gBS->UninstallMultipleProtocolInterfaces ( > > + Handle, > > + &gEfiLoadFile2ProtocolGuid, > > + &PciIoDevice->LoadFile2, > > + NULL > > + ); > > } > > > > // > > OK > > > @@ -678,19 +678,21 @@ StartPciDevicesOnBridge ( > > ChildHandleBuffer > > ); > > > > - PciIoDevice->PciIo.Attributes ( > > - &(PciIoDevice->PciIo), > > - EfiPciIoAttributeOperationSupported, > > - 0, > > - &Supports > > - ); > > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > > - PciIoDevice->PciIo.Attributes ( > > - &(PciIoDevice->PciIo), > > - EfiPciIoAttributeOperationEnable, > > - Supports, > > - NULL > > - ); > > + if (!EFI_ERROR (Status)) { > > + PciIoDevice->PciIo.Attributes ( > > + &(PciIoDevice->PciIo), > > + EfiPciIoAttributeOperationSupported, > > + 0, > > + &Supports > > + ); > > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > > + PciIoDevice->PciIo.Attributes ( > > + &(PciIoDevice->PciIo), > > + EfiPciIoAttributeOperationEnable, > > + Supports, > > + NULL > > + ); > > + } > > > > return Status; > > } else { > > OK > > > @@ -726,19 +728,21 @@ StartPciDevicesOnBridge ( > > ChildHandleBuffer > > ); > > > > - PciIoDevice->PciIo.Attributes ( > > - &(PciIoDevice->PciIo), > > - EfiPciIoAttributeOperationSupported, > > - 0, > > - &Supports > > - ); > > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > > - PciIoDevice->PciIo.Attributes ( > > - &(PciIoDevice->PciIo), > > - EfiPciIoAttributeOperationEnable, > > - Supports, > > - NULL > > - ); > > + if (!EFI_ERROR (Status)) { > > + PciIoDevice->PciIo.Attributes ( > > + &(PciIoDevice->PciIo), > > + EfiPciIoAttributeOperationSupported, > > + 0, > > + &Supports > > + ); > > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > > + PciIoDevice->PciIo.Attributes ( > > + &(PciIoDevice->PciIo), > > + EfiPciIoAttributeOperationEnable, > > + Supports, > > + NULL > > + ); > > + } > > } > > > > CurrentLink = CurrentLink->ForwardLink; > > I don't really like this; the original code is inconsistent. In the > first branch, StartPciDevicesOnBridge() failure is fatal, here it isn't. :/ > > Anyway, I agree we can at least restrict the enablement. OK. > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > index eda97285ee18..636885dd189d 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > @@ -2796,6 +2796,20 @@ IsPciDeviceRejected ( > > // Test its high 32-Bit BAR > > // > > Status = BarExisted (PciIoDevice, BarOffset, &TestValue, > &OldValue); > > + if (EFI_ERROR (Status)) { > > + // > > + // Not sure if it is correct to skip the below if > (TestValue == OldValue) check in this special scenario. > > + // If correct, then remove these 11 comment lines > eventually. > > + // If not correct, then replace "continue;" with blank "; > // Nothing to do" and also remove these 11 comment lines eventually > > + // OR > > + // Remove the newly added if (EFI_ERROR (Status)) { ... } > block completely and change > > + // Status = BarExisted (PciIoDevice, BarOffset, > &TestValue, &OldValue); > > + // => > > + // BarExisted (PciIoDevice, BarOffset, &TestValue, > &OldValue); > > + // i.e., no return value storage in Status > > + // > > + continue; > > + } > > if (TestValue == OldValue) { > > return TRUE; > > } > > "continue" is not right here. We have already determined (from the least > significant dword of the BAR) that the BAR exists. Continue seems only > right when the BAR doesn't exist. > > In my opinion (but Ray should correct me if I'm wrong), we should not > assign Status here, as we don't care whether BarExisted() finds that the > response Value from the device is zero or not. That only matters if > we're testing the low qword. So just remove "Status =". > > > > @@ -2861,13 +2875,13 @@ ResetAllPpbBusNumber ( > > if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { > > Register = 0; > > Address = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0x18); > > - Status = PciRootBridgeIo->Pci.Read ( > > - PciRootBridgeIo, > > - EfiPciWidthUint32, > > - Address, > > - 1, > > - &Register > > - ); > > + PciRootBridgeIo->Pci.Read ( > > + PciRootBridgeIo, > > + EfiPciWidthUint32, > > + Address, > > + 1, > > + &Register > > + ); > > SecondaryBus = (UINT8)(Register >> 8); > > > > if (SecondaryBus != 0) { > > @@ -2878,13 +2892,13 @@ ResetAllPpbBusNumber ( > > // Reset register 18h, 19h, 1Ah on PCI Bridge > > // > > Register &= 0xFF000000; > > - Status = PciRootBridgeIo->Pci.Write ( > > - PciRootBridgeIo, > > - EfiPciWidthUint32, > > - Address, > > - 1, > > - &Register > > - ); > > + PciRootBridgeIo->Pci.Write ( > > + PciRootBridgeIo, > > + EfiPciWidthUint32, > > + Address, > > + 1, > > + &Register > > + ); > > } > > > > if ((Func == 0) && !IS_PCI_MULTI_FUNC (&Pci)) { > > Wow, the original code is so sloppy. :( > > I guess itś best if we just don't assign Status here. If these accesses > don't work, then nothing will. > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > index 71767d3793d4..087fe563c0bc 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > @@ -1250,6 +1250,10 @@ PciScanBus ( > > &State > > ); > > > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, "Failed to initialize Hotplug PCI > Controller, Status %r\n", Status)); > > + } > > + > > PreprocessController ( > > PciDevice, > > PciDevice->BusNumber, > > Honestly, I don't have the slightest idea. The original code is utterly > broken. We have a PCI device that seems like a root hotplug controller, > we fail to initialzie the root hotplug controller, we capture the Status > there, and then happily go on to "pre-process" the controller. > > What the heck is this. If the root HPC init is not a pre-requisite for > preprocessing, then why capture the status??? > > Well, I guess your approach is the safest. Log it, but otherwise, > preserve the current behavior. Jesus. > > > @@ -1501,6 +1505,10 @@ PciRootBridgeP2CProcess ( > > > > if (!IsListEmpty (&Temp->ChildList)) { > > Status = PciRootBridgeP2CProcess (Temp); > > + > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_WARN, "Failed to process Option Rom on PCI root > bridge %p, Status %r\n", Temp, Status)); > > + } > > } > > > > CurrentLink = CurrentLink->ForwardLink; > > Yeah, I guess so. > > On a tangent, best cast Temp to (VOID*)Temp, for logging with %p. > > I didn't expect that the original code was this terrible. > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111019): https://edk2.groups.io/g/devel/message/111019 Mute This Topic: https://groups.io/mt/102438321/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 16715 bytes --] ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2023-11-21 17:07 UTC | newest] Thread overview: 32+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-07 6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh 2023-11-07 16:21 ` Laszlo Ersek 2023-11-10 6:14 ` Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh 2023-11-07 8:15 ` Ni, Ray 2023-11-07 16:23 ` Laszlo Ersek 2023-11-07 17:59 ` Michael D Kinney 2023-11-08 3:51 ` Ranbir Singh 2023-11-08 4:05 ` Michael D Kinney 2023-11-08 4:29 ` Ranbir Singh 2023-11-13 11:24 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh 2023-11-07 16:41 ` Laszlo Ersek 2023-11-10 5:59 ` Ranbir Singh 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh 2023-11-07 16:48 ` Laszlo Ersek 2023-11-10 6:11 ` Ranbir Singh 2023-11-13 11:28 ` Laszlo Ersek 2023-11-14 15:08 ` Ranbir Singh 2023-11-14 16:21 ` Michael D Kinney 2023-11-14 16:53 ` Ranbir Singh 2023-11-15 10:02 ` Laszlo Ersek 2023-11-14 19:37 ` Michael Kubacki 2023-11-15 10:10 ` Laszlo Ersek 2023-11-20 14:05 ` Michael Kubacki 2023-11-15 9:50 ` Laszlo Ersek 2023-11-20 3:57 ` Ranbir Singh 2023-11-21 17:07 ` Laszlo Ersek 2023-11-07 6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh 2023-11-07 17:20 ` Laszlo Ersek 2023-11-10 6:31 ` Ranbir Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox