Re: [RFC PATCH v1 2/4] PCI/ASPM: Remove struct pcie_link_state.clkpm_capable

Bjorn Helgaas
2021-09-30T16:05:21Z

On Wed, Sep 29, 2021 at 02:43:58AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
>
> The clkpm_capable member of the struct pcie_link_state indicates
> if the device is Clock PM capable. This can be calculated when
> it is needed.

... because it comes from Link Capabilities, a read-only register.

> This patch:
> - removes clkpm_capable from struct pcie_link_state
> - moves the calculation of clkpm_capable into
> pcie_is_clkpm_capable()
> - replaces references to clkpm_capable with a call to
> pcie_is_clkpm_capable()

No need to say "this patch"; commit logs *always* refer to "this
patch". Reword in imperative sentence structure, as if giving
commands, e.g.,

Remove pcie_link_state.clkpm_capable and replace it with
pcie_is_clkpm_capable(), which computes the value from Link
Capabilities as needed.

> Signed-off-by: Bolarinwa O. Saheed <refactormyself@gmail.com>
> ---
> drivers/pci/pcie/aspm.c | 39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c23da9a4e2fb..9e65da9a22dd 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -61,7 +61,6 @@ struct pcie_link_state {
> u32 aspm_disable:7; /* Disabled ASPM state */
>
> /* Clock PM state */
> - u32 clkpm_capable:1; /* Clock PM capable? */
> u32 clkpm_enabled:1; /* Current Clock PM state */
> u32 clkpm_disable:1; /* Clock PM disabled */
>
> @@ -146,6 +145,25 @@ static int pcie_get_clkpm_state(struct pci_dev *pdev)
> return enabled;
> }
>
> +static int pcie_is_clkpm_capable(struct pci_dev *pdev)
> +{
> + int capable = 1;
> + u32 reg32;
> + struct pci_dev *child;
> + struct pci_bus *linkbus = pdev->subordinate;
> +
> + /* All functions should have the same cap state, take the worst */
> + list_for_each_entry(child, &linkbus->devices, bus_list) {
> + pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> + if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> + capable = 0;
> + break;
> + }
> + }
> +
> + return capable;

static bool pcie_is_clkpm_capable()
{
list_for_each_entry(child, &linkbus->devices, bus_list) {
pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &cap);
if (!(cap & PCI_EXP_LNKCAP_CLKPM))
return false;
}
return true;
}

> +}
> +
> static int policy_to_clkpm_state(struct pcie_link_state *link)
> {
> switch (aspm_policy) {
> @@ -177,11 +195,12 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
>
> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> {
> + int capable = pcie_is_clkpm_capable(link->pdev);
> /*
> * Don't enable Clock PM if the link is not Clock PM capable
> * or Clock PM is disabled
> */
> - if (!link->clkpm_capable || link->clkpm_disable)
> + if (!capable || link->clkpm_disable)

I think I'd just call pcie_is_clkpm_capable() directly in the "if"
condition to avoid the local variable, as you did below in
aspm_ctrl_attrs_are_visible().

> enable = 0;
> /* Need nothing if the specified equals to current state */
> if (link->clkpm_enabled == enable)
> @@ -191,21 +210,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>
> static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> {
> - int capable = 1;
> - u32 reg32;
> - struct pci_dev *child;
> - struct pci_bus *linkbus = link->pdev->subordinate;
> -
> - /* All functions should have the same cap and state, take the worst */
> - list_for_each_entry(child, &linkbus->devices, bus_list) {
> - pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> - if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> - capable = 0;
> - break;
> - }
> - }
> link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
> - link->clkpm_capable = capable;

This makes good sense to me because PCI_EXP_LNKCAP_CLKPM is read-only
and there's no need to cache the boot-time value because we can always
recompute clkpm_capable if we need it.

> link->clkpm_disable = blacklist ? 1 : 0;
> }
>
> @@ -1346,7 +1351,7 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
> return 0;
>
> if (n == 0)
> - return link->clkpm_capable ? a->mode : 0;
> + return pcie_is_clkpm_capable(link->pdev) ? a->mode : 0;
>
> return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
> }
> --
> 2.20.1
>