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

["Wu, Hao A" <hao.a.wu@intel.com>]

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
>