Re: [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed.


Wu, Hao A
 

Two inline comments below:

-----Original Message-----
From: xueshengfeng <xueshengfeng@byosoft.com.cn>
Sent: Wednesday, September 22, 2021 11:18 AM
To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Wu, Hao A
<hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Xue, ShengfengX <shengfengx.xue@intel.com>; Liang, PanlingX
<panlingx.liang@intel.com>
Subject: [PATCH] On branch PCIBus dulePkg/PciBusDxe:
PciTestSupportedAttribute logic should be changed.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3635

In file Edk2\MdeModulePkg\Bus\Pci\PciBusDxe\PciEnumeratorSupport.c
Function PciTestSupportedAttribute, This function is called when doing the PCI
enumerate, it tries to test whether the device can support given attributes by
follow steps.
1), read the original register value
2), set to the input register value
3), read back the register value, return this value as output 4), restore the
original value This will cause the enabled bits in this
register be disabled during this sequence.

Below are the new suggested flow:
1) , read the original register value.
2), set to input register value OR(|) the original register value.
3), read back the register value, return the value AND(&) the input
command value as output.
4), restore the original value

Above sequence will not change the enabled bits in the register,
and can the new supported attributes by the device.

Signed-off-by: shengfengx.xue@intel.com
Reviewed-by: gaoliming@byosoft.com.cn

Please fix the inconsistent space indent within the commit log message.


---
MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index db1b35f8ef..2462f58833 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -933,6 +933,7 @@ PciTestSupportedAttribute (
)
{
EFI_TPL OldTpl;
+ UINT16 CommandTemp = 0;

Please separate the local variable definition and its initial value assignment.

With the above 2 comments handled,
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu



//
// Preserve the original value
@@ -944,9 +945,10 @@ PciTestSupportedAttribute (
//
OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);

- PCI_SET_COMMAND_REGISTER (PciIoDevice, *Command);
- PCI_READ_COMMAND_REGISTER (PciIoDevice, Command);
+ PCI_SET_COMMAND_REGISTER (PciIoDevice, (*Command | *OldCommand));
+ PCI_READ_COMMAND_REGISTER (PciIoDevice, &CommandTemp);

+ *Command = (*Command) & CommandTemp;
//
// Write back the original value
//
--
2.31.1.windows.1

Join devel@edk2.groups.io to automatically receive all group messages.