From: "Bolarinwa O. Saheed" <[email protected]>
These series removes the Clock PM members of the
struct pcie_link_state. Another member is introduced to mark
devices that the kernel has disbled.
MERGE NOTICE:
These series are based on
ยป 'commit e4e737bb5c17 ("Linux 5.15-rc2")'
Bolarinwa O. Saheed (4):
[PCI/ASPM:] Remove struct pcie_link_state.clkpm_default
PCI/ASPM: Remove struct pcie_link_state.clkpm_capable
PCI/ASPM: Remove struct pcie_link_state.clkpm_enabled
PCI/ASPM: Remove struct pcie_link_state.clkpm_disable
drivers/pci/pcie/aspm.c | 95 +++++++++++++++++++++++------------------
1 file changed, 53 insertions(+), 42 deletions(-)
--
2.20.1
From: "Bolarinwa O. Saheed" <[email protected]>
The clkpm_disable member of the struct pcie_link_state indicates
if the Clock PM state of the device is disabled. There are two
situations which can cause the Clock PM state disabled.
1. If the device fails sanity check as in pcie_aspm_sanity_check()
2. By calling __pci_disable_link_state()
It is possible to set the Clock PM state of a device ON or OFF by
calling pcie_set_clkpm(). The state can be retieved by calling
pcie_get_clkpm_state().
pcie_link_state.clkpm_disable is only accessed in pcie_set_clkpm()
to ensure that Clock PM state can be reenabled after being disabled.
This patch:
- add pm_disable to the struct pcie_link_state, to indicate that
the kernel has marked the device's AS and Clock PM states disabled
- removes clkpm_disable from the struct pcie_link_state
- removes all instance where clkpm_disable is set
- ensure that the Clock PM is always disabled if it is part of the
states passed into __pci_disable_link_state(), regardless of the
global policy
Signed-off-by: Bolarinwa O. Saheed <[email protected]>
---
drivers/pci/pcie/aspm.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 368828cd427d..e6ae00daa7ae 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -60,8 +60,7 @@ struct pcie_link_state {
u32 aspm_default:7; /* Default ASPM state by BIOS */
u32 aspm_disable:7; /* Disabled ASPM state */
- /* Clock PM state */
- u32 clkpm_disable:1; /* Clock PM disabled */
+ u32 pm_disabled:1; /* Disabled AS and Clock PM ? */
/* Exit latencies */
struct aspm_latency latency_up; /* Upstream direction exit latency */
@@ -198,7 +197,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
* Don't enable Clock PM if the link is not Clock PM capable
* or Clock PM is disabled
*/
- if (!capable || link->clkpm_disable)
+ if (enable && (!capable || link->pm_disabled))
enable = 0;
/* Need nothing if the specified equals to current state */
if (pcie_get_clkpm_state(link->pdev) == enable)
@@ -206,11 +205,6 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
pcie_set_clkpm_nocheck(link, enable);
}
-static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
-{
- link->clkpm_disable = blacklist ? 1 : 0;
-}
-
static bool pcie_retrain_link(struct pcie_link_state *link)
{
struct pci_dev *parent = link->pdev;
@@ -952,8 +946,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
*/
pcie_aspm_cap_init(link, blacklist);
- /* Setup initial Clock PM state */
- pcie_clkpm_cap_init(link, blacklist);
+ link->pm_disabled = blacklist;
/*
* At this stage drivers haven't had an opportunity to change the
@@ -1129,8 +1122,8 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
pcie_config_aspm_link(link, policy_to_aspm_state(link));
if (state & PCIE_LINK_STATE_CLKPM)
- link->clkpm_disable = 1;
- pcie_set_clkpm(link, policy_to_clkpm_state(link));
+ pcie_set_clkpm(link, 0);
+
mutex_unlock(&aspm_lock);
if (sem)
up_read(&pci_bus_sem);
@@ -1301,7 +1294,6 @@ static ssize_t clkpm_store(struct device *dev,
down_read(&pci_bus_sem);
mutex_lock(&aspm_lock);
- link->clkpm_disable = !state_enable;
pcie_set_clkpm(link, policy_to_clkpm_state(link));
mutex_unlock(&aspm_lock);
--
2.20.1
From: "Bolarinwa O. Saheed" <[email protected]>
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.
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()
Signed-off-by: Bolarinwa O. Saheed <[email protected]>
---
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, ®32);
+ if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+ capable = 0;
+ break;
+ }
+ }
+
+ return capable;
+}
+
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)
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, ®32);
- if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
- capable = 0;
- break;
- }
- }
link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
- link->clkpm_capable = capable;
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
From: "Bolarinwa O. Saheed" <[email protected]>
The clkpm_enabled member of the struct pcie_link_state stores the
current Clock PM state for the device. However, when the state changes
it is persisted and can be retrieve by calling pcie_get_clkpm_state()
introduced in patch [1/3] in this series.
This patch:
- removes clkpm_enabled from the struct pcie_link_state
- removes all instance where clkpm_enable is set
- replaces references to clkpm_enabled with a call to
pcie_get_clkpm_state()
Signed-off-by: Bolarinwa O. Saheed <[email protected]>
---
drivers/pci/pcie/aspm.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 9e65da9a22dd..368828cd427d 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_enabled:1; /* Current Clock PM state */
u32 clkpm_disable:1; /* Clock PM disabled */
/* Exit latencies */
@@ -190,7 +189,6 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
PCI_EXP_LNKCTL_CLKREQ_EN,
val);
- link->clkpm_enabled = !!enable;
}
static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
@@ -203,14 +201,13 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
if (!capable || link->clkpm_disable)
enable = 0;
/* Need nothing if the specified equals to current state */
- if (link->clkpm_enabled == enable)
+ if (pcie_get_clkpm_state(link->pdev) == enable)
return;
pcie_set_clkpm_nocheck(link, enable);
}
static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
{
- link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
link->clkpm_disable = blacklist ? 1 : 0;
}
@@ -1287,7 +1284,7 @@ static ssize_t clkpm_show(struct device *dev,
struct pci_dev *pdev = to_pci_dev(dev);
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
- return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
+ return sysfs_emit(buf, "%d\n", pcie_get_clkpm_state(link->pdev));
}
static ssize_t clkpm_store(struct device *dev,
--
2.20.1
From: "Bolarinwa O. Saheed" <[email protected]>
The clkpm_default member of the struct pcie_link_state stores the
value of the default clkpm state as it is in the BIOS.
This patch:
- Removes clkpm_default from struct pcie_link_state
- Creates pcie_get_clkpm_state() which return the clkpm state
obtained the BIOS
- Replaces references to clkpm_default with call to
pcie_get_clkpm_state()
Signed-off-by: Bolarinwa O. Saheed <[email protected]>
---
drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 013a47f587ce..c23da9a4e2fb 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -63,7 +63,6 @@ struct pcie_link_state {
/* Clock PM state */
u32 clkpm_capable:1; /* Clock PM capable? */
u32 clkpm_enabled:1; /* Current Clock PM state */
- u32 clkpm_default:1; /* Default Clock PM state by BIOS */
u32 clkpm_disable:1; /* Clock PM disabled */
/* Exit latencies */
@@ -123,6 +122,30 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
return 0;
}
+static int pcie_get_clkpm_state(struct pci_dev *pdev)
+{
+ int enabled = 1;
+ u32 reg32;
+ u16 reg16;
+ struct pci_dev *child;
+ struct pci_bus *linkbus = pdev->subordinate;
+
+ /* All functions should have the same clkpm state, take the worst */
+ list_for_each_entry(child, &linkbus->devices, bus_list) {
+ pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
+ if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+ enabled = 0;
+ break;
+ }
+
+ pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
+ if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
+ enabled = 0;
+ }
+
+ return enabled;
+}
+
static int policy_to_clkpm_state(struct pcie_link_state *link)
{
switch (aspm_policy) {
@@ -134,7 +157,7 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
/* Enable Clock PM */
return 1;
case POLICY_DEFAULT:
- return link->clkpm_default;
+ return pcie_get_clkpm_state(link->pdev);
}
return 0;
}
@@ -168,9 +191,8 @@ 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, enabled = 1;
+ int capable = 1;
u32 reg32;
- u16 reg16;
struct pci_dev *child;
struct pci_bus *linkbus = link->pdev->subordinate;
@@ -179,15 +201,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
capable = 0;
- enabled = 0;
break;
}
- pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
- if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
- enabled = 0;
}
- link->clkpm_enabled = enabled;
- link->clkpm_default = enabled;
+ link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
link->clkpm_capable = capable;
link->clkpm_disable = blacklist ? 1 : 0;
}
--
2.20.1
On Wed, Sep 29, 2021 at 02:43:57AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <[email protected]>
>
> The clkpm_default member of the struct pcie_link_state stores the
> value of the default clkpm state as it is in the BIOS.
>
> This patch:
> - Removes clkpm_default from struct pcie_link_state
> - Creates pcie_get_clkpm_state() which return the clkpm state
> obtained the BIOS
> - Replaces references to clkpm_default with call to
> pcie_get_clkpm_state()
>
> Signed-off-by: Bolarinwa O. Saheed <[email protected]>
> ---
> drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 013a47f587ce..c23da9a4e2fb 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -63,7 +63,6 @@ struct pcie_link_state {
> /* Clock PM state */
> u32 clkpm_capable:1; /* Clock PM capable? */
> u32 clkpm_enabled:1; /* Current Clock PM state */
> - u32 clkpm_default:1; /* Default Clock PM state by BIOS */
> u32 clkpm_disable:1; /* Clock PM disabled */
>
> /* Exit latencies */
> @@ -123,6 +122,30 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
> return 0;
> }
>
> +static int pcie_get_clkpm_state(struct pci_dev *pdev)
> +{
> + int enabled = 1;
> + u32 reg32;
> + u16 reg16;
> + struct pci_dev *child;
> + struct pci_bus *linkbus = pdev->subordinate;
> +
> + /* All functions should have the same clkpm state, take the worst */
> + list_for_each_entry(child, &linkbus->devices, bus_list) {
> + pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
> + if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> + enabled = 0;
> + break;
> + }
> +
> + pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> + if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> + enabled = 0;
> + }
> +
> + return enabled;
> +}
> +
> static int policy_to_clkpm_state(struct pcie_link_state *link)
> {
> switch (aspm_policy) {
> @@ -134,7 +157,7 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> /* Enable Clock PM */
> return 1;
> case POLICY_DEFAULT:
> - return link->clkpm_default;
> + return pcie_get_clkpm_state(link->pdev);
> }
> return 0;
> }
> @@ -168,9 +191,8 @@ 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, enabled = 1;
> + int capable = 1;
> u32 reg32;
> - u16 reg16;
> struct pci_dev *child;
> struct pci_bus *linkbus = link->pdev->subordinate;
>
> @@ -179,15 +201,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
> if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> capable = 0;
> - enabled = 0;
> break;
> }
> - pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> - if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> - enabled = 0;
> }
> - link->clkpm_enabled = enabled;
> - link->clkpm_default = enabled;
> + link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
I love the idea of removing clkpm_default, but I need a little more
convincing. Before this patch, this code computes clkpm_default from
PCI_EXP_LNKCAP_CLKPM and PCI_EXP_LNKCTL_CLKREQ_EN of all the functions
of the device.
PCI_EXP_LNKCAP_CLKPM is a read-only value, so we can re-read that any
time. But PCI_EXP_LNKCTL_CLKREQ_EN is writable, so if we want to know
the value that firmware put there, we need to read and save it before
we modify it.
Why is it safe to remove this init-time read of
PCI_EXP_LNKCTL_CLKREQ_EN and instead re-read it any time we need the
"default" settings from firmware?
> link->clkpm_capable = capable;
> link->clkpm_disable = blacklist ? 1 : 0;
> }
> --
> 2.20.1
>
On Wed, Sep 29, 2021 at 02:43:58AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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, ®32);
> + 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, ®32);
> - 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
>
On Wed, Sep 29, 2021 at 02:43:59AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <[email protected]>
>
> The clkpm_enabled member of the struct pcie_link_state stores the
> current Clock PM state for the device. However, when the state changes
> it is persisted and can be retrieve by calling pcie_get_clkpm_state()
> introduced in patch [1/3] in this series.
>
> This patch:
> - removes clkpm_enabled from the struct pcie_link_state
> - removes all instance where clkpm_enable is set
> - replaces references to clkpm_enabled with a call to
> pcie_get_clkpm_state()
Similar commit log comments as previous patch.
> Signed-off-by: Bolarinwa O. Saheed <[email protected]>
> ---
> drivers/pci/pcie/aspm.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 9e65da9a22dd..368828cd427d 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_enabled:1; /* Current Clock PM state */
> u32 clkpm_disable:1; /* Clock PM disabled */
>
> /* Exit latencies */
> @@ -190,7 +189,6 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
> pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> PCI_EXP_LNKCTL_CLKREQ_EN,
> val);
> - link->clkpm_enabled = !!enable;
> }
>
> static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> @@ -203,14 +201,13 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> if (!capable || link->clkpm_disable)
> enable = 0;
> /* Need nothing if the specified equals to current state */
> - if (link->clkpm_enabled == enable)
> + if (pcie_get_clkpm_state(link->pdev) == enable)
Instead of pcie_get_clkpm_state(), I think I would add this:
static int pcie_clkpm_enabled(struct pci_dev *pdev)
{
struct pci_dev *child;
struct pci_bus *linkbus = pdev->subordinate;
u16 ctl;
/* CLKREQ_EN is only applicable for Upstream Ports */
list_for_each_entry(child, &linkbus->devices, bus_list) {
pcie_capability_read_word(PCI_EXP_LNKCTL, &ctl);
if (!(ctl & PCI_EXP_LNKCTL_CLKREQ_EN))
return 0;
}
return 1;
}
And I would rename pcie_is_clkpm_capable() from the previous patch to
match, i.e., pcie_clkpm_capable(). I suggested "bool" for it, but maybe
it should stay "int" to match this. They both could be "bool", but
that seems a little messy because "enable" comes from
policy_to_clkpm_state(), so it would involve quite a few more changes.
> return;
> pcie_set_clkpm_nocheck(link, enable);
> }
>
> static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> {
> - link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
> link->clkpm_disable = blacklist ? 1 : 0;
> }
>
> @@ -1287,7 +1284,7 @@ static ssize_t clkpm_show(struct device *dev,
> struct pci_dev *pdev = to_pci_dev(dev);
> struct pcie_link_state *link = pcie_aspm_get_link(pdev);
>
> - return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
> + return sysfs_emit(buf, "%d\n", pcie_get_clkpm_state(link->pdev));
> }
>
> static ssize_t clkpm_store(struct device *dev,
> --
> 2.20.1
>
On Wed, Sep 29, 2021 at 02:44:00AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <[email protected]>
>
> The clkpm_disable member of the struct pcie_link_state indicates
> if the Clock PM state of the device is disabled. There are two
> situations which can cause the Clock PM state disabled.
> 1. If the device fails sanity check as in pcie_aspm_sanity_check()
> 2. By calling __pci_disable_link_state()
And, 3. clkpm_store(), when the user writes to the "clkpm" sysfs file,
right?
IIUC, clkpm_disable really tells us whether we can enable clkpm. The
only place we test clkpm_disable is in pcie_set_clkpm():
pcie_set_clkpm(struct pcie_link_state *link, int enable)
{
if (!link->clkpm_capable || link->clkpm_disable)
enable = 0;
pcie_set_clkpm_nocheck(link, enable);
}
So in other words, if clkpm_disable is set, we will never call
pcie_set_clkpm_nocheck() to *enable* clkpm. We will only call it to
*disable* clkpm.
Tangent: I think the usefulness of pcie_set_clkpm_nocheck() being a
separate function is gone. I think things will be a little simpler if
we integrate it into pcie_set_clkpm(). Separate preliminary patch, of
course.
> It is possible to set the Clock PM state of a device ON or OFF by
> calling pcie_set_clkpm(). The state can be retieved by calling
> pcie_get_clkpm_state().
s/retieved/retrieved/
> pcie_link_state.clkpm_disable is only accessed in pcie_set_clkpm()
> to ensure that Clock PM state can be reenabled after being disabled.
>
> This patch:
> - add pm_disable to the struct pcie_link_state, to indicate that
> the kernel has marked the device's AS and Clock PM states disabled
> - removes clkpm_disable from the struct pcie_link_state
> - removes all instance where clkpm_disable is set
> - ensure that the Clock PM is always disabled if it is part of the
> states passed into __pci_disable_link_state(), regardless of the
> global policy
>
> Signed-off-by: Bolarinwa O. Saheed <[email protected]>
> ---
> drivers/pci/pcie/aspm.c | 18 +++++-------------
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 368828cd427d..e6ae00daa7ae 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -60,8 +60,7 @@ struct pcie_link_state {
> u32 aspm_default:7; /* Default ASPM state by BIOS */
> u32 aspm_disable:7; /* Disabled ASPM state */
>
> - /* Clock PM state */
> - u32 clkpm_disable:1; /* Clock PM disabled */
> + u32 pm_disabled:1; /* Disabled AS and Clock PM ? */
What did we gain by renaming this? AFAICT this only affects clkpm
(the only test of pm_disabled is in pcie_set_clkpm()).
> /* Exit latencies */
> struct aspm_latency latency_up; /* Upstream direction exit latency */
> @@ -198,7 +197,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> * Don't enable Clock PM if the link is not Clock PM capable
> * or Clock PM is disabled
> */
> - if (!capable || link->clkpm_disable)
> + if (enable && (!capable || link->pm_disabled))
> enable = 0;
> /* Need nothing if the specified equals to current state */
> if (pcie_get_clkpm_state(link->pdev) == enable)
> @@ -206,11 +205,6 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> pcie_set_clkpm_nocheck(link, enable);
> }
>
> -static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> -{
> - link->clkpm_disable = blacklist ? 1 : 0;
> -}
> -
> static bool pcie_retrain_link(struct pcie_link_state *link)
> {
> struct pci_dev *parent = link->pdev;
> @@ -952,8 +946,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> */
> pcie_aspm_cap_init(link, blacklist);
>
> - /* Setup initial Clock PM state */
> - pcie_clkpm_cap_init(link, blacklist);
> + link->pm_disabled = blacklist;
>
> /*
> * At this stage drivers haven't had an opportunity to change the
> @@ -1129,8 +1122,8 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> pcie_config_aspm_link(link, policy_to_aspm_state(link));
>
> if (state & PCIE_LINK_STATE_CLKPM)
> - link->clkpm_disable = 1;
> - pcie_set_clkpm(link, policy_to_clkpm_state(link));
> + pcie_set_clkpm(link, 0);
> +
> mutex_unlock(&aspm_lock);
> if (sem)
> up_read(&pci_bus_sem);
> @@ -1301,7 +1294,6 @@ static ssize_t clkpm_store(struct device *dev,
> down_read(&pci_bus_sem);
> mutex_lock(&aspm_lock);
>
> - link->clkpm_disable = !state_enable;
Something is seriously wrong here because clkpm_store() no longer does
anything with "state_enable", the value we got from the user.
> pcie_set_clkpm(link, policy_to_clkpm_state(link));
>
> mutex_unlock(&aspm_lock);
> --
> 2.20.1
>
On Thu, Sep 30, 2021 at 5:54 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 02:43:57AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <[email protected]>
> >
> > The clkpm_default member of the struct pcie_link_state stores the
> > value of the default clkpm state as it is in the BIOS.
> >
> > This patch:
> > - Removes clkpm_default from struct pcie_link_state
> > - Creates pcie_get_clkpm_state() which return the clkpm state
> > obtained the BIOS
> > - Replaces references to clkpm_default with call to
> > pcie_get_clkpm_state()
> >
> > Signed-off-by: Bolarinwa O. Saheed <[email protected]>
> > ---
> > drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++----------
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 013a47f587ce..c23da9a4e2fb 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -63,7 +63,6 @@ struct pcie_link_state {
> > /* Clock PM state */
> > u32 clkpm_capable:1; /* Clock PM capable? */
> > u32 clkpm_enabled:1; /* Current Clock PM state */
> > - u32 clkpm_default:1; /* Default Clock PM state by BIOS */
> > u32 clkpm_disable:1; /* Clock PM disabled */
> >
> > /* Exit latencies */
> > @@ -123,6 +122,30 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
> > return 0;
> > }
> >
> > +static int pcie_get_clkpm_state(struct pci_dev *pdev)
> > +{
> > + int enabled = 1;
> > + u32 reg32;
> > + u16 reg16;
> > + struct pci_dev *child;
> > + struct pci_bus *linkbus = pdev->subordinate;
> > +
> > + /* All functions should have the same clkpm state, take the worst */
> > + list_for_each_entry(child, &linkbus->devices, bus_list) {
> > + pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
> > + if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> > + enabled = 0;
> > + break;
> > + }
> > +
> > + pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> > + if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> > + enabled = 0;
> > + }
> > +
> > + return enabled;
> > +}
> > +
> > static int policy_to_clkpm_state(struct pcie_link_state *link)
> > {
> > switch (aspm_policy) {
> > @@ -134,7 +157,7 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
> > /* Enable Clock PM */
> > return 1;
> > case POLICY_DEFAULT:
> > - return link->clkpm_default;
> > + return pcie_get_clkpm_state(link->pdev);
> > }
> > return 0;
> > }
> > @@ -168,9 +191,8 @@ 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, enabled = 1;
> > + int capable = 1;
> > u32 reg32;
> > - u16 reg16;
> > struct pci_dev *child;
> > struct pci_bus *linkbus = link->pdev->subordinate;
> >
> > @@ -179,15 +201,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> > pcie_capability_read_dword(child, PCI_EXP_LNKCAP, ®32);
> > if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> > capable = 0;
> > - enabled = 0;
> > break;
> > }
> > - pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> > - if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> > - enabled = 0;
> > }
> > - link->clkpm_enabled = enabled;
> > - link->clkpm_default = enabled;
> > + link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);
>
> I love the idea of removing clkpm_default, but I need a little more
> convincing. Before this patch, this code computes clkpm_default from
> PCI_EXP_LNKCAP_CLKPM and PCI_EXP_LNKCTL_CLKREQ_EN of all the functions
> of the device.
>
> PCI_EXP_LNKCAP_CLKPM is a read-only value, so we can re-read that any
> time. But PCI_EXP_LNKCTL_CLKREQ_EN is writable, so if we want to know
> the value that firmware put there, we need to read and save it before
> we modify it.
>
> Why is it safe to remove this init-time read of
> PCI_EXP_LNKCTL_CLKREQ_EN and instead re-read it any time we need the
> "default" settings from firmware?
After another look, it "may" not be safe, but then clkpm_default
should be documented
as /* Clock PM state at last boot */ because I don't think it is the
*default* state. Please,
let me know what I missing, I list below my reasons: (pardon the repetitions)
- I agree that clkpm_default current stores the boot time value.
- currently, the value of clkpm_default reflect the value of
PCI_EXP_LNKCAP_CLKPM
(read-only) and PCI_EXP_LNKCTL_CLKREQ_EN (writable)
- calling pcie_set_clkpm() can change the "default" state in the
firmware and it is stored
in clkpm_enable until the next boot, when clkpm_enable = clkpm_default
- if the "default" state is changed after boot then its initial value
stored in clkpm_default is
lost at reboot.
- IMO the intention of calling pcie_set_clkpm() is to set the default
value on the firmware.
We may need another function to set clkpm to a *temporary state*
that may at any time
be different from the value in the firmware.
- Currently, clkpm_enable always reflect the value in the firmware and
the may be different
from the value of clkpm_default.
- If clkpm_default does not reflect the value in the firmware after
boot, it feels to me like it is
not the *default* value
- I also think that clkpm_enabled is supposed to be a sort of
*temporary/current state* that
may or may not be stored as the default. Although, I am not sure why
it will be needed!
>
> > link->clkpm_capable = capable;
> > link->clkpm_disable = blacklist ? 1 : 0;
> > }
> > --
> > 2.20.1
> >
Thank you for the review.
On Thu, Sep 30, 2021 at 10:20 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 02:44:00AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <[email protected]>
> >
> > The clkpm_disable member of the struct pcie_link_state indicates
> > if the Clock PM state of the device is disabled. There are two
> > situations which can cause the Clock PM state disabled.
> > 1. If the device fails sanity check as in pcie_aspm_sanity_check()
> > 2. By calling __pci_disable_link_state()
>
> And, 3. clkpm_store(), when the user writes to the "clkpm" sysfs file,
> right?
That's right and something went wrong truly. I intend to propose that
instead of setting
link->clkpm_disable = !state_enable;
we can :
if (!state_enable)
pci_disable_link_state_locked(pdev, PCIE_LINK_STATE_CLKPM);
>
> IIUC, clkpm_disable really tells us whether we can enable clkpm. The
> only place we test clkpm_disable is in pcie_set_clkpm():
>
> pcie_set_clkpm(struct pcie_link_state *link, int enable)
> {
> if (!link->clkpm_capable || link->clkpm_disable)
> enable = 0;
> pcie_set_clkpm_nocheck(link, enable);
> }
>
> So in other words, if clkpm_disable is set, we will never call
> pcie_set_clkpm_nocheck() to *enable* clkpm. We will only call it to
> *disable* clkpm.
>
For case 1 (as stated above), this is fine. However, I am concerned that
the intention of cases 2 and 3 may require that it should be possible to
re-enable clkpm after disabling it. Is this the correct perspective?
I think the essence of this patch should have been to show that cases 2 and 3
have the same goal which is different from case 1. Hence, three and not two
checks are needed within pcie_set_clkpm().
> Tangent: I think the usefulness of pcie_set_clkpm_nocheck() being a
> separate function is gone. I think things will be a little simpler if
> we integrate it into pcie_set_clkpm(). Separate preliminary patch, of
> course.
>
> > It is possible to set the Clock PM state of a device ON or OFF by
> > calling pcie_set_clkpm(). The state can be retieved by calling
> > pcie_get_clkpm_state().
>
> s/retieved/retrieved/
>
> > pcie_link_state.clkpm_disable is only accessed in pcie_set_clkpm()
> > to ensure that Clock PM state can be reenabled after being disabled.
> >
> > This patch:
> > - add pm_disable to the struct pcie_link_state, to indicate that
> > the kernel has marked the device's AS and Clock PM states disabled
> > - removes clkpm_disable from the struct pcie_link_state
> > - removes all instance where clkpm_disable is set
> > - ensure that the Clock PM is always disabled if it is part of the
> > states passed into __pci_disable_link_state(), regardless of the
> > global policy
> >
> > Signed-off-by: Bolarinwa O. Saheed <[email protected]>
> > ---
> > drivers/pci/pcie/aspm.c | 18 +++++-------------
> > 1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 368828cd427d..e6ae00daa7ae 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -60,8 +60,7 @@ struct pcie_link_state {
> > u32 aspm_default:7; /* Default ASPM state by BIOS */
> > u32 aspm_disable:7; /* Disabled ASPM state */
> >
> > - /* Clock PM state */
> > - u32 clkpm_disable:1; /* Clock PM disabled */
> > + u32 pm_disabled:1; /* Disabled AS and Clock PM ? */
>
> What did we gain by renaming this? AFAICT this only affects clkpm
> (the only test of pm_disabled is in pcie_set_clkpm()).
>
> > /* Exit latencies */
> > struct aspm_latency latency_up; /* Upstream direction exit latency */
> > @@ -198,7 +197,7 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> > * Don't enable Clock PM if the link is not Clock PM capable
> > * or Clock PM is disabled
> > */
> > - if (!capable || link->clkpm_disable)
> > + if (enable && (!capable || link->pm_disabled))
> > enable = 0;
> > /* Need nothing if the specified equals to current state */
> > if (pcie_get_clkpm_state(link->pdev) == enable)
> > @@ -206,11 +205,6 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
> > pcie_set_clkpm_nocheck(link, enable);
> > }
> >
> > -static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
> > -{
> > - link->clkpm_disable = blacklist ? 1 : 0;
> > -}
> > -
> > static bool pcie_retrain_link(struct pcie_link_state *link)
> > {
> > struct pci_dev *parent = link->pdev;
> > @@ -952,8 +946,7 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
> > */
> > pcie_aspm_cap_init(link, blacklist);
> >
> > - /* Setup initial Clock PM state */
> > - pcie_clkpm_cap_init(link, blacklist);
> > + link->pm_disabled = blacklist;
> >
> > /*
> > * At this stage drivers haven't had an opportunity to change the
> > @@ -1129,8 +1122,8 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
> > pcie_config_aspm_link(link, policy_to_aspm_state(link));
> >
> > if (state & PCIE_LINK_STATE_CLKPM)
> > - link->clkpm_disable = 1;
> > - pcie_set_clkpm(link, policy_to_clkpm_state(link));
> > + pcie_set_clkpm(link, 0);
> > +
> > mutex_unlock(&aspm_lock);
> > if (sem)
> > up_read(&pci_bus_sem);
> > @@ -1301,7 +1294,6 @@ static ssize_t clkpm_store(struct device *dev,
> > down_read(&pci_bus_sem);
> > mutex_lock(&aspm_lock);
> >
> > - link->clkpm_disable = !state_enable;
>
> Something is seriously wrong here because clkpm_store() no longer does
> anything with "state_enable", the value we got from the user.
>
> > pcie_set_clkpm(link, policy_to_clkpm_state(link));
> >
> > mutex_unlock(&aspm_lock);
> > --
> > 2.20.1
> >