The SATA controller driver crashes while accessing the
PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
space is not enabled.

Enable the PCI memory space access to prevent the SATA
Controller driver from crashing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
The changes can be viewed at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2

Notes:
    v2:
    - Improved log message and code documentation based on feedback   [SAMI]
    - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
    - This SATA Controller driver only uses the PCI BAR5 register
      space which is the AHCI Base Address (ABAR). According to the
      'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
      specification, section 2.1.11, 'This register allocates space
      for the HBA memory registers'.
      The section 2.1.10, allows provision for Optional BARs which
      may support either memory or I/O spaces. However, in the context
      of the current SATA controller driver, which only ever access
      the ABAR, enabling I/O memory space is not required.            [SAMI]
    - Prefer to use // surrounding comments                           [ZENG]
    - Doing this would violate the edk2 coding standard. See EDK2
      Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
    
    v1:
    - Fix SATA Controller driver crash                                [SAMI]

 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 +++++++++++++++++++-
 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -2,6 +2,7 @@
   This driver module produces IDE_CONTROLLER_INIT protocol for Sata Controllers.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2018, ARM Ltd. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at
@@ -364,6 +365,7 @@ SataControllerStart (
   EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
   UINT32                            Data32;
   UINTN                             TotalCount;
+  UINT64                            PciAttributes;
 
   DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
 
@@ -406,6 +408,61 @@ SataControllerStart (
   Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
   Private->IdeInit.SetTiming      = IdeInitSetTiming;
   Private->IdeInit.EnumAll        = SATA_ENUMER_ALL;
+  Private->PciAttributesChanged   = FALSE;
+
+  // Save original PCI attributes
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationGet,
+                    0,
+                    &Private->OriginalPciAttributes
+                    );
+  if (EFI_ERROR (Status)) {
+      goto Done;
+  }
+
+  DEBUG ((
+    EFI_D_INFO,
+    "Original PCI Attributes = 0x%llx\n",
+    Private->OriginalPciAttributes
+    ));
+
+  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationSupported,
+                      0,
+                      &PciAttributes
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+
+    DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", PciAttributes));
+
+    if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+      DEBUG ((
+        EFI_D_ERROR,
+        "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
+        ));
+      Status = EFI_UNSUPPORTED;
+      goto Done;
+    }
+
+    PciAttributes = Private->OriginalPciAttributes | EFI_PCI_IO_ATTRIBUTE_MEMORY;
+
+    DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationEnable,
+                      PciAttributes,
+                      NULL
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+    Private->PciAttributesChanged = TRUE;
+  }
 
   Status = PciIo->Pci.Read (
                         PciIo,
@@ -414,7 +471,10 @@ SataControllerStart (
                         sizeof (PciData.Hdr.ClassCode),
                         PciData.Hdr.ClassCode
                         );
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    ASSERT (FALSE);
+    goto Done;
+  }
 
   if (IS_PCI_IDE (&PciData)) {
     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
@@ -481,6 +541,15 @@ Done:
       if (Private->IdentifyValid != NULL) {
         FreePool (Private->IdentifyValid);
       }
+      if (Private->PciAttributesChanged) {
+        // Restore original PCI attributes
+        PciIo->Attributes (
+                 PciIo,
+                 EfiPciIoAttributeOperationSet,
+                 Private->OriginalPciAttributes,
+                 NULL
+                 );
+      }
       FreePool (Private);
     }
   }
@@ -556,6 +625,15 @@ SataControllerStop (
     if (Private->IdentifyValid != NULL) {
       FreePool (Private->IdentifyValid);
     }
+    if (Private->PciAttributesChanged) {
+      // Restore original PCI attributes
+      Private->PciIo->Attributes (
+                        Private->PciIo,
+                        EfiPciIoAttributeOperationSet,
+                        Private->OriginalPciAttributes,
+                        NULL
+                        );
+    }
     FreePool (Private);
   }
 
diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
index f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..f927c1f02713771f92e3b76012848206321391f0 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
@@ -2,6 +2,7 @@
   Header file for Sata Controller driver.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017, ARM Ltd. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at
