diff mbox series

[Linaro-uefi,Linaro-uefi,v1,11/21] Hisilicon: Fix SCT PCIBusSupportTest error

Message ID 1490015485-53685-12-git-send-email-chenhui.sun@linaro.org
State New
Headers show
Series D02/D03 platforms bug fix | expand

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m. UTC 
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL/EFI_PCI_IO_PROTOCOL must returns
EFI_INVALID_PARAMETER, change return value.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: wanglijun <wanglijun@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
---
 .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c          | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Leif Lindholm March 21, 2017, 3:28 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 09:11:15PM +0800, Chenhui Sun wrote:
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL/EFI_PCI_IO_PROTOCOL must returns
> EFI_INVALID_PARAMETER, change return value.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: wanglijun <wanglijun@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> ---
>  .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c          | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> index 03edcf1..30619f5 100644
> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -824,8 +824,8 @@ RootBridgeConstructor (
>    PrivateData->Supports    = EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | \
>                               EFI_PCI_ATTRIBUTE_ISA_IO_16 | EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | \
>                               EFI_PCI_ATTRIBUTE_VGA_MEMORY | \
> -                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16 | \
> -                             EFI_PCI_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER;
> +                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
> +

This deletion of EFI_PCI_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER does not
obviously relate to what the commit message describes. Can you expand
the explanation, please?

>    PrivateData->Attributes  = PrivateData->Supports;
>  
>    Protocol->ParentHandle   = HostBridgeHandle;
> @@ -1275,6 +1275,9 @@ RootBridgeIoPollMem (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  if (Width > EfiPciWidthUint64) {
> +    return EFI_INVALID_PARAMETER;
> +  }

Add blank line after?

>    //
>    // No matter what, always do a single poll.
>    //
> @@ -2010,7 +2013,10 @@ RootBridgeIoMap (
>    )
>  {
>    DMA_MAP_OPERATION   DmaOperation;
> -

Why delete this blank line?

> +  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
> +      Mapping == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }

I would prefer a blank line here.

>    if (Operation == EfiPciOperationBusMasterRead) {
>      DmaOperation = MapOperationBusMasterRead;
>    } else if (Operation == EfiPciOperationBusMasterWrite) {
> @@ -2245,6 +2251,11 @@ RootBridgeIoSetAttributes (
>  
>    PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS(This);
>  
> +  if((Attributes & (EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE | EFI_PCI_ATTRIBUTE_MEMORY_CACHED)) &&
> +     (ResourceBase == NULL || ResourceLength == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    if (Attributes != 0) {
>      if ((Attributes & (~(PrivateData->Supports))) != 0) {
>        return EFI_UNSUPPORTED;
> -- 
> 1.9.1
>
Chenhui Sun April 1, 2017, 7:21 a.m. UTC | #2 
Hi Leif,

在 2017/3/21 23:28, Leif Lindholm 写道:
> On Mon, Mar 20, 2017 at 09:11:15PM +0800, Chenhui Sun wrote:
>> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL/EFI_PCI_IO_PROTOCOL must returns
>> EFI_INVALID_PARAMETER, change return value.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: wanglijun <wanglijun@huawei.com>
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
>> ---
>>   .../Drivers/PciHostBridgeDxe/PciRootBridgeIo.c          | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 03edcf1..30619f5 100644
>> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -824,8 +824,8 @@ RootBridgeConstructor (
>>     PrivateData->Supports    = EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | \
>>                                EFI_PCI_ATTRIBUTE_ISA_IO_16 | EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | \
>>                                EFI_PCI_ATTRIBUTE_VGA_MEMORY | \
>> -                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16 | \
>> -                             EFI_PCI_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER;
>> +                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
>> +
> This deletion of EFI_PCI_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER does not
> obviously relate to what the commit message describes. Can you expand
> the explanation, please?
There seems to be something wrong here,  we will check it first.
thanks.

Regards,
Chenhui
>>     PrivateData->Attributes  = PrivateData->Supports;
>>   
>>     Protocol->ParentHandle   = HostBridgeHandle;
>> @@ -1275,6 +1275,9 @@ RootBridgeIoPollMem (
>>       return EFI_INVALID_PARAMETER;
>>     }
>>   
>> +  if (Width > EfiPciWidthUint64) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
> Add blank line after?
>
>>     //
>>     // No matter what, always do a single poll.
>>     //
>> @@ -2010,7 +2013,10 @@ RootBridgeIoMap (
>>     )
>>   {
>>     DMA_MAP_OPERATION   DmaOperation;
>> -
> Why delete this blank line?
>
>> +  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
>> +      Mapping == NULL) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
> I would prefer a blank line here.
>
>>     if (Operation == EfiPciOperationBusMasterRead) {
>>       DmaOperation = MapOperationBusMasterRead;
>>     } else if (Operation == EfiPciOperationBusMasterWrite) {
>> @@ -2245,6 +2251,11 @@ RootBridgeIoSetAttributes (
>>   
>>     PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS(This);
>>   
>> +  if((Attributes & (EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE | EFI_PCI_ATTRIBUTE_MEMORY_CACHED)) &&
>> +     (ResourceBase == NULL || ResourceLength == NULL)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>>     if (Attributes != 0) {
>>       if ((Attributes & (~(PrivateData->Supports))) != 0) {
>>         return EFI_UNSUPPORTED;
>> -- 
>> 1.9.1
>>
diff mbox series

Patch

diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
index 03edcf1..30619f5 100644
--- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -824,8 +824,8 @@  RootBridgeConstructor (
   PrivateData->Supports    = EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | \
                              EFI_PCI_ATTRIBUTE_ISA_IO_16 | EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | \
                              EFI_PCI_ATTRIBUTE_VGA_MEMORY | \
-                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16 | \
-                             EFI_PCI_ATTRIBUTE_VALID_FOR_ALLOCATE_BUFFER;
+                             EFI_PCI_ATTRIBUTE_VGA_IO_16  | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
+
   PrivateData->Attributes  = PrivateData->Supports;
 
   Protocol->ParentHandle   = HostBridgeHandle;
@@ -1275,6 +1275,9 @@  RootBridgeIoPollMem (
     return EFI_INVALID_PARAMETER;
   }
 
+  if (Width > EfiPciWidthUint64) {
+    return EFI_INVALID_PARAMETER;
+  }
   //
   // No matter what, always do a single poll.
   //
@@ -2010,7 +2013,10 @@  RootBridgeIoMap (
   )
 {
   DMA_MAP_OPERATION   DmaOperation;
-
+  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
+      Mapping == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
   if (Operation == EfiPciOperationBusMasterRead) {
     DmaOperation = MapOperationBusMasterRead;
   } else if (Operation == EfiPciOperationBusMasterWrite) {
@@ -2245,6 +2251,11 @@  RootBridgeIoSetAttributes (
 
   PrivateData = DRIVER_INSTANCE_FROM_PCI_ROOT_BRIDGE_IO_THIS(This);
 
+  if((Attributes & (EFI_PCI_ATTRIBUTE_MEMORY_WRITE_COMBINE | EFI_PCI_ATTRIBUTE_MEMORY_CACHED)) &&
+     (ResourceBase == NULL || ResourceLength == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (Attributes != 0) {
     if ((Attributes & (~(PrivateData->Supports))) != 0) {
       return EFI_UNSUPPORTED;