lkml.org 
[lkml]   [2021]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH v1 2/4] PCI/ASPM: Remove struct pcie_link_state.clkpm_capable
    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
    >

    \
     
     \ /
      Last update: 2021-09-30 18:06    [W:4.856 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site