@@ -104,6 +105,12 @@ typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
   //
   EFI_IDENTIFY_DATA                 *IdentifyData;
   BOOLEAN                           *IdentifyValid;
+
+  /// Track the state so that the PCI attributes that were modified
+  /// can be restored to the original value later.
+  BOOLEAN                           PciAttributesChanged;
+  /// Copy of the original PCI Attributes
+  UINT64                            OriginalPciAttributes;
 } EFI_SATA_CONTROLLER_PRIVATE_DATA;
 
 #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE)
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
> The SATA controller driver crashes while accessing the
> PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
> space is not enabled.
>
> Enable the PCI memory space access to prevent the SATA
> Controller driver from crashing.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> The changes can be viewed at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2
>
> Notes:
>     v2:
>     - Improved log message and code documentation based on feedback   [SAMI]
>     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
>     - This SATA Controller driver only uses the PCI BAR5 register
>       space which is the AHCI Base Address (ABAR). According to the
>       'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
>       specification, section 2.1.11, 'This register allocates space
>       for the HBA memory registers'.
>       The section 2.1.10, allows provision for Optional BARs which
>       may support either memory or I/O spaces. However, in the context
>       of the current SATA controller driver, which only ever access
>       the ABAR, enabling I/O memory space is not required.            [SAMI]
>     - Prefer to use // surrounding comments                           [ZENG]
>     - Doing this would violate the edk2 coding standard. See EDK2
>       Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
>

Please stop obsessing about the coding standard. The maintainer was
quite clear what he wanted, and in the past, I have also indicated
that for the ARM related packages, I prefer readability and
consistency over adherence to a dubious standard.


