Re: [edk2-devel] [PATCH v4 09/13] OvmfPkg/MptScsiDxe: Set and restore PCI attributes

["Laszlo Ersek" <lersek@redhat.com>]

On 04/14/20 19:38, Nikita Leshenko wrote:
> Enable the IO Space and Bus Mastering and restore the original values
> when the device is stopped. This is a standard procedure in PCI
> drivers.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c | 42 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 275265774252..4bfd03d2acb0 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -36,6 +36,7 @@ typedef struct {
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
> +  UINT64                          OriginalPciAttributes;
>  } MPT_SCSI_DEV;
>  
>  #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> @@ -308,6 +309,30 @@ MptScsiControllerStart (
>      goto FreePool;
>    }
>  
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationGet,
> +                         0,
> +                         &Dev->OriginalPciAttributes
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto CloseProtocol;
> +  }
> +
> +  //
> +  // Enable I/O Space & Bus-Mastering
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         (EFI_PCI_IO_ATTRIBUTE_IO |
> +                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto CloseProtocol;
> +  }
> +
>    //
>    // Host adapter channel, doesn't exist
>    //
> @@ -332,11 +357,19 @@ MptScsiControllerStart (
>                    &Dev->PassThru
>                    );
>    if (EFI_ERROR (Status)) {
> -    goto CloseProtocol;
> +    goto RestoreAttributes;
>    }
>  
>    return EFI_SUCCESS;
>  
> +RestoreAttributes:
> +  Dev->PciIo->Attributes (
> +                Dev->PciIo,
> +                EfiPciIoAttributeOperationEnable,

(1) This should be "...Set", not "...Enable".

> +                Dev->OriginalPciAttributes,
> +                NULL
> +                );
> +
>  CloseProtocol:
>    gBS->CloseProtocol (
>           ControllerHandle,
> @@ -388,6 +421,13 @@ MptScsiControllerStop (
>      return Status;
>    }
>  
> +  Dev->PciIo->Attributes (
> +                Dev->PciIo,
> +                EfiPciIoAttributeOperationEnable,

(2) Same as (1).


With (1) and (2) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +                Dev->OriginalPciAttributes,
> +                NULL
> +                );
> +
>    gBS->CloseProtocol (
>           ControllerHandle,
>           &gEfiPciIoProtocolGuid,
>