Re: [RFC PATCH v1 12/13] IB/hfi1: Use pci_disable/enable_link_state()

[Dean Luick]

On 6/2/2023 6:47 AM, Ilpo Järvinen wrote:
> IB/hfi1 driver adjusts ASPM state itself which leaves ASPM service
> driver in PCI core unaware of the link state changes the driver
> implemented.
>
> Call pci_disable_link_state() and pci_enable_link_state() instead of
> adjusting ASPMC field in LNKCTL directly in the driver and let PCI core
> handle the ASPM state management. Remove the functions that handled the
> ASPM changes that are now unnecessary.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  drivers/infiniband/hw/hfi1/aspm.c | 38 +++----------------------------
>  drivers/infiniband/hw/hfi1/pcie.c |  2 +-
>  2 files changed, 4 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/aspm.c b/drivers/infiniband/hw/hfi1/aspm.c
> index a3c53be4072c..8e3fc1d4c9c6 100644
> --- a/drivers/infiniband/hw/hfi1/aspm.c
> +++ b/drivers/infiniband/hw/hfi1/aspm.c
> @@ -54,45 +54,13 @@ static void aspm_hw_set_l1_ent_latency(struct hfi1_devdata *dd)
>       pci_write_config_dword(dd->pcidev, PCIE_CFG_REG_PL3, reg32);
>  }
>
> -static void aspm_hw_enable_l1(struct hfi1_devdata *dd)
> -{
> -     struct pci_dev *parent = dd->pcidev->bus->self;
> -
> -     /*
> -      * If the driver does not have access to the upstream component,
> -      * it cannot support ASPM L1 at all.
> -      */
> -     if (!parent)
> -             return;
> -
> -     /* Enable ASPM L1 first in upstream component and then downstream */
> -     pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC,
> -                                        PCI_EXP_LNKCTL_ASPM_L1);
> -     pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC,
> -                                        PCI_EXP_LNKCTL_ASPM_L1);
> -}
> -
> -void aspm_hw_disable_l1(struct hfi1_devdata *dd)
> -{
> -     struct pci_dev *parent = dd->pcidev->bus->self;
> -
> -     /* Disable ASPM L1 first in downstream component and then upstream */
> -     pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKCTL,
> -                                        PCI_EXP_LNKCTL_ASPMC, 0x0);
> -     if (parent)
> -             pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> -                                                PCI_EXP_LNKCTL_ASPMC, 0x0);
> -}
> -
>  static  void aspm_enable(struct hfi1_devdata *dd)
>  {
>       if (dd->aspm_enabled || aspm_mode == ASPM_MODE_DISABLED ||
>           !dd->aspm_supported)
>               return;
>
> -     aspm_hw_enable_l1(dd);
> +     pci_enable_link_state(dd->pcidev, PCI_EXP_LNKCTL_ASPM_L1);
>       dd->aspm_enabled = true;
>  }
>
> @@ -101,7 +69,7 @@ static  void aspm_disable(struct hfi1_devdata *dd)
>       if (!dd->aspm_enabled || aspm_mode == ASPM_MODE_ENABLED)
>               return;
>
> -     aspm_hw_disable_l1(dd);
> +     pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>       dd->aspm_enabled = false;
>  }
>
> @@ -254,7 +222,7 @@ void aspm_init(struct hfi1_devdata *dd)
>       /* Start with ASPM disabled */
>       aspm_hw_set_l1_ent_latency(dd);
>       dd->aspm_enabled = false;
> -     aspm_hw_disable_l1(dd);
> +     pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>
>       /* Now turn on ASPM if configured */
>       aspm_enable_all(dd);
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
> index 08732e1ac966..767f6cb770b6 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -1182,7 +1182,7 @@ int do_pcie_gen3_transition(struct hfi1_devdata *dd)
>        * will be enabled if required later
>        */
>       dd_dev_info(dd, "%s: clearing ASPM\n", __func__);
> -     aspm_hw_disable_l1(dd);
> +     pci_disable_link_state(dd->pcidev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>
>       /*
>        * step 5f: clear DirectSpeedChange

Looks good.

Reviewed-by: Dean Luick <dean.luick@xxxxxxxxxxxxxxxxxxxx>

-Dean
External recipient