>     v1:
>     - Fix SATA Controller driver crash                                [SAMI]
>
>  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 +++++++++++++++++++-
>  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
>  2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> index a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318 100644
> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> @@ -2,6 +2,7 @@
>    This driver module produces IDE_CONTROLLER_INIT protocol for Sata Controllers.
>
>    Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2018, ARM Ltd. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
>    which accompanies this distribution.  The full text of the license may be found at
> @@ -364,6 +365,7 @@ SataControllerStart (
>    EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
>    UINT32                            Data32;
>    UINTN                             TotalCount;
> +  UINT64                            PciAttributes;
>
>    DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
>
> @@ -406,6 +408,61 @@ SataControllerStart (
>    Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
>    Private->IdeInit.SetTiming      = IdeInitSetTiming;
>    Private->IdeInit.EnumAll        = SATA_ENUMER_ALL;
> +  Private->PciAttributesChanged   = FALSE;
> +
> +  // Save original PCI attributes
> +  Status = PciIo->Attributes (
> +                    PciIo,
> +                    EfiPciIoAttributeOperationGet,
> +                    0,
> +                    &Private->OriginalPciAttributes
> +                    );
> +  if (EFI_ERROR (Status)) {
> +      goto Done;
> +  }
> +
> +  DEBUG ((
> +    EFI_D_INFO,
> +    "Original PCI Attributes = 0x%llx\n",
> +    Private->OriginalPciAttributes
> +    ));
> +
> +  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +    Status = PciIo->Attributes (
> +                      PciIo,
> +                      EfiPciIoAttributeOperationSupported,
> +                      0,
> +                      &PciAttributes
> +                      );
> +    if (EFI_ERROR (Status)) {
> +      goto Done;
> +    }
> +
> +    DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", PciAttributes));
> +
> +    if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +      DEBUG ((
> +        EFI_D_ERROR,
> +        "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
> +        ));
> +      Status = EFI_UNSUPPORTED;
> +      goto Done;
> +    }
> +
> +    PciAttributes = Private->OriginalPciAttributes | EFI_PCI_IO_ATTRIBUTE_MEMORY;
> +
> +    DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
> +    Status = PciIo->Attributes (
> +                      PciIo,
> +                      EfiPciIoAttributeOperationEnable,
> +                      PciAttributes,
> +                      NULL
> +                      );
> +    if (EFI_ERROR (Status)) {
> +      goto Done;
> +    }
> +    Private->PciAttributesChanged = TRUE;
> +  }
>
>    Status = PciIo->Pci.Read (
>                          PciIo,
> @@ -414,7 +471,10 @@ SataControllerStart (
>                          sizeof (PciData.Hdr.ClassCode),
>                          PciData.Hdr.ClassCode
>                          );
> -  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    goto Done;
> +  }
>
>    if (IS_PCI_IDE (&PciData)) {
>      Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
> @@ -481,6 +541,15 @@ Done:
>        if (Private->IdentifyValid != NULL) {
>          FreePool (Private->IdentifyValid);
>        }
> +      if (Private->PciAttributesChanged) {
> +        // Restore original PCI attributes
> +        PciIo->Attributes (
> +                 PciIo,
> +                 EfiPciIoAttributeOperationSet,
> +                 Private->OriginalPciAttributes,
> +                 NULL
> +                 );
> +      }
>        FreePool (Private);
>      }
>    }
> @@ -556,6 +625,15 @@ SataControllerStop (
>      if (Private->IdentifyValid != NULL) {
>        FreePool (Private->IdentifyValid);
>      }
> +    if (Private->PciAttributesChanged) {
> +      // Restore original PCI attributes
> +      Private->PciIo->Attributes (
> +                        Private->PciIo,
> +                        EfiPciIoAttributeOperationSet,
> +                        Private->OriginalPciAttributes,
> +                        NULL
> +                        );
> +    }
>      FreePool (Private);
>    }
>
> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
> index f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..f927c1f02713771f92e3b76012848206321391f0 100644
> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
> @@ -2,6 +2,7 @@
>    Header file for Sata Controller driver.
>
>    Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2017, ARM Ltd. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
>    which accompanies this distribution.  The full text of the license may be found at
> @@ -104,6 +105,12 @@ typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
>    //
>    EFI_IDENTIFY_DATA                 *IdentifyData;
>    BOOLEAN                           *IdentifyValid;
> +
> +  /// Track the state so that the PCI attributes that were modified
> +  /// can be restored to the original value later.
> +  BOOLEAN                           PciAttributesChanged;
> +  /// Copy of the original PCI Attributes
> +  UINT64                            OriginalPciAttributes;
>  } EFI_SATA_CONTROLLER_PRIVATE_DATA;
>
>  #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE)
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Ard, Zeng

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: 15 June 2018 15:17
> To: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric Dong
> <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>;
> Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd
> <Evan.Lloyd@arm.com>; Thomas Abraham <thomas.abraham@arm.com>;
> nd <nd@arm.com>
> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
> space
> 
> On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
> > The SATA controller driver crashes while accessing the PCI memory
> > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
> >
> > Enable the PCI memory space access to prevent the SATA Controller
> > driver from crashing.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > ---
> > The changes can be viewed at
> >
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
> i
> > x_v2
> >
> > Notes:
> >     v2:
> >     - Improved log message and code documentation based on feedback
> [SAMI]
> >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
> >     - This SATA Controller driver only uses the PCI BAR5 register
> >       space which is the AHCI Base Address (ABAR). According to the
> >       'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
> >       specification, section 2.1.11, 'This register allocates space
> >       for the HBA memory registers'.
> >       The section 2.1.10, allows provision for Optional BARs which
> >       may support either memory or I/O spaces. However, in the context
> >       of the current SATA controller driver, which only ever access
> >       the ABAR, enabling I/O memory space is not required.            [SAMI]
> >     - Prefer to use // surrounding comments                           [ZENG]
> >     - Doing this would violate the edk2 coding standard. See EDK2
> >       Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
> >
> 
> Please stop obsessing about the coding standard. The maintainer was quite
> clear what he wanted, and in the past, I have also indicated that for the ARM
> related packages, I prefer readability and consistency over adherence to a
> dubious standard.

[[Evan Lloyd]] I'd like to make some points here:
1.	It is perfectly reasonable for Sami to explain why he has done something a certain way - Zeng is then at liberty to respond with his preference, but we do not (yet) know what that might be.  

