[PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Ruiyu Ni]

The patches caused Windows 10 S4 resume failure.
Considering the similar changes are reverted from PciBus driver,
revert the patches from AtaAtapiPassThru as well.

Ruiyu Ni (2):
  MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
  MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +---------------------
 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
 2 files changed, 1 insertion(+), 62 deletions(-)

-- 
2.15.0.gvfs.1.preview.4

[PATCH 1/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master

[Ruiyu Ni]

This patch caused Windows 10 S4 resume failure.
Considering the similar changes are reverted from PciBus driver,
revert the patch from AtaAtapiPassThru as well.

Revert "MdeModulePkg/AtaAtapiPassThru: disable only BM-DMA at
ExitBootServices()"
This reverts commit 76fd5a660d704538a1b14a58d03a4eef9682b01c.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 5 +++--
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index e10e0d4e65..09064dda18 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -480,7 +480,8 @@ InitializeAtaAtapiPassThru (
 }
 
 /**
-  Disable Bus Master DMA on the device when exiting the boot services.
+  Disable the device (especially Bus Master DMA) when exiting the boot
+  services.
 
   @param[in] Event    Event for which this notification function is being
                       called.
@@ -505,7 +506,7 @@ AtaPassThruExitBootServices (
   PciIo->Attributes (
            PciIo,
            EfiPciIoAttributeOperationDisable,
-           Instance->EnabledPciAttributes & EFI_PCI_IO_ATTRIBUTE_BUS_MASTER,
+           Instance->EnabledPciAttributes,
            NULL
            );
 }
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 92c5bf2001..8d6eac706c 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -123,7 +123,8 @@ typedef struct {
   LIST_ENTRY                        NonBlockingTaskList;
 
   //
-  // For disabling Bus Master DMA on the device at ExitBootServices().
+  // For disabling the device (especially Bus Master DMA) at
+  // ExitBootServices().
   //
   EFI_EVENT                         ExitBootEvent;
 } ATA_ATAPI_PASS_THRU_INSTANCE;
-- 
2.15.0.gvfs.1.preview.4

[PATCH 2/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Ruiyu Ni]

This patch caused Windows 10 S4 resume failure.
Considering the similar changes are reverted from PciBus driver,
revert the patch from AtaAtapiPassThru as well.

Revert "MdeModulePkg/AtaAtapiPassThru: disable the device
at ExitBootServices()"

This reverts commit 6fb8ddd36bde45614b0a069528cdc97077835a74.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 59 +---------------------
 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  6 ---
 2 files changed, 1 insertion(+), 64 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 09064dda18..a48b295d26 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -104,8 +104,7 @@ ATA_ATAPI_PASS_THRU_INSTANCE gAtaAtapiPassThruInstanceTemplate = {
   {                   // NonBlocking TaskList
     NULL,
     NULL
-  },
-  NULL,               // ExitBootEvent
+  }
 };
 
 ATAPI_DEVICE_PATH    mAtapiDevicePathTemplate = {
@@ -479,38 +478,6 @@ InitializeAtaAtapiPassThru (
   return Status;
 }
 
-/**
-  Disable the device (especially Bus Master DMA) when exiting the boot
-  services.
-
-  @param[in] Event    Event for which this notification function is being
-                      called.
-  @param[in] Context  Pointer to the ATA_ATAPI_PASS_THRU_INSTANCE that
-                      represents the HBA.
-**/
-VOID
-EFIAPI
-AtaPassThruExitBootServices (
-  IN EFI_EVENT Event,
-  IN VOID      *Context
-  )
-{
-  ATA_ATAPI_PASS_THRU_INSTANCE *Instance;
-  EFI_PCI_IO_PROTOCOL          *PciIo;
-
-  DEBUG ((DEBUG_VERBOSE, "%a: Context=0x%p\n", __FUNCTION__, Context));
-
-  Instance = Context;
-  PciIo = Instance->PciIo;
-
-  PciIo->Attributes (
-           PciIo,
-           EfiPciIoAttributeOperationDisable,
-           Instance->EnabledPciAttributes,
-           NULL
-           );
-}
-
 /**
   Tests to see if this driver supports a given controller. If a child device is provided,
   it further tests to see if this driver supports creating a handle for the specified child device.
@@ -790,17 +757,6 @@ AtaAtapiPassThruStart (
   InitializeListHead(&Instance->DeviceList);
   InitializeListHead(&Instance->NonBlockingTaskList);
 
-  Status = gBS->CreateEvent (
-                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
-                  TPL_CALLBACK,
-                  AtaPassThruExitBootServices,
-                  Instance,
-                  &Instance->ExitBootEvent
-                  );
-  if (EFI_ERROR (Status)) {
-    goto ErrorExit;
-  }
-
   Instance->TimerEvent = NULL;
 
   Status = gBS->CreateEvent (
@@ -854,10 +810,6 @@ ErrorExit:
     gBS->CloseEvent (Instance->TimerEvent);
   }
 
-  if ((Instance != NULL) && (Instance->ExitBootEvent != NULL)) {
-    gBS->CloseEvent (Instance->ExitBootEvent);
-  }
-
   //
   // Remove all inserted ATA devices.
   //
@@ -956,15 +908,6 @@ AtaAtapiPassThruStop (
     Instance->TimerEvent = NULL;
   }
   DestroyAsynTaskList (Instance, FALSE);
-
-  //
-  // Close event signaled at gBS->ExitBootServices().
-  //
-  if (Instance->ExitBootEvent != NULL) {
-    gBS->CloseEvent (Instance->ExitBootEvent);
-    Instance->ExitBootEvent = NULL;
-  }
-
   //
   // Free allocated resource
   //
diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
index 8d6eac706c..85e5a55539 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
@@ -121,12 +121,6 @@ typedef struct {
   //
   EFI_EVENT                         TimerEvent;
   LIST_ENTRY                        NonBlockingTaskList;
-
-  //
-  // For disabling the device (especially Bus Master DMA) at
-  // ExitBootServices().
-  //
-  EFI_EVENT                         ExitBootEvent;
 } ATA_ATAPI_PASS_THRU_INSTANCE;
 
 //
-- 
2.15.0.gvfs.1.preview.4

Re: [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Yao, Jiewen]

Both patches are reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Monday, November 27, 2017 9:20 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to
> disable PCI attributes
> 
> The patches caused Windows 10 S4 resume failure.
> Considering the similar changes are reverted from PciBus driver,
> revert the patches from AtaAtapiPassThru as well.
> 
> Ruiyu Ni (2):
>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes
> 
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +---------------------
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
>  2 files changed, 1 insertion(+), 62 deletions(-)
> 
> --
> 2.15.0.gvfs.1.preview.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Monday, November 27, 2017 9:20 AM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

The patches caused Windows 10 S4 resume failure.
Considering the similar changes are reverted from PciBus driver, revert the patches from AtaAtapiPassThru as well.

Ruiyu Ni (2):
  MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
  MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +---------------------
 .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
 2 files changed, 1 insertion(+), 62 deletions(-)

--
2.15.0.gvfs.1.preview.4

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

Re: [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Laszlo Ersek]

Hello Ray,

On 11/27/17 02:19, Ruiyu Ni wrote:
> The patches caused Windows 10 S4 resume failure.
> Considering the similar changes are reverted from PciBus driver,
> revert the patches from AtaAtapiPassThru as well.
> 
> Ruiyu Ni (2):
>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes
> 
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +---------------------
>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
>  2 files changed, 1 insertion(+), 62 deletions(-)
> 

it looks like these patches have not been committed yet, which is a good
thing, because apparently there's a better solution than a full revert.
Namely, in the other sub-thread at
<http://mid.mail-archive.com/0236afa2-e365-af7a-9374-7fd1ad742c36@redhat.com>
(alternative link:
<https://lists.01.org/pipermail/edk2-devel/2017-November/018046.html>),
Jiewen and Star seem to accept AhciReset() as a better way to abort
pending DMA.

This means that we need not revert the EBS-handler altogether, only
change what it does. Could you give that a try please?

(If the Windows regression is very urgent to fix, then I don't mind if
the Bus Master disabling is removed separately, before AhciReset() is
added; but in that case, a full revert looks unjustified, since the EBS
handler will have to be re-added for AhciReset() anyway.)

Thanks,
Laszlo

Re: [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Ni, Ruiyu]

Laszlo,
The Windows resume issue is urgent to fix.
I'd like to check-in the patches first.
If I didn't know the change may cause combability issue, I'd be very fine
to change it to AhciReset() then submit the patch, then after got a R-b,
I'd be very happy to check in that patch.
But since now I know there might be some combability issue, I don't
want to switch to AhciReset() solution without thoroughly testing.
And frankly I don't know what a thoroughly testing would be like.
There are many OSes and many PCI storage cards! ☹

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, November 27, 2017 10:10 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: edk2-devel@lists.01.org; Paolo Bonzini <pbonzini@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert
> patch to disable PCI attributes
> 
> Hello Ray,
> 
> On 11/27/17 02:19, Ruiyu Ni wrote:
> > The patches caused Windows 10 S4 resume failure.
> > Considering the similar changes are reverted from PciBus driver,
> > revert the patches from AtaAtapiPassThru as well.
> >
> > Ruiyu Ni (2):
> >   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
> >   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI
> > attributes
> >
> >  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +--------------------
> -
> >  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
> >  2 files changed, 1 insertion(+), 62 deletions(-)
> >
> 
> it looks like these patches have not been committed yet, which is a good
> thing, because apparently there's a better solution than a full revert.
> Namely, in the other sub-thread at
> <http://mid.mail-archive.com/0236afa2-e365-af7a-9374-
> 7fd1ad742c36@redhat.com>
> (alternative link:
> <https://lists.01.org/pipermail/edk2-devel/2017-November/018046.html>),
> Jiewen and Star seem to accept AhciReset() as a better way to abort pending
> DMA.
> 
> This means that we need not revert the EBS-handler altogether, only change
> what it does. Could you give that a try please?
> 
> (If the Windows regression is very urgent to fix, then I don't mind if the Bus
> Master disabling is removed separately, before AhciReset() is added; but in
> that case, a full revert looks unjustified, since the EBS handler will have to be
> re-added for AhciReset() anyway.)
> 
> Thanks,
> Laszlo

Re: [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI attributes

[Laszlo Ersek]

On 11/28/17 08:39, Ni, Ruiyu wrote:
> Laszlo,
> The Windows resume issue is urgent to fix.
> I'd like to check-in the patches first.
> If I didn't know the change may cause combability issue, I'd be very fine
> to change it to AhciReset() then submit the patch, then after got a R-b,
> I'd be very happy to check in that patch.
> But since now I know there might be some combability issue, I don't
> want to switch to AhciReset() solution without thoroughly testing.
> And frankly I don't know what a thoroughly testing would be like.
> There are many OSes and many PCI storage cards! ☹

Fair enough; thank you for the explanation.

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

Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, November 27, 2017 10:10 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Cc: edk2-devel@lists.01.org; Paolo Bonzini <pbonzini@redhat.com>; Yao,
>> Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [edk2] [PATCH 0/2] MdeModulePkg/AtaAtapiPassThru: Revert
>> patch to disable PCI attributes
>>
>> Hello Ray,
>>
>> On 11/27/17 02:19, Ruiyu Ni wrote:
>>> The patches caused Windows 10 S4 resume failure.
>>> Considering the similar changes are reverted from PciBus driver,
>>> revert the patches from AtaAtapiPassThru as well.
>>>
>>> Ruiyu Ni (2):
>>>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable Bus Master
>>>   MdeModulePkg/AtaAtapiPassThru: Revert patch to disable PCI
>>> attributes
>>>
>>>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 58 +--------------------
>> -
>>>  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  5 --
>>>  2 files changed, 1 insertion(+), 62 deletions(-)
>>>
>>
>> it looks like these patches have not been committed yet, which is a good
>> thing, because apparently there's a better solution than a full revert.
>> Namely, in the other sub-thread at
>> <http://mid.mail-archive.com/0236afa2-e365-af7a-9374-
>> 7fd1ad742c36@redhat.com>
>> (alternative link:
>> <https://lists.01.org/pipermail/edk2-devel/2017-November/018046.html>),
>> Jiewen and Star seem to accept AhciReset() as a better way to abort pending
>> DMA.
>>
>> This means that we need not revert the EBS-handler altogether, only change
>> what it does. Could you give that a try please?
>>
>> (If the Windows regression is very urgent to fix, then I don't mind if the Bus
>> Master disabling is removed separately, before AhciReset() is added; but in
>> that case, a full revert looks unjustified, since the EBS handler will have to be
>> re-added for AhciReset() anyway.)
>>
>> Thanks,
>> Laszlo