Re: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS per policy

["Zeng, Star" <star.zeng@intel.com>]

Fine.


I just had the thought from my first view to the code.
When I saw the name AhciPuisEnable, I thought there should be AhciPuisDisable as well, but I did not find it.
Then I must check the comments and function header to know the details.



Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Tuesday, June 5, 2018 1:40 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Chiu, Chasel <chasel.chiu@intel.com>
Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: enable/disable PUIS per policy

I see your point.
I prefer to use AhciPuisEnable().
The function name describe the purpose.
The function body explains the details.


Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, June 5, 2018 1:17 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> enable/disable PUIS per policy
> 
> Yes, that is why I was saying to use AhciSetFeaturePuis as the function name.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, June 5, 2018 11:41 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Chiu, Chasel <chasel.chiu@intel.com>
> Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> enable/disable PUIS per policy
> 
> The Set Feature cmd is used to enable/disable PUIS.
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, June 5, 2018 11:35 AM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zeng, Star 
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru:
> > enable/disable PUIS per policy
> >
> > Reviewed-by: Star Zeng <star.zeng@intel.com>
> >
> > How about using function name AhciSetFeaturePuis instead of 
> > AhciPuisEnable as the function is not just to enable Puis?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Monday, June 4, 2018 3:04 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Chiu, Chasel 
> > <chasel.chiu@intel.com>
> > Subject: [PATCH v2 3/4] MdeModulePkg/AtaAtapiPassThru: 
> > enable/disable PUIS per policy
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Chasel Chiu <chasel.chiu@intel.com>
> > ---
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c   | 48
> > ++++++++++++++++++++++
> >  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c    | 19 ++++++++-
> >  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h    |  3 ++
> >  .../Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf  |  3 +-
> >  4 files changed, 71 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > index 14578c0f94..841b6a0e60 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
> > @@ -2316,6 +2316,38 @@ AhciSpinUpDisk (
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  Enable/disable/skip PUIS of the disk according to policy.
> > +
> > +  @param  PciIo               The PCI IO protocol instance.
> > +  @param  AhciRegisters       The pointer to the EFI_AHCI_REGISTERS.
> > +  @param  Port                The number of port.
> > +  @param  PortMultiplier      The multiplier of port.
> > +
> > +**/
> > +EFI_STATUS
> > +AhciPuisEnable (
> > +  IN EFI_PCI_IO_PROTOCOL           *PciIo,
> > +  IN EFI_AHCI_REGISTERS            *AhciRegisters,
> > +  IN UINT8                         Port,
> > +  IN UINT8                         PortMultiplier
> > +  )
> > +{
> > +  EFI_STATUS                       Status;
> > +
> > +  Status = EFI_SUCCESS;
> > +  if (mAtaAtapiPolicy->PuisEnable == 0) {
> > +    Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 
> > +PortMultiplier, ATA_SUB_CMD_DISABLE_PUIS, 0x00,
> > ATA_ATAPI_TIMEOUT);
> > +  } else if (mAtaAtapiPolicy->PuisEnable == 1) {
> > +    Status = AhciDeviceSetFeature (PciIo, AhciRegisters, Port, 
> > +PortMultiplier, ATA_SUB_CMD_ENABLE_PUIS, 0x00,
> > ATA_ATAPI_TIMEOUT);
> > +  }
> > +  DEBUG ((DEBUG_INFO, "%a PUIS feature at port [%d] PortMultiplier 
> > + [%d] -
> >  %r!\n",
> > +    (mAtaAtapiPolicy->PuisEnable == 0) ? "Disable" : (
> > +    (mAtaAtapiPolicy->PuisEnable == 1) ? "Enable" : "Skip"
> > +      ), Port, PortMultiplier, Status));
> > +  return Status;
> > +}
> > +
> >  /**
> >    Initialize ATA host controller at AHCI mode.
> >
> > @@ -2658,6 +2690,22 @@ AhciModeInitialization (
> >        if (DeviceType == EfiIdeHarddisk) {
> >          REPORT_STATUS_CODE (EFI_PROGRESS_CODE, 
> > (EFI_PERIPHERAL_FIXED_MEDIA | EFI_P_PC_ENABLE));
> >        }
> > +
> > +      //
> > +      // Enable/disable PUIS according to policy setting if PUIS is 
> > + capable
> > (Word[83].BIT5 is set).
> > +      //
> > +      if ((Buffer.AtaData.command_set_supported_83 & BIT5) != 0) {
> > +        Status = AhciPuisEnable (
> > +                   PciIo,
> > +                   AhciRegisters,
> > +                   Port,
> > +                   0
> > +                   );
> > +        if (EFI_ERROR (Status)) {
> > +          DEBUG ((DEBUG_ERROR, "PUIS enable/disable failed, Status 
> > + = %r\n",
> > Status));
> > +          continue;
> > +        }
> > +      }
> >      }
> >    }
> >
> > diff --git 
> > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > index a48b295d26..aab704bcd3 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > @@ -2,7 +2,7 @@
> >    This file implements ATA_PASSTHRU_PROCTOCOL and 
> > EXT_SCSI_PASSTHRU_PROTOCOL interfaces
> >    for managed ATA controllers.
> >
> > -  Copyright (c) 2010 - 2016, Intel Corporation. All rights 
> > reserved.<BR>
> > +  Copyright (c) 2010 - 2018, Intel Corporation. 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 @@ -142,6 +142,15 @@ UINT8
> mScsiId[TARGET_MAX_BYTES] = {
> >    0xFF, 0xFF, 0xFF, 0xFF
> >  };
> >
> > +EDKII_ATA_ATAPI_POLICY_PROTOCOL *mAtaAtapiPolicy; 
> > +EDKII_ATA_ATAPI_POLICY_PROTOCOL mDefaultAtaAtapiPolicy = {
> > +  EDKII_ATA_ATAPI_POLICY_VERSION,
> > +  2,  // PuisEnable
> > +  0,  // DeviceSleepEnable
> > +  0,  // AggressiveDeviceSleepEnable
> > +  0   // Reserved
> > +};
> > +
> >  /**
> >    Sends an ATA command to an ATA device that is attached to the ATA 
> > controller. This function
> >    supports both blocking I/O and non-blocking I/O. The blocking I/O 
> > functionality is required, @@ -739,6 +748,14 @@ AtaAtapiPassThruStart (
> >      goto ErrorExit;
> >    }
> >
> > +  Status = gBS->LocateProtocol (&gEdkiiAtaAtapiPolicyProtocolGuid,
> > + NULL, (VOID **)&mAtaAtapiPolicy);  if (EFI_ERROR (Status)) {
> > +    //
> > +    // If there is no AtaAtapiPolicy exposed, use the default policy.
> > +    //
> > +    mAtaAtapiPolicy = &mDefaultAtaAtapiPolicy;  }
> > +
> >    //
> >    // Allocate a buffer to store the ATA_ATAPI_PASS_THRU_INSTANCE 
> > data structure
> >    //
> > diff --git 
> > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> > index 31b005f2f6..b07bcbbb3e 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.h
> > @@ -24,6 +24,7 @@
> >  #include <Protocol/IdeControllerInit.h>  #include 
> > <Protocol/AtaPassThru.h> #include <Protocol/ScsiPassThruExt.h>
> > +#include <Protocol/AtaAtapiPolicy.h>
> >
> >  #include <Library/DebugLib.h>
> >  #include <Library/BaseLib.h>
> > @@ -45,6 +46,8 @@ extern EFI_DRIVER_BINDING_PROTOCOL 
> > gAtaAtapiPassThruDriverBinding;  extern
> EFI_COMPONENT_NAME_PROTOCOL
> > gAtaAtapiPassThruComponentName; extern
> EFI_COMPONENT_NAME2_PROTOCOL
> > gAtaAtapiPassThruComponentName2;
> >
> > +extern EDKII_ATA_ATAPI_POLICY_PROTOCOL *mAtaAtapiPolicy;
> > +
> >  #define ATA_ATAPI_PASS_THRU_SIGNATURE  SIGNATURE_32 ('a', 'a', 'p',
> 't')
> >  #define ATA_ATAPI_DEVICE_SIGNATURE     SIGNATURE_32 ('a', 'd', 'e', 'v')
> >  #define ATA_NONBLOCKING_TASK_SIGNATURE  SIGNATURE_32 ('a', 't', 
> > 's',
> > 'k') diff --git
> > a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > index 82d5f7a46c..d1ce859091 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
> > @@ -4,7 +4,7 @@
> >  #  This driver installs AtaPassThru and ExtScsiPassThru protocol in 
> > each ide/sata controller  #  to access to all attached Ata/Atapi devices.
> >  #
> > -#  Copyright (c) 2010 - 2014, Intel Corporation. All rights 
> > reserved.<BR>
> > +#  Copyright (c) 2010 - 2018, Intel Corporation. All rights 
> > +reserved.<BR>
> >  #
> >  #  This program and the accompanying materials  #  are licensed and 
> > made available under the terms and conditions of the BSD License @@
> > -67,6 +67,7 @@ [Protocols]
> >    gEfiIdeControllerInitProtocolGuid             ## TO_START
> >    gEfiDevicePathProtocolGuid                    ## TO_START
> >    gEfiPciIoProtocolGuid                         ## TO_START
> > +  gEdkiiAtaAtapiPolicyProtocolGuid              ## CONSUMES
> >
> >  [Pcd]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAtaSmartEnable   ##
> > SOMETIMES_CONSUMES
> > --
> > 2.16.1.windows.1