2.	"readability and consistency" is the very purpose of any coding standard.  If consistency is valuable, doesn't that apply across modules?  I understand that it may be particularly valuable for maintainers within their modules, but the rest of us benefit from a consistent style - especially when looking outside our normal demesne.

3.	Applying a consistent Coding Standard across the whole project is a necessary pre-condition for automated CI checks on new submissions.  I'd like to aim e.g. for an improved patchcheck.py, but that requires consistency across modules, at least for new code.

Note: I am not disputing the dubiosity of the  CCS.  It could be improved (a lot).  However it is all we have right now.

Regards,
Evan

> 
> 
> >     v1:
> >     - Fix SATA Controller driver crash                                [SAMI]
...
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 15 June 2018 at 17:26, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Hi Ard, Zeng
>
>> -----Original Message-----
>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Sent: 15 June 2018 15:17
>> To: Sami Mujawar <Sami.Mujawar@arm.com>
>> Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric Dong
>> <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif Lindholm
>> <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>;
>> Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd
>> <Evan.Lloyd@arm.com>; Thomas Abraham <thomas.abraham@arm.com>;
>> nd <nd@arm.com>
>> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
>> space
>>
>> On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
>> > The SATA controller driver crashes while accessing the PCI memory
>> > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
>> >
>> > Enable the PCI memory space access to prevent the SATA Controller
>> > driver from crashing.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> > ---
>> > The changes can be viewed at
>> >
>> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
>> i
>> > x_v2
>> >
>> > Notes:
>> >     v2:
>> >     - Improved log message and code documentation based on feedback
>> [SAMI]
>> >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
>> >     - This SATA Controller driver only uses the PCI BAR5 register
>> >       space which is the AHCI Base Address (ABAR). According to the
>> >       'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
>> >       specification, section 2.1.11, 'This register allocates space
>> >       for the HBA memory registers'.
>> >       The section 2.1.10, allows provision for Optional BARs which
>> >       may support either memory or I/O spaces. However, in the context
>> >       of the current SATA controller driver, which only ever access
>> >       the ABAR, enabling I/O memory space is not required.            [SAMI]
>> >     - Prefer to use // surrounding comments                           [ZENG]
>> >     - Doing this would violate the edk2 coding standard. See EDK2
>> >       Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
>> >
>>
>> Please stop obsessing about the coding standard. The maintainer was quite
>> clear what he wanted, and in the past, I have also indicated that for the ARM
>> related packages, I prefer readability and consistency over adherence to a
>> dubious standard.
>
> [[Evan Lloyd]] I'd like to make some points here:
> 1.      It is perfectly reasonable for Sami to explain why he has done something a certain way - Zeng is then at liberty to respond with his preference, but we do not (yet) know what that might be.
>

Yes, we do. He already told us his preference.

> 2.      "readability and consistency" is the very purpose of any coding standard.  If consistency is valuable, doesn't that apply across modules?  I understand that it may be particularly valuable for maintainers within their modules, but the rest of us benefit from a consistent style - especially when looking outside our normal demesne.
>
> 3.      Applying a consistent Coding Standard across the whole project is a necessary pre-condition for automated CI checks on new submissions.  I'd like to aim e.g. for an improved patchcheck.py, but that requires consistency across modules, at least for new code.
>
> Note: I am not disputing the dubiosity of the  CCS.  It could be improved (a lot).  However it is all we have right now.
>

