PCI: PM: Do not read power state in pci_enable_device_flags()
PATCH 1/1 PCI: PM: Do not read power state in pci_enable_device_flags()
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the current_state field of struct pci_dev in pci_enable_device_flags() before calling do_pci_enable_device() for the device, because none of the code between that point and the pci_set_power_state() call in do_pci_enable_device() invoked later depends on it.
Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose _STA method initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from the PCI_PM_CTRL register is D0, the current_state field in the struct pci_dev representing that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.
To avoid such issues it is better to leave the current_state value as is until it is changed to PCI_D0 by do_pci_enable_device() as appropriate. However, the power state of the device is not changed to PCI_D0 if it is already enabled when pci_enable_device_flags() gets called for it, so update its current_state in that case, but use pci_update_current_state() covering platform PM too for that.
---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
drivers/pci/pci.c +3 -13
@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK ); - } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)
Quoted 24 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.
Quoted 8 lines
Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
Thanks! I've tested this now as well, all looks good.
Regards,
Max
Quoted 36 lines
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +3
-13@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK ); - } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)
Quoted 24 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.
Quoted 4 lines
Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Rafael J. Wysocki
maintainerQuoted 25 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.
Quoted 4 lines
Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>
Bjorn, can I take this, or do you want to take care of it yourself?
Quoted 40 lines
---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +3
-13@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK ); - } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)
Rafael J. Wysocki
maintainerQuoted 27 lines · Rafael J. Wysocki
Quoted 25 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.
Quoted 6 lines
Quoted 4 lines
Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>Bjorn, can I take this, or do you want to take care of it yourself?
I'm taking the silence as consent, so the patch has been applied as 5.13 material with the R-by from Mika.
Quoted 40 lines
Quoted 40 lines
---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +3
-13@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK ); - } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)
Hi,
Quoted 55 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-10@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK );
drivers/pci/pci.c static int pci_enable_device_flags(struc · line 1879
Quoted 10 lines
drivers/pci/pci.c +3
-3- } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)
A user in Debian reported that this commit caused an issue, cf.
https://bugs.debian.org/990008#10 with the e1000e driver failing to probe the device. It was reported as well to
https://bugzilla.kernel.org/show_bug.cgi?id=213481
According to the above and
https://bugzilla.kernel.org/show_bug.cgi?id=213481#c2 reverting 4514d991d992 ("PCI: PM: Do not read power state in pci_enable_device_flags()") fixes the issue.
Any idea what is going on here?
Regards,
Salvatore
Rafael J. Wysocki
maintainerQuoted 75 lines · Salvatore Bonaccorso
Hi,
Quoted 55 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
It should not be necessary to update the
current_statefield of structpci_devinpci_enable_device_flags()before callingdo_pci_enable_device()for the device, because none of the code between that point and thepci_set_power_state()call indo_pci_enable_device()invoked later depends on it.Moreover, doing that is actively harmful in some cases. For example, if the given PCI device depends on an ACPI power resource whose
_STAmethod initially returns 0 ("off"), but the config space of the PCI device is accessible and the power state retrieved from thePCI_PM_CTRLregister is D0, thecurrent_statefield in the structpci_devrepresenting that device will get out of sync with the power.state of its ACPI companion object and that will lead to power management issues going forward.To avoid such issues it is better to leave the
current_statevalue as is until it is changed toPCI_D0bydo_pci_enable_device()as appropriate. However, the power state of the device is not changed toPCI_D0if it is already enabled whenpci_enable_device_flags()gets called for it, so update itscurrent_statein that case, but usepci_update_current_state()covering platform PM too for that.Reported-by Maximilian Luz <[email protected]>Tested-by Maximilian Luz <[email protected]>Signed-off-by Rafael J. Wysocki <[email protected]>---
Max, I've added a T-by from you even though the patch is slightly different from what you have tested, but the difference shouldn't matter for your case.
--- drivers/pci/pci.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)Index:
linux-pm/drivers/pci/pci.c
===================================================================drivers/pci/pci.c +0
-10@@ -1870,20 +1870,10 @@ static int pci_enable_device_flags(strucint err;int i, bars =0 ; -/* -* Power state could be unknown at this point, either due to a fresh -* boot or a device removal call. So get the current power state -* so that things like MSI message writing will behave as expected -* (e.g. if the device really is in D0 at enable time). -*/ -if (dev->pm_cap) { -u16 pmcsr; -pci_read_config_word (dev, dev->pm_cap +PCI_PM_CTRL , &pmcsr); - dev->current_state = (pmcsr &PCI_PM_CTRL_STATE_MASK );drivers/pci/pci.c static int pci_enable_device_flags(struc · line 1879
Quoted 10 lines
drivers/pci/pci.c +3
-3- } - -if (atomic_inc_return (&dev->enable_cnt) >1 ) +if (atomic_inc_return (&dev->enable_cnt) >1 ) { +pci_update_current_state (dev, dev->current_state);return 0 ;/* already enabled */ + } bridge =pci_upstream_bridge (dev);if (bridge)A user in Debian reported that this commit caused an issue, cf.
https://bugs.debian.org/990008#10 with the e1000e driver failing to probe the device. It was reported as well to
https://bugzilla.kernel.org/show_bug.cgi?id=213481
Quoted 4 lines
According to the above and
https://bugzilla.kernel.org/show_bug.cgi?id=213481#c2 reverting4514d991d992("PCI: PM: Do not read power state inpci_enable_device_flags()") fixes the issue.
This commit has just been reverted.
We will try to address the original issue addressed by it in a different way.
Thanks!