PCI/PM: Rework powering up PCI devices
cover PCI/PM: Rework powering up PCI devices
Hi All,
This patch set replaces patches [4-9/9] from the series at
which had to be dropped, because they were problematic:
This time there are more patches making smaller changes each and I've done my best to avoid making any changes without a good enough motivation.
Please refer to the patch changelogs for details.
Thanks!
PATCH 1/11 PCI/PM: Split pci_raw_set_power_state()
From: Rafael J. Wysocki <[email protected]>
The transitions from low-power states to D0 and the other way around are unnecessarily tangled in pci_raw_set_power_state() which makes it rather hard to follow.
Moreover, the only caller of pci_raw_set_power_state() passing PCI_D0 as its state argument is pci_power_up(), so the code carrying out transitions into D0 can be put directly into that function.
Accordingly, move the code handling transitions from low-power states into D0 directly into pci_power_up() and rename the remaining part of pci_raw_set_power_state() to pci_set_low_power_state(), because it only handles transitions into low-power state now.
While at it, fix up some white space, update some comments and modify messages printed by pci_power_up() and pci_set_low_power_state() to be less confusing (which is the only expected functional impact of this change).
--- drivers/pci/pci.c | 143 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 83 insertions(+), 60 deletions(-)
drivers/pci/pci.c +83 -60
@@ -1068,10 +1068,11 @@ static inline bool platform_pci_bridge_d }/** -* pci_raw_set_power_state - Use PCI PM registers to set the power state of -* given PCI device +* pci_set_low_power_state - Put a PCI device into a low-power state. * @dev: PCI device to handle. -* @state: PCI power state (D0, D1, D2, D3hot) to put the device into. +* @state: PCI power state (D1, D2, D3hot) to put the device into. +* +* Use the device's PCI_PM_CTRL register to put it into a low-power state. * * RETURN VALUE: * -EINVAL if the requested state is invalid.
@@ -1080,10 +1081,9 @@ static inline bool platform_pci_bridge_d* 0 if device already is in the requested state. * 0 if device's power state has been successfully changed. */ -static int pci_raw_set_power_state (struct pci_dev *dev,pci_power_t state) +static int pci_set_low_power_state (struct pci_dev *dev,pci_power_t state) {u16 pmcsr; -bool need_restore = false;/* Check if we're already there */ if (dev->current_state == state)
@@ -1092,7 +1092,7 @@ static int pci_raw_set_power_state(strucif (!dev->pm_cap)return -EIO ; -if (state <PCI_D0 || state > PCI_D3hot) +if (state <PCI_D1 || state > PCI_D3hot)return -EINVAL ;/*
@@ -1101,8 +1101,7 @@ static int pci_raw_set_power_state(struc* we can go from D1 to D3, but we can't go directly from D3 to D1; * we'd have to go from D3 to D0, then to D1. */ -if (state !=PCI_D0 && dev->current_state <= PCI_D3cold - && dev->current_state > state) { +if (dev->current_state <= PCI_D3cold && dev->current_state > state) {pci_err (dev,"invalid power transition (from %s to %s)\n" ,pci_power_name (dev->current_state),pci_power_name (state));
@@ -1116,70 +1115,30 @@ static int pci_raw_set_power_state(strucpci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr);if (PCI_POSSIBLE_ERROR (pmcsr)) { -pci_err (dev,"can't change power state from %s to %s (config space inaccessible)\n" , +pci_err (dev,"Unable to change power state from %s to %s, device inaccessible\n" ,pci_power_name (dev->current_state),pci_power_name (state));return -EIO ; } -/* -* If we're (effectively) in D3, force entire word to 0. -* This doesn't affect PME_Status, disables PME_En, and -* sets PowerState to 0. -*/ -switch (dev->current_state) { -case PCI_D0 : -case PCI_D1 : -case PCI_D2 : - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - pmcsr |= state; -break ; -case PCI_D3hot: -case PCI_D3cold: -case PCI_UNKNOWN :/* Boot-up */ -if ((pmcsr &PCI_PM_CTRL_STATE_MASK ) == PCI_D3hot - && !(pmcsr &PCI_PM_CTRL_NO_SOFT_RESET )) - need_restore = true; - fallthrough;/* force to D0 */ -default : - pmcsr =0 ; -break ; - } + pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; + pmcsr |= state;/* Enter specified state */ pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); -/* -* Mandatory power management transition delays; see PCI PM 1.1 -* 5.6.1 table 18 -*/ -if (state == PCI_D3hot || dev->current_state == PCI_D3hot) +/* Mandatory power management transition delays; see PCI PM 1.2. */ +if (state == PCI_D3hot)pci_dev_d3_sleep (dev); -else if (state ==PCI_D2 || dev->current_state ==PCI_D2 ) +else if (state ==PCI_D2 )udelay (PCI_PM_D2_DELAY );pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK ); + dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ;if (dev->current_state != state) -pci_info_ratelimited (dev,"refused to change power state from %s to %s\n" , -pci_power_name (dev->current_state), -pci_power_name (state)); - -/* -* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT -* INTERFACE SPECIFICATION, REV. 1.2", a device transitioning -* from D3hot to D0 _may_ perform an internal reset, thereby -* going to "D0 Uninitialized" rather than "D0 Initialized". -* For example, at least some versions of the 3c905B and the -* 3c556B exhibit this behaviour. -* -* At least some laptop BIOSen (e.g. the Thinkpad T21) leave -* devices in a D3hot state at boot. Consequently, we need to -* restore at least the BARs so that the device will be -* accessible to its driver. -*/ -if (need_restore) -pci_restore_bars (dev); +pci_info_ratelimited (dev,"Refused to change power state from %s to %s\n" , +pci_power_name (dev->current_state), +pci_power_name (state));if (dev->bus->self)pcie_aspm_pm_state_change (dev->bus->self);
@@ -1312,8 +1271,72 @@ static int pci_dev_wait(struct pci_dev **/ int pci_power_up (struct pci_dev *dev) { +bool need_restore = false; +u16 pmcsr; +pci_platform_power_transition (dev,PCI_D0 ); -return pci_raw_set_power_state (dev,PCI_D0 ); + +if (dev->current_state ==PCI_D0 ) +return 0 ; + +if (!dev->pm_cap) +return -EIO ; + +pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); +if (PCI_POSSIBLE_ERROR (pmcsr)) { +pci_err (dev,"Unable to change power state from %s to D0, device inaccessible\n" , +pci_power_name (dev->current_state)); +return -EIO ; + } + +/* +* If we're (effectively) in D3, force entire word to 0. This doesn't +* affect PME_Status, disables PME_En, and sets PowerState to 0. +*/ +if (dev->current_state >= PCI_D3hot) { +if ((pmcsr &PCI_PM_CTRL_STATE_MASK ) == PCI_D3hot && + !(pmcsr &PCI_PM_CTRL_NO_SOFT_RESET )) + need_restore = true; + + pmcsr =0 ; + }else { + pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; + } + +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); + +/* Mandatory transition delays; see PCI PM 1.2. */ +if (dev->current_state == PCI_D3hot) +pci_dev_d3_sleep (dev); +else if (dev->current_state ==PCI_D2 ) +udelay (PCI_PM_D2_DELAY ); + +pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); + dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ; +if (dev->current_state !=PCI_D0 ) +pci_info_ratelimited (dev,"Refused to change power state from %s to D0\n" , +pci_power_name (dev->current_state)); + +/* +* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT +* INTERFACE SPECIFICATION, REV. 1.2", a device transitioning +* from D3hot to D0 _may_ perform an internal reset, thereby +* going to "D0 Uninitialized" rather than "D0 Initialized". +* For example, at least some versions of the 3c905B and the +* 3c556B exhibit this behaviour. +* +* At least some laptop BIOSen (e.g. the Thinkpad T21) leave +* devices in a D3hot state at boot. Consequently, we need to +* restore at least the BARs so that the device will be +* accessible to its driver. +*/ +if (need_restore) +pci_restore_bars (dev); + +if (dev->bus->self) +pcie_aspm_pm_state_change (dev->bus->self); + +return 0 ; }/**
@@ -1394,7 +1417,7 @@ int pci_set_power_state(struct pci_dev ** To put device in D3cold, we put device into D3hot in native * way, then put device into D3cold with platform ops */ - error =pci_raw_set_power_state (dev, state > PCI_D3hot ? + error =pci_set_low_power_state (dev, state > PCI_D3hot ? PCI_D3hot : state);if (pci_platform_power_transition (dev, state))
PATCH 2/11 PCI/PM: Relocate pci_set_low_power_state()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Because pci_set_power_state() is the only caller of pci_set_low_power_state(), put the latter next to the former.
No functional impact.
--- drivers/pci/pci.c | 158 +++++++++++++++++++++++++++--------------------------- 1 file changed, 79 insertions(+), 79 deletions(-)
drivers/pci/pci.c +79 -79
@@ -1068,85 +1068,6 @@ static inline bool platform_pci_bridge_d }/** -* pci_set_low_power_state - Put a PCI device into a low-power state. -* @dev: PCI device to handle. -* @state: PCI power state (D1, D2, D3hot) to put the device into. -* -* Use the device's PCI_PM_CTRL register to put it into a low-power state. -* -* RETURN VALUE: -* -EINVAL if the requested state is invalid. -* -EIO if device does not support PCI PM or its PM capabilities register has a -* wrong version, or device doesn't support the requested state. -* 0 if device already is in the requested state. -* 0 if device's power state has been successfully changed. -*/ -static int pci_set_low_power_state (struct pci_dev *dev,pci_power_t state) -{ -u16 pmcsr; - -/* Check if we're already there */ -if (dev->current_state == state) -return 0 ; - -if (!dev->pm_cap) -return -EIO ; - -if (state <PCI_D1 || state > PCI_D3hot) -return -EINVAL ; - -/* -* Validate transition: We can enter D0 from any state, but if -* we're already in a low-power state, we can only go deeper. E.g., -* we can go from D1 to D3, but we can't go directly from D3 to D1; -* we'd have to go from D3 to D0, then to D1. -*/ -if (dev->current_state <= PCI_D3cold && dev->current_state > state) { -pci_err (dev,"invalid power transition (from %s to %s)\n" , -pci_power_name (dev->current_state), -pci_power_name (state)); -return -EINVAL ; - } - -/* Check if this device supports the desired state */ -if ((state ==PCI_D1 && !dev->d1_support) - || (state ==PCI_D2 && !dev->d2_support)) -return -EIO ; - -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); -if (PCI_POSSIBLE_ERROR (pmcsr)) { -pci_err (dev,"Unable to change power state from %s to %s, device inaccessible\n" , -pci_power_name (dev->current_state), -pci_power_name (state)); -return -EIO ; - } - - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - pmcsr |= state; - -/* Enter specified state */ -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); - -/* Mandatory power management transition delays; see PCI PM 1.2. */ -if (state == PCI_D3hot) -pci_dev_d3_sleep (dev); -else if (state ==PCI_D2 ) -udelay (PCI_PM_D2_DELAY ); - -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ; -if (dev->current_state != state) -pci_info_ratelimited (dev,"Refused to change power state from %s to %s\n" , -pci_power_name (dev->current_state), -pci_power_name (state)); - -if (dev->bus->self) -pcie_aspm_pm_state_change (dev->bus->self); - -return 0 ; -} - -/** * pci_update_current_state - Read power state of given device and cache it * @dev: PCI device to handle. * @state: State to cache in case the device doesn't have the PM capability
@@ -1364,6 +1285,85 @@ void pci_bus_set_current_state(struct pc }/** +* pci_set_low_power_state - Put a PCI device into a low-power state. +* @dev: PCI device to handle. +* @state: PCI power state (D1, D2, D3hot) to put the device into. +* +* Use the device's PCI_PM_CTRL register to put it into a low-power state. +* +* RETURN VALUE: +* -EINVAL if the requested state is invalid. +* -EIO if device does not support PCI PM or its PM capabilities register has a +* wrong version, or device doesn't support the requested state. +* 0 if device already is in the requested state. +* 0 if device's power state has been successfully changed. +*/ +static int pci_set_low_power_state (struct pci_dev *dev,pci_power_t state) +{ +u16 pmcsr; + +/* Check if we're already there */ +if (dev->current_state == state) +return 0 ; + +if (!dev->pm_cap) +return -EIO ; + +if (state <PCI_D1 || state > PCI_D3hot) +return -EINVAL ; + +/* +* Validate transition: We can enter D0 from any state, but if +* we're already in a low-power state, we can only go deeper. E.g., +* we can go from D1 to D3, but we can't go directly from D3 to D1; +* we'd have to go from D3 to D0, then to D1. +*/ +if (dev->current_state <= PCI_D3cold && dev->current_state > state) { +pci_err (dev,"invalid power transition (from %s to %s)\n" , +pci_power_name (dev->current_state), +pci_power_name (state)); +return -EINVAL ; + } + +/* Check if this device supports the desired state */ +if ((state ==PCI_D1 && !dev->d1_support) + || (state ==PCI_D2 && !dev->d2_support)) +return -EIO ; + +pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); +if (PCI_POSSIBLE_ERROR (pmcsr)) { +pci_err (dev,"Unable to change power state from %s to %s, device inaccessible\n" , +pci_power_name (dev->current_state), +pci_power_name (state)); +return -EIO ; + } + + pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; + pmcsr |= state; + +/* Enter specified state */ +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); + +/* Mandatory power management transition delays; see PCI PM 1.2. */ +if (state == PCI_D3hot) +pci_dev_d3_sleep (dev); +else if (state ==PCI_D2 ) +udelay (PCI_PM_D2_DELAY ); + +pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); + dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ; +if (dev->current_state != state) +pci_info_ratelimited (dev,"Refused to change power state from %s to %s\n" , +pci_power_name (dev->current_state), +pci_power_name (state)); + +if (dev->bus->self) +pcie_aspm_pm_state_change (dev->bus->self); + +return 0 ; +} + +/** * pci_set_power_state - Set the power state of a PCI device * @dev: PCI device to handle. * @state: PCI power state (D0, D1, D2, D3hot) to put the device into.
PATCH 3/11 PCI/PM: Set current_state to D3cold if the device is not accessible
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Make pci_power_up() and pci_set_low_power_state() change current_state to PCI_D3cold when the device is not accessible along the lines of pci_update_current_state().
--- drivers/pci/pci.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/pci/pci.c +2 -0
@@ -1207,6 +1207,7 @@ int pci_power_up(struct pci_dev *dev)if (PCI_POSSIBLE_ERROR (pmcsr)) {pci_err (dev,"Unable to change power state from %s to D0, device inaccessible\n" ,pci_power_name (dev->current_state)); + dev->current_state = PCI_D3cold;return -EIO ; }
@@ -1335,6 +1336,7 @@ static int pci_set_low_power_state(strucpci_err (dev,"Unable to change power state from %s to %s, device inaccessible\n" ,pci_power_name (dev->current_state),pci_power_name (state)); + dev->current_state = PCI_D3cold;return -EIO ; }
PATCH 4/11 PCI/PM: Unfold pci_platform_power_transition() in pci_power_up()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Some actions carried out by pci_platform_power_transition(() in pci_power_up() are redundant, but before dealing with them, make pci_power_up() call the pci_platform_power_transition() code directly (and avoid a redundant check when pm_cap is unset while at it).
--- drivers/pci/pci.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/pci/pci.c +8 -1
@@ -1194,8 +1194,15 @@ int pci_power_up(struct pci_dev *dev) {bool need_restore = false;u16 pmcsr; +int ret; -pci_platform_power_transition (dev,PCI_D0 ); + ret =platform_pci_set_power_state (dev,PCI_D0 ); +if (!ret) { +pci_update_current_state (dev,PCI_D0 ); + }else if (!dev->pm_cap) {/* Fall back to PCI_D0 */ + dev->current_state =PCI_D0 ; +return 0 ; + }if (dev->current_state ==PCI_D0 )return 0 ;
PATCH 5/11 PCI/PM: Do not call pci_update_current_state() from pci_power_up()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Notice that calling pci_update_current_state() from pci_power_up() is redundant and may be harmful in some cases.
First, if the device is in a low-power state before pci_power_up() gets called for it and platform_pci_set_power_state() successfully changes its power state to D0, pci_update_current_state() will update current_state to reflect that and pci_power_up() will return success right away without restoring the device's BARs or reconfiguring ASPM which may be necessary. This is arguably incorrect and definitely inconsistent with the case when platform_pci_set_power_state() returns an error (for example, because the device is not power-manageable by the platform firmware).
Second, current_state should not be overwritten until the decision whether or not to restore the device's BARs is made, because that decision generally depends on its value. Again, calling pci_update_current_state() in pci_power_up() is not consistent with this observation.
Next, pci_power_up() attempts to read from the device's PCI_PM_CTRL register regardless of the current_state value unless it is PCI_D0, including the case when pci_update_current_state() sets current_state to PCI_D3cold to indicate that the device is not accessible. If the register read is not successful, current_state will be set to PCI_D3cold anyway, so that pci_update_current_state() action is redundant.
Further, if pci_update_current_state() reads the device's PCI_PM_CTRL register, pci_power_up() will repeat that read going forward and it is not necessary to update current_state in the meantime.
Finally, if pm_cap is not set (in which case the PCI_PM_CTRL register is not present), the power state of the device should be determined with the help of the platform firmware or set to D0 if that's not possible and pci_update_current_state() does not do that.
Accordingly, rearrange pci_power_up() so as to address the above shortcomings.
--- drivers/pci/pci.c | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-)
drivers/pci/pci.c +28 -21
@@ -1192,23 +1192,24 @@ static int pci_dev_wait(struct pci_dev **/ int pci_power_up (struct pci_dev *dev) { -bool need_restore = false; +bool need_restore; +pci_power_t state;u16 pmcsr; -int ret; - ret =platform_pci_set_power_state (dev,PCI_D0 ); -if (!ret) { -pci_update_current_state (dev,PCI_D0 ); - }else if (!dev->pm_cap) {/* Fall back to PCI_D0 */ - dev->current_state =PCI_D0 ; -return 0 ; - } +platform_pci_set_power_state (dev,PCI_D0 ); + +if (!dev->pm_cap) { + state =platform_pci_get_power_state (dev); +if (state ==PCI_UNKNOWN ) + dev->current_state =PCI_D0 ; +else + dev->current_state = state; -if (dev->current_state ==PCI_D0 ) -return 0 ; +if (state ==PCI_D0 ) +return 0 ; -if (!dev->pm_cap)return -EIO ; + }pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr);if (PCI_POSSIBLE_ERROR (pmcsr)) {
@@ -1218,26 +1219,31 @@ int pci_power_up(struct pci_dev *dev)return -EIO ; } + state = pmcsr &PCI_PM_CTRL_STATE_MASK ; + + need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) && + !(pmcsr &PCI_PM_CTRL_NO_SOFT_RESET ); + +if (state ==PCI_D0 ) { + dev->current_state =PCI_D0 ; + goto end; + } +/* * If we're (effectively) in D3, force entire word to 0. This doesn't * affect PME_Status, disables PME_En, and sets PowerState to 0. */ -if (dev->current_state >= PCI_D3hot) { -if ((pmcsr &PCI_PM_CTRL_STATE_MASK ) == PCI_D3hot && - !(pmcsr &PCI_PM_CTRL_NO_SOFT_RESET )) - need_restore = true; - +if (state == PCI_D3hot) pmcsr =0 ; - }else { +else pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - }pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr);/* Mandatory transition delays; see PCI PM 1.2. */ -if (dev->current_state == PCI_D3hot) +if (state == PCI_D3hot)pci_dev_d3_sleep (dev); -else if (dev->current_state ==PCI_D2 ) +else if (state ==PCI_D2 )udelay (PCI_PM_D2_DELAY );pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr);
@@ -1246,6 +1252,7 @@ int pci_power_up(struct pci_dev *dev)pci_info_ratelimited (dev,"Refused to change power state from %s to D0\n" ,pci_power_name (dev->current_state)); +end:/* * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
PATCH 6/11 PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Make pci_power_up() write 0 to the device's PCI_PM_CTRL register in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.
--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/pci/pci.c +3 -8
@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. +* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );/* Mandatory transition delays; see PCI PM 1.2. */ if (state == PCI_D3hot)
Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0.
drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );
Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CS
We intend to set PowerState to 0 (D0), apparently intend to clear PME_En, and PME_Status is "write 1 to clear" to writing 0 does nothing, so those look OK.
But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing Data_Select could potentially break the Data Register power consumption reporting (which I don't think we support today).
It seems like maybe we should do this instead:
pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)
to just unconditionally clear PowerState?
Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 55 lines · Bjorn Helgaas
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CSWe intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, andPME_Statusis "write 1 to clear" to writing 0 does nothing, so those look OK.But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing
Data_Selectcould potentially break the Data Register power consumption reporting (which I don't think we support today).
Quoted 6 lines
It seems like maybe we should do this instead:
pci_write_config_word(dev,
dev->pm_cap+PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)to just unconditionally clear PowerState?
Or I guess this, since we want to clear PME_En as well?
pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr &
~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 62 lines · Bjorn Helgaas
Quoted 60 lines · Bjorn Helgaas
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CSWe intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, andPME_Statusis "write 1 to clear" to writing 0 does nothing, so those look OK.But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing
Data_Selectcould potentially break the Data Register power consumption reporting (which I don't think we support today).It seems like maybe we should do this instead:
pci_write_config_word(dev,
dev->pm_cap+PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)
Quoted 6 lines
Quoted 1 line
to just unconditionally clear PowerState?
Or I guess this, since we want to clear
PME_Enas well?pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr & ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));
Yes.
Also, this patch actually only makes a difference if the device is going into D0 from D1 or D2, because we have always written 0 to the PMCSR during transitions from D3hot.
It is inconsistent and confusing to do different things depending on the initial power state here and the code is simpler when 0 is written regardless.
Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 72 lines · Rafael J. Wysocki
Quoted 62 lines · Bjorn Helgaas
Quoted 60 lines · Bjorn Helgaas
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CSWe intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, andPME_Statusis "write 1 to clear" to writing 0 does nothing, so those look OK.But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing
Data_Selectcould potentially break the Data Register power consumption reporting (which I don't think we support today).It seems like maybe we should do this instead:
pci_write_config_word(dev,
dev->pm_cap+PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)Quoted 6 lines
Quoted 1 line
to just unconditionally clear PowerState?
Or I guess this, since we want to clear
PME_Enas well?pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr & ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));Yes.
Quoted 7 lines
Also, this patch actually only makes a difference if the device is going into D0 from D1 or D2, because we have always written 0 to the PMCSR during transitions from D3hot.
It is inconsistent and confusing to do different things depending on the initial power state here and the code is simpler when 0 is written regardless.
I agree that depending on the initial power state is confusing (it confused me :)).
What would you think of replacing this patch with the one below?
commit defde70748bc ("PCI/PM: Always put device in D0 and disable PME in pci_power_up()")
Author: Bjorn Helgaas <[email protected]> Date: Fri May 27 17:45:07 2022 -0500
PCI/PM: Always put device in D0 and disable PME in pci_power_up()
Unconditionally put the device in PCI_D0 and disable PME generation in pci_power_up(), regardless of the power state returned by the previous read from PCI_PM_CTRL, which should not matter.
drivers/pci/pci.c +3 -8
@@ -1229,14 +1229,9 @@ int pci_power_up(struct pci_dev *dev) goto end; } -/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. -*/ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; +/* Set PowerState to 0 (PCI_D0) and disable PME generation */ + pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; + pmcsr &= ~PCI_PM_CTRL_PME_ENABLE ;pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr);
Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 77 lines · Bjorn Helgaas
Quoted 76 lines · Rafael J. Wysocki
Quoted 62 lines · Bjorn Helgaas
Quoted 60 lines · Bjorn Helgaas
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CSWe intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, andPME_Statusis "write 1 to clear" to writing 0 does nothing, so those look OK.But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing
Data_Selectcould potentially break the Data Register power consumption reporting (which I don't think we support today).It seems like maybe we should do this instead:
pci_write_config_word(dev,
dev->pm_cap+PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)Quoted 6 lines
Quoted 1 line
to just unconditionally clear PowerState?
Or I guess this, since we want to clear
PME_Enas well?pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr & ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));Yes.
Also, this patch actually only makes a difference if the device is going into D0 from D1 or D2, because we have always written 0 to the PMCSR during transitions from D3hot.
Quoted 8 lines
Quoted 3 lines
It is inconsistent and confusing to do different things depending on the initial power state here and the code is simpler when 0 is written regardless.
I agree that depending on the initial power state is confusing (it confused me :)).
What would you think of replacing this patch with the one below?
Well, I don't know why I sent this, since I had already sent the pull request. Not thinking clearly, I guess. Anyway, your original patch is now upstream. Sorry for the distraction.
Bjorn
Re: [PATCH v1 06/11] PCI/PM: Write 0 to PMCSR in pci_power_up() in all cases
Quoted 83 lines · Bjorn Helgaas
Quoted 81 lines · Bjorn Helgaas
Quoted 79 lines · Rafael J. Wysocki
Quoted 62 lines · Bjorn Helgaas
Quoted 60 lines · Bjorn Helgaas
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Make
pci_power_up()write 0 to the device'sPCI_PM_CTRLregister in order to put it into D0 regardless of the power state returned by the previous read from that register which should not matter.Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/pci/pci.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-2@@ -1230,15 +1230,10 @@ int pci_power_up(struct pci_dev *dev) }/* -* If we're (effectively) in D3, force entire word to 0. This doesn't -* affect PME_Status, disables PME_En, and sets PowerState to 0. drivers/pci/pci.c int pci_power_up(struct pci_dev *dev) · line 1236
Quoted 10 lines
drivers/pci/pci.c +3
-6+* Force the entire word to 0. This doesn't affect PME_Status, disables +* PME_En, and sets PowerState to 0. */ -if (state == PCI_D3hot) - pmcsr =0 ; -else - pmcsr &= ~PCI_PM_CTRL_STATE_MASK ; - -pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL , pmcsr); +pci_write_config_word (dev, dev->pm_cap +PCI_PM_CTRL ,0 );Can you reassure me why this is safe and useful?
This is a 16-bit write that includes (PCIe r6.0, sec 7.5.2.2):
0x0003 PowerState RW 0x0004 RsvdP 0x0008 No_Soft_Reset RO 0x00f0 RsvdP 0x0100 PME_En RW/RWS 0x1e00 Data_Select RW, VF ROZ 0x6000 Data_Scale RO, VF ROZ 0x8000 PME_Status RW1CSWe intend to set PowerState to 0 (D0), apparently intend to clear
PME_En, andPME_Statusis "write 1 to clear" to writing 0 does nothing, so those look OK.But the RsvdP fields are reserved for future RW bits and should be preserved, and it looks like clearing
Data_Selectcould potentially break the Data Register power consumption reporting (which I don't think we support today).It seems like maybe we should do this instead:
pci_write_config_word(dev,
dev->pm_cap+PCI_PM_CTRL,
pmcsr & ~PCI_PM_CTRL_STATE_MASK)Quoted 6 lines
Quoted 1 line
to just unconditionally clear PowerState?
Or I guess this, since we want to clear
PME_Enas well?pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr & ~(PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_PME_ENABLE));Yes.
Also, this patch actually only makes a difference if the device is going into D0 from D1 or D2, because we have always written 0 to the PMCSR during transitions from D3hot.
It is inconsistent and confusing to do different things depending on the initial power state here and the code is simpler when 0 is written regardless.
Quoted 8 lines
Quoted 4 lines
I agree that depending on the initial power state is confusing (it confused me :)).
What would you think of replacing this patch with the one below?
Well, I don't know why I sent this, since I had already sent the pull request. Not thinking clearly, I guess. Anyway, your original patch is now upstream. Sorry for the distraction.
No biggie.
If it turns out to be problematic, it can be changed to preserving the reserved bits easily enough.
PATCH 7/11 PCI/PM: Split pci_power_up()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
One of the two callers of pci_power_up() invokes pci_update_current_state() and pci_restore_state() right after calling it, in which case running the part of it happening after the mandatory transition delays is redundant, so move that part out of it into a new function called pci_set_full_power_state() that will be invoked from pci_set_power_state() which is the other caller of pci_power_up().
--- drivers/pci/pci.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)
drivers/pci/pci.c +35 -6
@@ -1189,6 +1189,9 @@ static int pci_dev_wait(struct pci_dev */** * pci_power_up - Put the given device into D0 * @dev: PCI device to power up +* +* On success, return 0 or 1, depending on whether or not it is necessary to +* restore the device's BARs subsequently (1 is returned in that case). */ int pci_power_up (struct pci_dev *dev) {
@@ -1224,10 +1227,8 @@ int pci_power_up(struct pci_dev *dev) need_restore = (state == PCI_D3hot || dev->current_state >= PCI_D3hot) && !(pmcsr &PCI_PM_CTRL_NO_SOFT_RESET ); -if (state ==PCI_D0 ) { - dev->current_state =PCI_D0 ; +if (state ==PCI_D0 ) goto end; - }/* * Force the entire word to 0. This doesn't affect PME_Status, disables
@@ -1241,13 +1242,41 @@ int pci_power_up(struct pci_dev *dev)else if (state ==PCI_D2 )udelay (PCI_PM_D2_DELAY ); +end: + dev->current_state =PCI_D0 ; +if (need_restore) +return 1 ; + +return 0 ; +} + +/** +* pci_set_full_power_state - Put a PCI device into D0 and update its state +* @dev: PCI device to power up +* +* Call pci_power_up() to put @dev into D0, read from its PCI_PM_CTRL register +* to confirm the state change, restore its BARs if they might be lost and +* reconfigure ASPM in acordance with the new power state. +* +* If pci_restore_state() is going to be called right after a power state change +* to D0, it is more efficient to use pci_power_up() directly instead of this +* function. +*/ +static int pci_set_full_power_state (struct pci_dev *dev) +{ +u16 pmcsr; +int ret; + + ret =pci_power_up (dev); +if (ret <0 ) +return ret; +pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ;if (dev->current_state !=PCI_D0 )pci_info_ratelimited (dev,"Refused to change power state from %s to D0\n" ,pci_power_name (dev->current_state)); -end:/* * According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT * INTERFACE SPECIFICATION, REV. 1.2", a device transitioning
@@ -1261,7 +1290,7 @@ end:* restore at least the BARs so that the device will be * accessible to its driver. */ -if (need_restore) +if (ret >0 )pci_restore_bars (dev);if (dev->bus->self)
@@ -1415,7 +1444,7 @@ int pci_set_power_state(struct pci_dev *return 0 ;if (state ==PCI_D0 ) -return pci_power_up (dev); +return pci_set_full_power_state (dev);/* * This device is quirked not to be put into D3, so don't put it in
PATCH 8/11 PCI/PM: Do not restore BARs if device is not in D0
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Do not attempt to restore the device's BARs inpci_set_full_power_state() if the actual current
power state of the device is not D0.
--- drivers/pci/pci.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
drivers/pci/pci.c +16 -16
@@ -1273,25 +1273,25 @@ static int pci_set_full_power_state(strupci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); dev->current_state = pmcsr &PCI_PM_CTRL_STATE_MASK ; -if (dev->current_state !=PCI_D0 ) +if (dev->current_state !=PCI_D0 ) {pci_info_ratelimited (dev,"Refused to change power state from %s to D0\n" ,pci_power_name (dev->current_state)); - -/* -* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT -* INTERFACE SPECIFICATION, REV. 1.2", a device transitioning -* from D3hot to D0 _may_ perform an internal reset, thereby -* going to "D0 Uninitialized" rather than "D0 Initialized". -* For example, at least some versions of the 3c905B and the -* 3c556B exhibit this behaviour. -* -* At least some laptop BIOSen (e.g. the Thinkpad T21) leave -* devices in a D3hot state at boot. Consequently, we need to -* restore at least the BARs so that the device will be -* accessible to its driver. -*/ -if (ret >0 ) + }else if (ret >0 ) { +/* +* According to section 5.4.1 of the "PCI BUS POWER MANAGEMENT +* INTERFACE SPECIFICATION, REV. 1.2", a device transitioning +* from D3hot to D0 _may_ perform an internal reset, thereby +* going to "D0 Uninitialized" rather than "D0 Initialized". +* For example, at least some versions of the 3c905B and the +* 3c556B exhibit this behaviour. +* +* At least some laptop BIOSen (e.g. the Thinkpad T21) leave +* devices in a D3hot state at boot. Consequently, we need to +* restore at least the BARs so that the device will be +* accessible to its driver. +*/ pci_restore_bars (dev); + }if (dev->bus->self)pcie_aspm_pm_state_change (dev->bus->self);
PATCH 9/11 PCI/PM: Clean up pci_set_low_power_state()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Make the following assorted non-essential changes in pci_set_low_power_state():
1. Drop two redundant checks from it (the caller takes care of these conditions).
2. Change the log level of a messages printed by it to "debug", because it only indicates a programming mistake.
--- drivers/pci/pci.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/pci/pci.c +1 -8
@@ -1341,16 +1341,9 @@ static int pci_set_low_power_state(struc {u16 pmcsr; -/* Check if we're already there */ -if (dev->current_state == state) -return 0 ; -if (!dev->pm_cap)return -EIO ; -if (state <PCI_D1 || state > PCI_D3hot) -return -EINVAL ; -/* * Validate transition: We can enter D0 from any state, but if * we're already in a low-power state, we can only go deeper. E.g.,
@@ -1358,7 +1351,7 @@ static int pci_set_low_power_state(struc* we'd have to go from D3 to D0, then to D1. */ if (dev->current_state <= PCI_D3cold && dev->current_state > state) { -pci_err (dev,"invalid power transition (from %s to %s)\n" , +pci_dbg (dev,"Invalid power transition (from %s to %s)\n" ,pci_power_name (dev->current_state),pci_power_name (state));return -EINVAL ;
PATCH 10/11 PCI/PM: Rearrange pci_set_power_state()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
The part of pci_set_power_state() related to transitions into low-power states is unnecessary convoluted, so clearly divide it into the D3cold special case and the general case covering all of the other states.
Also fix a potential issue with calling pci_bus_set_current_state() to set the current state of all devices on the subordinate bus to D3cold without checking if the power state of the parent bridge has really changed to D3cold.
--- drivers/pci/pci.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)
drivers/pci/pci.c +17 -11
@@ -1446,19 +1446,25 @@ int pci_set_power_state(struct pci_dev *if (state >= PCI_D3hot && (dev->dev_flags &PCI_DEV_FLAGS_NO_D3 ))return 0 ; -/* -* To put device in D3cold, we put device into D3hot in native -* way, then put device into D3cold with platform ops -*/ - error =pci_set_low_power_state (dev, state > PCI_D3hot ? - PCI_D3hot : state); +if (state == PCI_D3cold) { +/* +* To put the device in D3cold, put it into D3hot in the native +* way, then put it into D3cold using platform ops. +*/ + error =pci_set_low_power_state (dev, PCI_D3hot); -if (pci_platform_power_transition (dev, state)) -return error; +if (pci_platform_power_transition (dev, PCI_D3cold)) +return error; -/* Powering off a bridge may power off the whole hierarchy */ -if (state == PCI_D3cold) -pci_bus_set_current_state (dev->subordinate, PCI_D3cold); +/* Powering off a bridge may power off the whole hierarchy */ +if (dev->current_state == PCI_D3cold) +pci_bus_set_current_state (dev->subordinate, PCI_D3cold); + }else { + error =pci_set_low_power_state (dev, state); + +if (pci_platform_power_transition (dev, state)) +return error; + }return 0 ; }
PATCH 11/11 PCI/PM: Replace pci_set_power_state() in pci_pm_thaw_noirq()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Calling pci_set_power_state() to put the given device into D0 in pci_pm_thaw_noirq() may cause it to restore the device's BARs, which is redundant before calling pci_restore_state(), so replace it with a direct pci_power_up() call followed by pci_update_current_state() if it returns a nonzeor value, in analogy with pci_pm_default_resume_early().
Avoid code duplication by introducing a wrapper function to contain the repeating pattern and calling it in both places.
--- drivers/pci/pci-driver.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/pci/pci-driver.c +7 -2
@@ -551,10 +551,15 @@ static void pci_pm_default_resume(structpci_enable_wake (pci_dev,PCI_D0 , false); } -static void pci_pm_default_resume_early (struct pci_dev *pci_dev) +static void pci_pm_power_up_and_verify_state (struct pci_dev *pci_dev) {pci_power_up (pci_dev);pci_update_current_state (pci_dev,PCI_D0 ); +} + +static void pci_pm_default_resume_early (struct pci_dev *pci_dev) +{ +pci_pm_power_up_and_verify_state (pci_dev);pci_restore_state (pci_dev);pci_pme_restore (pci_dev); }
@@ -1079,7 +1084,7 @@ static int pci_pm_thaw_noirq(struct devi* in case the driver's "freeze" callbacks put it into a low-power * state. */ -pci_set_power_state (pci_dev,PCI_D0 ); +pci_pm_power_up_and_verify_state (pci_dev);pci_restore_state (pci_dev);if (pci_has_legacy_pm_support (pci_dev))
Bjorn Helgaas
Applied[+cc Krzysztof, just FYI]
Quoted 5 lines · Rafael J. Wysocki
Hi All,
This patch set replaces patches [4-9/9] from the series at
I applied this to pci/pm, thanks!
I reordered the branch so Krzysztof's "pci_restore_standard_config() defined but not used" fix is first, followed by your changes, Rafael.
Bjorn
Quoted 5 lines · Bjorn Helgaas
[+cc Krzysztof, just FYI]
Quoted 2 lines · Rafael J. Wysocki
Hi All,
Quoted 8 lines
Quoted 3 lines
This patch set replaces patches [4-9/9] from the series at
I applied this to pci/pm, thanks!
I reordered the branch so Krzysztof's "
pci_restore_standard_config()defined but not used" fix is first, followed by your changes, Rafael.
I can confirm that at commit 0f40ac35e4ec ("PCI/PM: Replace pci_set_power_state() in pci_pm_thaw_noirq()"), the problematic machine boots up with no visible issues. Thanks a lot for the quick fixes!
Cheers,
Nathan