OK, so you are arguing that we should adhere to the letter of the CCS
even though there is consensus about its shortcomings and its
detachment from reality? I think that is a waste of everybody's time.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On Fri, Jun 15, 2018 at 03:26:54PM +0000, Evan Lloyd wrote:
> Hi Ard, Zeng
> 
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: 15 June 2018 15:17
> > To: Sami Mujawar <Sami.Mujawar@arm.com>
> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric Dong
> > <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Matteo Carlini <Matteo.Carlini@arm.com>;
> > Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd
> > <Evan.Lloyd@arm.com>; Thomas Abraham <thomas.abraham@arm.com>;
> > nd <nd@arm.com>
> > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
> > space
> > 
> > On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
> > > The SATA controller driver crashes while accessing the PCI memory
> > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
> > >
> > > Enable the PCI memory space access to prevent the SATA Controller
> > > driver from crashing.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > > ---
> > > The changes can be viewed at
> > >
> > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
> > i
> > > x_v2
> > >
> > > Notes:
> > >     v2:
> > >     - Improved log message and code documentation based on feedback
> > [SAMI]
> > >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
> > >     - This SATA Controller driver only uses the PCI BAR5 register
> > >       space which is the AHCI Base Address (ABAR). According to the
> > >       'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
> > >       specification, section 2.1.11, 'This register allocates space
> > >       for the HBA memory registers'.
> > >       The section 2.1.10, allows provision for Optional BARs which
> > >       may support either memory or I/O spaces. However, in the context
> > >       of the current SATA controller driver, which only ever access
> > >       the ABAR, enabling I/O memory space is not required.            [SAMI]
> > >     - Prefer to use // surrounding comments                           [ZENG]
> > >     - Doing this would violate the edk2 coding standard. See EDK2
> > >       Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
> > >
> > 
> > Please stop obsessing about the coding standard. The maintainer was quite
> > clear what he wanted, and in the past, I have also indicated that for the ARM
> > related packages, I prefer readability and consistency over adherence to a
> > dubious standard.
> 
> [[Evan Lloyd]] I'd like to make some points here:
> 1.	It is perfectly reasonable for Sami to explain why he has done
>   	something a certain way - Zeng is then at liberty to respond
>   	with his preference, but we do not (yet) know what that might
>   	be.

Yes we do. Zeng responded with that in the first instance. Sami then
disputed that explicitly stated preference, referring to the coding
standard. There was no lack of clarity in what Zeng wanted - so I
remain unclear on what this is intending to achieve?

> 2.	"readability and consistency" is the very purpose of any
>   	coding standard.  If consistency is valuable, doesn't that
>   	apply across modules?  I understand that it may be
>   	particularly valuable for maintainers within their modules,
>   	but the rest of us benefit from a consistent style -
>   	especially when looking outside our normal demesne.

At which point the rule of thumb is: aligning with the immediately
surrounding code always trumps adhering to the specified style.
Which is in itself trumped by the maintainer explicitly stating
another preference. (Like here.)

> 3.	Applying a consistent Coding Standard across the whole project
>   	is a necessary pre-condition for automated CI checks on new
>   	submissions.  I'd like to aim e.g. for an improved
>   	patchcheck.py, but that requires consistency across modules,
>   	at least for new code.

Such a system will _never_ be fully automatable.

I will _always_ take a 80 < x < 90 line over one that breaks up an
output string or one that reduces readability more than having to
side-scroll does.

That doesn't make it worthless, far from it.

> Note: I am not disputing the dubiosity of the  CCS.  It could be
> improved (a lot).  However it is all we have right now.

Then a more constructive approach is to recognise that not a single
one of the maintainers are appearing to be respecting the horror vacui
rule and submit a patch against
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification
to have section 6.2.3 deleted.

(Much like I should at some point get around to have 5.4.2.2.2
deleted, or at least conditionalised to only apply for !ARM.)

Regards,

Leif


> Regards,
> Evan
> 
> > 
> > 
> > >     v1:
> > >     - Fix SATA Controller driver crash                                [SAMI]
> ...
> > > --
> > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > >
> > >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Leif,

I totally agree with you.
Before we have clear direction about this to align code and CCS spec. I prefer to align with the immediately surrounding code.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif Lindholm
Sent: Saturday, June 16, 2018 12:13 AM
To: Evan Lloyd <Evan.Lloyd@arm.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

