linux-kernel - Re: [RFC PATCH v1 12/13] IB/hfi1: Use pci_disable/enable_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ae831b51-0db2-7d10-3050-b9981db32448@cornelisnetworks.com>
Date:   Mon, 5 Jun 2023 11:27:58 -0500
From:   Dean Luick <dean.luick@...nelisnetworks.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Lukas Wunner <lukas@...ner.de>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Leon Romanovsky <leon@...nel.org>, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 12/13] IB/hfi1: Use pci_disable/enable_link_state()

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@...ux.intel.com>
> ---
>  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@...nelisnetworks.com>

-Dean
External recipient