v1 -> v2:
- Rebased to latest master HEAD
- Changed the commit message wrt MISSING_BREAK issues
- Cc update
Ranbir Singh (2):
MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
issue
MdeModulePkg/
From: Ranbir Singh
The function NotifyPhase has a check
ASSERT (Index < TypeMax);
but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2
From: Ranbir Singh
The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: if there is no scenario of
return EFI_INVALID_PARAMETER;
While this may be intentional, it is not evident to any general
v2 -> v3:
- Dropped the ASSERT wrt REVERSE_INULL issue as suggested
- Added Reviewed-by: tags
- Rebased to latest master HEAD
v1 -> v2:
- Rebased to latest master HEAD
- Changed the solution wrt MISSING_BREAK issues
Ranbir Singh (2):
MdeModulePkg/Bus/Usb/UsbMouseDx
From: Ranbir Singh
The function USBMouseDriverBindingStart do have
ASSERT (UsbMouseDevice != NULL);
after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference
if it hypothetically happens are redundant as the code
still will eventually return NULL only at the function end point.
Better introduce straight forward break; statement within actual cases.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
Cc: Ray Ni
Signed-off-by: Ranbir Singh
v1 -> v2:
- Rebased to latest master HEAD
- Updated Cc list
- Updated the commit message wrt MISSING_BREAK issues
Ranbir Singh (2):
MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues
MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues
MdeModulePkg/Bus/
From: Ranbir Singh
The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr
and UsbHcFreeMem do have
ASSERT ((Block != NULL));
statements after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside for
loop and
From: Ranbir Singh
The functions
XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has
a switch-case code in which the case USB_ENDPOINT_CONTROL: falls
through to default:
While this may be intentional, it is not evident to any general code
reader why there is no break; in between
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_BR
From: Ranbir Singh
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
From: Ranbir Singh
The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly intentional,
it is not evident to any general code reader why there is no break; in
between. Adding
// No break; here as this is an
From: Ranbir Singh
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
From: Ranbir Singh
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 wi
y 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
Signed-off-by: Ranbir Singh
---
MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
v1 -> v2:
- Rebased to latest master HEAD
Ranbir Singh (2):
FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues
FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue
FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
FatPkg/EnhancedFatDxe/DiskCache.c | 2 +-
2 fi
From: Ranbir Singh
The functions FatGetDirEntInfo and FatOpenDirEnt contains the code
statements
Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
and
OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) |
(D
From: Ranbir Singh
The function FatInitializeDiskCache evaluates an expression
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment
and assigns it to DataCacheSize which is of type UINTN.
As per Coverity report,
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].Page
gt; Sent: Tuesday, November 7, 2023 8:23 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Ni, Ray ; Veeresh Sangolli
> >
> > Subject: Re: [edk2-devel] [PATCH v2 2/5]
> > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> >
de 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
wrote:
> Hi Ranbir,
>
>
>
> Ignoring false positive in static analysis tools is typ
11/7/23 06:06, Ranbir Singh wrote:
> > From: Ranbir Singh
> >
> > The function NotifyPhase has a check
> >
> > ASSERT (Index < TypeMax);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if t
v2 -> v3:
- Rebased to latest master HEAD
- Replaced continue with CpuDeadLoop wrt OVERRUN issue
- Changed to more generic solution wrt MISSING_BREAK issue
v1 -> v2:
- Rebased to latest master HEAD
- Changed the commit message wrt MISSING_BREAK issues
- Cc update
Ranbir Si
From: Ranbir Singh
The function NotifyPhase has a check
ASSERT (Index < TypeMax);
but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2
From: Ranbir Singh
The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
While this may be intentional, it may not be evident to any general code
reader
>
>
> > -Original Message-
> > From: devel@edk2.groups.io On Behalf Of Ranbir
> > Singh
> > Sent: Thursday, November 9, 2023 9:39 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Ni, Ray ; Veeresh Sangolli
> >
> > Subj
Options before us till now -
1. Add array overrun check and Debug statement before CpuDeadLoop within
2. Status Quo (not everything can be ideal :-))
Question before us
- Is 1 better than 2 ?
On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh
wrote:
> As far as I know, from a secure cod
v2 -> v3:
- Updated FORWARD_NULL issue
- Included DEBUG message and CpuDeadLoop within if blocks
- Updated MISSING_BREAK issues resolution
- Merged fallthrough case with default with conditional messaging
Ranbir Singh (2):
MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Cover
From: Ranbir Singh
The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr
and UsbHcFreeMem do have
ASSERT ((Block != NULL));
statements after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside for
loop and
From: Ranbir Singh
The functions
XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has
a switch-case code in which the case USB_ENDPOINT_CONTROL: falls
through to default:
While this may be intentional, it may not be evident to any general code
reader/developer or static analyis
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 wrote:
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh
> >
> > The function PciHostBridgeRes
Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek wrote:
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh
> >
> > The function StartPciDevices has a check
> >
> > ASSERT (RootBridge != NULL);
> >
> > but this comes into play only in DEBUG mo
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 wrote:
> On 11/7/23 07:19, Ranbir Singh wrote:
> > Fro
at 10:50 PM Laszlo Ersek 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,
> > g
On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek 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 appr
On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek wrote:
> On 11/9/23 18:39, Ranbir Singh wrote:
> > From: Ranbir Singh
> >
> > The function SubmitResources has a switch-case code in which the
> > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
> > ca
On Mon, Nov 13, 2023 at 9:42 PM Laszlo Ersek wrote:
> On 11/10/23 05:07, Ranbir Singh wrote:
> > Options before us till now -
> >
> > 1. Add array overrun check and Debug statement before CpuDeadLoop within
>
> This would be useless.
>
> Your current patch
ltered 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.
On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek 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 an
On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A wrote:
> > -Original Message-
> > From: Ranbir Singh
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ; Veeresh
> > Sangolli
> >
llowed
>
> value for 'Interval' (which is 1) is being passed into this
> UhciConvertPollRate
>
> function if anything does go wrong brought by future changes.
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Ranbir Singh
> *Sent:* Monday, August
v1 -> v2:
- Update patch wrt BAD_SHIFT changes as per review comments
Ranbir Singh (2):
MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
MdeModulePkg/Bus/Pci/UhciDxe: Fix FORWARD_NULL Coverity issues
MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 10 +-
MdeModulePkg/Bus/
From: Ranbir Singh
The function UhciConvertPollRate has a check
ASSERT (Interval != 0);
but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Interval parameter value is ZERO. To avoid shifting
by a negative amount later in the code flow in this
From: Ranbir Singh
The function UsbHcGetPciAddressForHostMem has
ASSERT ((Block != NULL)); and
and the function UsbHcFreeMem has
ASSERT (Block != NULL);
statement after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match
Soft reminder to have a look at the series
On Mon, Jul 17, 2023 at 5:24 PM Ranbir Singh
wrote:
> Ranbir Singh (2):
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
> issue
>
> Md
Ranbir Singh (5):
MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues
MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue
MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue
MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues
MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE
From: Ranbir Singh
The functions CoreConvertSpace and CoreAllocateSpace in
MdeModulePkg/Core/Dxe/Gcd/Gcd.c has
ASSERT (FALSE); at lines 755 and 1155 which gets hit when
Operation neither include GCD_MEMORY_SPACE_OPERATION nor include
GCD_IO_SPACE_OPERATION but this comes into play
From: Ranbir Singh
The function CoreIsSchedulable has switch-case code in which
case EFI_DEP_BEFORE:
case EFI_DEP_AFTER:
has a comment that
// For a well-formed Dependency Expression, the code should never get here.
It also has an ASSERT (FALSE); but this is applicable only in
From: Ranbir Singh
In the function PromoteGuardedFreePages(), the value of AvailablePages
cannot be ZERO at the condition check point
if (AvailablePages != 0) {
as the code can come out of the while loop above only when AvailablePages
is non-ZERO.
Hence, remove the redundant condition check
From: Ranbir Singh
"1 << Priority" / "1 << Event->NotifyTpl" are potentially overflowing
expressions with type "int" (32 bits, signed) evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "UINTN&quo
Singh
Signed-off-by: Ranbir Singh
---
MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 15 +++
MdeModulePkg/Core/Dxe/Image/Image.c | 3 +++
3 files changed, 33 insertions(+)
diff --git a/MdeModulePkg/Core
v1 -> v2:
- Rebased
- Commit message update wrt Fix UNUSED_VALUE Coverity issues
Ranbir Singh (5):
MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues
MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue
MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue
MdeModulePkg/Core/
From: Ranbir Singh
The function CoreIsSchedulable has switch-case code in which
case EFI_DEP_BEFORE:
case EFI_DEP_AFTER:
has a comment that
// For a well-formed Dependency Expression, the code should never get here.
It also has an ASSERT (FALSE); but this is applicable only in
From: Ranbir Singh
The functions CoreConvertSpace and CoreAllocateSpace in
MdeModulePkg/Core/Dxe/Gcd/Gcd.c has
ASSERT (FALSE); at lines 755 and 1155 which gets hit when
Operation neither include GCD_MEMORY_SPACE_OPERATION nor include
GCD_IO_SPACE_OPERATION but this comes into play
From: Ranbir Singh
In the function PromoteGuardedFreePages(), the value of AvailablePages
cannot be ZERO at the condition check point
if (AvailablePages != 0) {
as the code can come out of the while loop above only when AvailablePages
is non-ZERO.
Hence, remove the redundant condition check
From: Ranbir Singh
"1 << Priority" / "1 << Event->NotifyTpl" are potentially overflowing
expressions with type "int" (32 bits, signed) evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "UINTN&quo
: Ranbir Singh
---
MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 15 +++
MdeModulePkg/Core/Dxe/Image/Image.c | 3 +++
3 files changed, 33 insertions(+)
diff --git a/MdeModulePkg/Core/Dxe/Dispatcher
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
From: Ranbir Singh
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
From: Ranbir Singh
The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly intentional,
it is not evident to any static analyzer tool. Just adding
// No break; here as this is an intentional fallthrough.
as
From: Ranbir Singh
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
From: Ranbir Singh
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 wi
y 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: Hao A Wu
Cc: Ray Ni
Signed-off-by: Ranbir Singh
---
MdeModulePkg/Bus/Pci/
Ranbir Singh (2):
FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues
FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue
FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
FatPkg/EnhancedFatDxe/DiskCache.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions
From: Ranbir Singh
The functions FatGetDirEntInfo and FatOpenDirEnt contains the code
statements
Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
and
OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) |
(D
From: Ranbir Singh
The function FatInitializeDiskCache evaluates an expression
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment
and assigns it to DataCacheSize which is of type UINTN.
As per Coverity report,
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].Page
Ranbir Singh (2):
MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues
MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues
MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 14 ++
MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 6 ++
2 files changed, 20 insertions
From: Ranbir Singh
The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr
and UsbHcFreeMem do have
ASSERT ((Block != NULL));
statements after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside for
loop and
From: Ranbir Singh
The functions
XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has
a switch-case code in which the case USB_ENDPOINT_CONTROL: falls
through to default:
While this may be intentional, it is not evident to any general code
reader as well as any static analyzer
Ranbir Singh (2):
MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues
MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8
MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4
2 files changed, 12
From: Ranbir Singh
The function USBMouseDriverBindingStart do have
ASSERT (UsbMouseDevice != NULL);
after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference
From: Ranbir Singh
The function GetNextHidItem has a switch-case code in which the
case 1: falls through to case 2: and then case 2: falls through
to case 3: in the block.
While this may be intentional, it is not evident to any general
code reader as well as any static analyzer tool. Just
h for this.
Ranbir Singh
On Thu, Oct 5, 2023 at 12:17 AM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:
> I do not prefer special comments for one static analyzer.
>
> Is there an alternative design/implementation of this code
> to make it more readable and not trigger any stati
v1 -> v2:
- Rebased to latest master HEAD
- Changed the solution wrt MISSING_BREAK issues
Ranbir Singh (2):
MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues
MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHi
From: Ranbir Singh
The function USBMouseDriverBindingStart do have
ASSERT (UsbMouseDevice != NULL);
after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference
if it hypothetically happens are redundant as the code
still will eventually return NULL only at the function end point.
Better introduce straight forward break; statement within actual cases.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
Cc: Hao A Wu
Cc: Ray Ni
Signed-off-by: Ranbir
ucture boundary.
Hence keep array access and index increment inside if block only and
return invalid performance timestamp when PerfIdx is invalid.
Cc: Chasel Chiu
Cc: Nate DeSimone
Cc: Star Zeng
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4200
Signed-off-by: Ranbir Singh
Signed-off-b
-Original Message-
> > From: devel@edk2.groups.io On Behalf Of Pedro
> > Falcato
> > Sent: Friday, May 19, 2023 5:29 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Chiu, Chasel ; Desimone, Nathaniel L
> > ; Zeng, Star ;
> Ranbir
> >
From: Ranbir Singh
The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Assuming, this non-usage is deliberate, the storage in Status can be
done away with.
Cc: Hao A Wu
Cc: Ray Ni
REF: https
From: Ranbir Singh
Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
Cc: Hao A W
From: Ranbir Singh
Line number 365 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
Cc: Hao A W
2023 at 21:42, Ranbir Singh wrote:
> >
> > From: Ranbir Singh
> >
> > Line number 1348 does contain a typecast with UINT32, but it is after
> > all the operations (16-bit left shift followed by OR'ing) are over.
> > To avoid any SIGN_EXTENSION, typecast the int
n Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel wrote:
> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato
> wrote:
> >
> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh
> wrote:
> > >
> > > From: Ranbir Singh
> > >
> > > The return value stored in St
uter cast is being implicitly done as
per the data type of *BlockSize *and also there was no Coverity warning
after the removal*.*
On Mon, Jun 5, 2023 at 2:14 PM Ard Biesheuvel wrote:
> On Mon, 5 Jun 2023 at 09:54, Ranbir Singh wrote:
> >
> > Please read the Coverity report a
Yes, I have already noted to update this patch on the same lines as
discussed in context of https://edk2.groups.io/g/devel/topic/99293622
On Thu, Jun 8, 2023 at 12:48 PM Wu, Hao A wrote:
> > -Original Message-
> > From: Ranbir Singh
> > Sent: Saturday, June 3, 20
2023 at 12:25 PM Wu, Hao A wrote:
> > -Original Message-
> > From: devel@edk2.groups.io On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, June 5, 2023 4:32 PM
> > To: Ranbir Singh
> > Cc: devel@edk2.groups.io; pedro.falc...@gmail.com; Wu, Hao A
> &g
From: Ranbir Singh
Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
Cc: Hao A W
From: Ranbir Singh
The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Add error check as is done after calls to SetDeviceTransferMode.
Cc: Hao A Wu
Cc: Ray Ni
REF: https://bugzilla.tianocore.org/show_bug.cgi?id
v1 -> v2:
- Retain outer cast
- Add error check instead of Status storage removal
Ranbir Singh (2):
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
MdeModulePkg/Bus/Ata/AtaAtapiPassT
From: Ranbir Singh
The function UsbHcGetPciAddressForHostMem has
ASSERT ((Block != NULL));
and the UsbHcFreeMem has
ASSERT (Block != NULL);
statement after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside for
From: Ranbir Singh
The function UsbHcGetPciAddressForHostMem has
ASSERT ((Block != NULL));
and the UsbHcFreeMem has
ASSERT (Block != NULL);
statement after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside the
for
Agreed! Will update accordingly.
On Wed, Jul 12, 2023 at 12:36 PM Wu, Hao A wrote:
> It works for me, better to override by:
>
> Status = EFI_SUCCESS;
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Ranbir Singh
> *Sent:* Wednesday, July 12, 202
know if this is acceptable and I will
update the patch as below then
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
Status));
/* Ignore Warning and proceed normally */
Status = 0;
}
Best Regards,
Ranbir Sin
v2 -> v3:
- [Patch 2] Update as per review comments
v1 -> v2:
- Retain outer cast
- Add error check instead of Status storage removal
Ranbir Singh (2):
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED
From: Ranbir Singh
Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
Cc: Hao A W
From: Ranbir Singh
The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Add error check as is done after calls to SetDeviceTransferMode.
Cc: Hao A Wu
Cc: Ray Ni
REF: https://bugzilla.tianocore.org/show_bug.cgi?id
>
> To please both sides, how about:
> 1. Remove the 'Status' assignment of the return value from
> SetDriveParameters()
> Based on my findings (https://edk2.groups.io/g/devel/message/106844), the
> successful execution of SetDriveParameters() is not mandatory for
> initializing
> IDE mode hard dis
of Status storage removal
Ranbir Singh (2):
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/
v1 -> v2:
- Retain outer cast
Ranbir Singh (1):
MdeModulePkg/Bus/Ata/AtaBusDxe: Fix SIGN_EXTENSION Coverity issue
MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
2.34.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive
From: Ranbir Singh
Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.
REF:
From: Ranbir Singh
The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Based on Hao's findings (https://edk2.groups.io/g/devel/message/106844),
the successful execution of SetDriveParameters() is not mandator
1 - 100 of 140 matches
Mail list logo