On Fri, Jun 15, 2018 at 03:26:54PM +0000, Evan Lloyd wrote:
> Hi Ard, Zeng
> 
> > -----Original Message-----
> > From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Sent: 15 June 2018 15:17
> > To: Sami Mujawar <Sami.Mujawar@arm.com>
> > Cc: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Eric 
> > Dong <eric.dong@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>; Leif 
> > Lindholm <leif.lindholm@linaro.org>; Matteo Carlini 
> > <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt 
> > <Stephanie.Hughes-Fitt@arm.com>; Evan Lloyd <Evan.Lloyd@arm.com>; 
> > Thomas Abraham <thomas.abraham@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem 
> > space
> > 
> > On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
> > > The SATA controller driver crashes while accessing the PCI memory 
> > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
> > >
> > > Enable the PCI memory space access to prevent the SATA Controller 
> > > driver from crashing.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > > ---
> > > The changes can be viewed at
> > >
> > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_
> > f
> > i
> > > x_v2
> > >
> > > Notes:
> > >     v2:
> > >     - Improved log message and code documentation based on 
> > > feedback
> > [SAMI]
> > >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
> > >     - This SATA Controller driver only uses the PCI BAR5 register
> > >       space which is the AHCI Base Address (ABAR). According to the
> > >       'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
> > >       specification, section 2.1.11, 'This register allocates space
> > >       for the HBA memory registers'.
> > >       The section 2.1.10, allows provision for Optional BARs which
> > >       may support either memory or I/O spaces. However, in the context
> > >       of the current SATA controller driver, which only ever access
> > >       the ABAR, enabling I/O memory space is not required.            [SAMI]
> > >     - Prefer to use // surrounding comments                           [ZENG]
> > >     - Doing this would violate the edk2 coding standard. See EDK2
> > >       Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
> > >
> > 
> > Please stop obsessing about the coding standard. The maintainer was 
> > quite clear what he wanted, and in the past, I have also indicated 
> > that for the ARM related packages, I prefer readability and 
> > consistency over adherence to a dubious standard.
> 
> [[Evan Lloyd]] I'd like to make some points here:
> 1.	It is perfectly reasonable for Sami to explain why he has done
>   	something a certain way - Zeng is then at liberty to respond
>   	with his preference, but we do not (yet) know what that might
>   	be.

Yes we do. Zeng responded with that in the first instance. Sami then disputed that explicitly stated preference, referring to the coding standard. There was no lack of clarity in what Zeng wanted - so I remain unclear on what this is intending to achieve?

> 2.	"readability and consistency" is the very purpose of any
>   	coding standard.  If consistency is valuable, doesn't that
>   	apply across modules?  I understand that it may be
>   	particularly valuable for maintainers within their modules,
>   	but the rest of us benefit from a consistent style -
>   	especially when looking outside our normal demesne.

At which point the rule of thumb is: aligning with the immediately surrounding code always trumps adhering to the specified style.
Which is in itself trumped by the maintainer explicitly stating another preference. (Like here.)

> 3.	Applying a consistent Coding Standard across the whole project
>   	is a necessary pre-condition for automated CI checks on new
>   	submissions.  I'd like to aim e.g. for an improved
>   	patchcheck.py, but that requires consistency across modules,
>   	at least for new code.

Such a system will _never_ be fully automatable.

I will _always_ take a 80 < x < 90 line over one that breaks up an output string or one that reduces readability more than having to side-scroll does.

That doesn't make it worthless, far from it.

> Note: I am not disputing the dubiosity of the  CCS.  It could be 
> improved (a lot).  However it is all we have right now.

Then a more constructive approach is to recognise that not a single one of the maintainers are appearing to be respecting the horror vacui rule and submit a patch against https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification
to have section 6.2.3 deleted.

(Much like I should at some point get around to have 5.4.2.2.2 deleted, or at least conditionalised to only apply for !ARM.)

Regards,

Leif


> Regards,
> Evan
> 
> > 
> > 
> > >     v1:
> > >     - Fix SATA Controller driver crash                                [SAMI]
> ...
> > > --
> > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > >
> > >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

off-topic:

On 06/15/18 16:13, Sami Mujawar wrote:
>     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]

My understanding is that "Star" is Star's given name, and "Zeng" is his
surname. Let's not start calling each other by surname, shall we?

(Email addresses under @intel.com seem to follow the
"given-name.surname" or "given-name.middle-initial.surname" pattern.)

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Laszlo, Star.
Looking at the e-mail thread, I think I also made the mistake of getting Star's name inverted.
I'm sorry, especially as in this case it is not as unclear as some Chinese names can be for foreigners.  I can only put this one down to approaching senility.
 Sorry, Star - I'll try not to do it again.

Evan

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo
> Ersek
> Sent: 18 June 2018 16:52
> To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
> Cc: ruiyu.ni@intel.com; Stephanie Hughes-Fitt <Stephanie.Hughes-
> Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org;
> leif.lindholm@linaro.org; nd <nd@arm.com>; star.zeng@intel.com
> Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI
> mem space
> 
> off-topic:
> 
> On 06/15/18 16:13, Sami Mujawar wrote:
> >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
> 
> My understanding is that "Star" is Star's given name, and "Zeng" is his
> surname. Let's not start calling each other by surname, shall we?
> 
> (Email addresses under @intel.com seem to follow the "given-
> name.surname" or "given-name.middle-initial.surname" pattern.)
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Never mind. :)

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Evan Lloyd
Sent: Tuesday, June 19, 2018 1:19 AM
To: Laszlo Ersek <lersek@redhat.com>; Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Dong, Eric <eric.dong@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; nd <nd@arm.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

Hi Laszlo, Star.
Looking at the e-mail thread, I think I also made the mistake of getting Star's name inverted.
I'm sorry, especially as in this case it is not as unclear as some Chinese names can be for foreigners.  I can only put this one down to approaching senility.
 Sorry, Star - I'll try not to do it again.

Evan

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo 
> Ersek
> Sent: 18 June 2018 16:52
> To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
> Cc: ruiyu.ni@intel.com; Stephanie Hughes-Fitt <Stephanie.Hughes- 
> Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org; 
> leif.lindholm@linaro.org; nd <nd@arm.com>; star.zeng@intel.com
> Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller 
> PCI mem space
> 
> off-topic:
> 
> On 06/15/18 16:13, Sami Mujawar wrote:
> >     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
> 
> My understanding is that "Star" is Star's given name, and "Zeng" is 
> his surname. Let's not start calling each other by surname, shall we?
> 
> (Email addresses under @intel.com seem to follow the "given- 
> name.surname" or "given-name.middle-initial.surname" pattern.)
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Star,

My sincere apologies for getting your name wrong. For some reason my email client displays your name as 'surname, name'. However, it is still my fault.
I will ensure I address you correctly in the future.

Hi Lazlo,
Thanks for letting me know.

Regards,

Sami Mujawar

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: 18 June 2018 04:52 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
Cc: ruiyu.ni@intel.com; nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; star.zeng@intel.com
Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

off-topic:

On 06/15/18 16:13, Sami Mujawar wrote:
>     - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]

My understanding is that "Star" is Star's given name, and "Zeng" is his surname. Let's not start calling each other by surname, shall we?

(Email addresses under @intel.com seem to follow the "given-name.surname" or "given-name.middle-initial.surname" pattern.)

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Sami,

I will have feedback in the V1 patch thread based on your response there.

Thanks,
Star
-----Original Message-----
From: Sami Mujawar [mailto:sami.mujawar@arm.com] 
Sent: Friday, June 15, 2018 10:14 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; evan.lloyd@arm.com; thomas.abraham@arm.com; nd@arm.com
Subject: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

The SATA controller driver crashes while accessing the PCI memory [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.

Enable the PCI memory space access to prevent the SATA Controller driver from crashing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
The changes can be viewed at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2

Notes:
    v2:
    - Improved log message and code documentation based on feedback   [SAMI]
    - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
    - This SATA Controller driver only uses the PCI BAR5 register
      space which is the AHCI Base Address (ABAR). According to the
      'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
      specification, section 2.1.11, 'This register allocates space
      for the HBA memory registers'.
      The section 2.1.10, allows provision for Optional BARs which
      may support either memory or I/O spaces. However, in the context
      of the current SATA controller driver, which only ever access
      the ABAR, enabling I/O memory space is not required.            [SAMI]
    - Prefer to use // surrounding comments                           [ZENG]
    - Doing this would violate the edk2 coding standard. See EDK2
      Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
    
    v1:
    - Fix SATA Controller driver crash                                [SAMI]

 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 +++++++++++++++++++-  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -2,6 +2,7 @@
   This driver module produces IDE_CONTROLLER_INIT protocol for Sata Controllers.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2018, ARM Ltd. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at @@ -364,6 +365,7 @@ SataControllerStart (
   EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
   UINT32                            Data32;
   UINTN                             TotalCount;
+  UINT64                            PciAttributes;
 
   DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
 
@@ -406,6 +408,61 @@ SataControllerStart (
   Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
   Private->IdeInit.SetTiming      = IdeInitSetTiming;
   Private->IdeInit.EnumAll        = SATA_ENUMER_ALL;
+  Private->PciAttributesChanged   = FALSE;
+
+  // Save original PCI attributes
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationGet,
+                    0,
+                    &Private->OriginalPciAttributes
+                    );
+  if (EFI_ERROR (Status)) {
+      goto Done;
+  }
+
+  DEBUG ((
+    EFI_D_INFO,
+    "Original PCI Attributes = 0x%llx\n",
+    Private->OriginalPciAttributes
+    ));
+
+  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationSupported,
+                      0,
+                      &PciAttributes
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+
+    DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", 
+ PciAttributes));
+
+    if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+      DEBUG ((
+        EFI_D_ERROR,
+        "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
+        ));
+      Status = EFI_UNSUPPORTED;
+      goto Done;
+    }
+
+    PciAttributes = Private->OriginalPciAttributes | 
+ EFI_PCI_IO_ATTRIBUTE_MEMORY;
+
+    DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationEnable,
+                      PciAttributes,
+                      NULL
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+    Private->PciAttributesChanged = TRUE;  }
 
   Status = PciIo->Pci.Read (
                         PciIo,
@@ -414,7 +471,10 @@ SataControllerStart (
                         sizeof (PciData.Hdr.ClassCode),
                         PciData.Hdr.ClassCode
                         );
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    ASSERT (FALSE);
+    goto Done;
+  }
 
   if (IS_PCI_IDE (&PciData)) {
     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL; @@ -481,6 +541,15 @@ Done:
       if (Private->IdentifyValid != NULL) {
         FreePool (Private->IdentifyValid);
       }
+      if (Private->PciAttributesChanged) {
+        // Restore original PCI attributes
+        PciIo->Attributes (
+                 PciIo,
+                 EfiPciIoAttributeOperationSet,
+                 Private->OriginalPciAttributes,
+                 NULL
+                 );
+      }
       FreePool (Private);
     }
   }
@@ -556,6 +625,15 @@ SataControllerStop (
     if (Private->IdentifyValid != NULL) {
       FreePool (Private->IdentifyValid);
     }
+    if (Private->PciAttributesChanged) {
+      // Restore original PCI attributes
+      Private->PciIo->Attributes (
+                        Private->PciIo,
+                        EfiPciIoAttributeOperationSet,
+                        Private->OriginalPciAttributes,
+                        NULL
+                        );
+    }
     FreePool (Private);
   }
 
diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
index f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..f927c1f02713771f92e3b76012848206321391f0 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
@@ -2,6 +2,7 @@
   Header file for Sata Controller driver.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017, ARM Ltd. All rights reserved.<BR>
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
   which accompanies this distribution.  The full text of the license may be found at @@ -104,6 +105,12 @@ typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
   //
   EFI_IDENTIFY_DATA                 *IdentifyData;
   BOOLEAN                           *IdentifyValid;
+
+  /// Track the state so that the PCI attributes that were modified  
+ /// can be restored to the original value later.
+  BOOLEAN                           PciAttributesChanged;
+  /// Copy of the original PCI Attributes
+  UINT64                            OriginalPciAttributes;
 } EFI_SATA_CONTROLLER_PRIVATE_DATA;
 
 #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE)
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel