PCI: Set device power state to PCI_D0 for device without native PM support
PATCH 1/1 PCI: Set device power state to PCI_D0 for device without native PM support
During test of one IB card with guest VM, found that, msi is not initialized properly.
It turns out __write_msi_msg will do nothing if device current_state is not PCI_D0. And, that pci device does not have pm_cap in guest VM.
There is an error in setting of power state to PCI_D0 in pci_enable_device(), but error is not returned for this. Following is code flow:pci_enable_device() --> __pci_enable_device_flags() --> do_pci_enable_device() --> pci_set_power_state() --> __pci_start_power_transition() We have following condition inside __pci_start_power_transition():
if (platform_pci_power_manageable(dev)) {
error = platform_pci_set_power_state(dev, state);
if (!error)
pci_update_current_state(dev, state);
} else {
error = -ENODEV;
/* Fall back to PCI_D0 if native PM is not supported */
if (!dev->pm_cap)
dev->current_state = PCI_D0;
}Here, from platform_pci_set_power_state(), acpi_pci_set_power_state() is getting called and that is failing with ENODEV because of following condition:
if (!handle || ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0",&tmp)))
return -ENODEV;Because of that, pci_update_current_state() is not getting called.
With this patch, if device power state can not be set via platform_pci_set_power_state and that device does not have native pm support, then PCI device power state will be set to PCI_D0.
--- drivers/pci/pci.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/pci/pci.c +3 -0
@@ -664,6 +664,9 @@ static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) error =platform_pci_set_power_state (dev, state);if (!error)pci_update_current_state (dev, state); +/* Fall back to PCI_D0 if native PM is not supported */ +else if (!dev->pm_cap) + dev->current_state =PCI_D0 ; }else { error = -ENODEV ;/* Fall back to PCI_D0 if native PM is not supported */
Signature · 1 line
1.7.5.1
Hi,
Quoted 40 lines · Ajaykumar Hotchandani
During test of one IB card with guest VM, found that, msi is not initialized properly.
It turns out
__write_msi_msgwill do nothing if devicecurrent_stateis notPCI_D0. And, that pci device does not havepm_capin guest VM.There is an error in setting of power state to
PCI_D0inpci_enable_device(), but error is not returned for this. Following is code flow:pci_enable_device()-->__pci_enable_device_flags()-->do_pci_enable_device()-->pci_set_power_state()-->__pci_start_power_transition()We have following condition inside__pci_start_power_transition():if (platform_pci_power_manageable(dev)) { error = platform_pci_set_power_state(dev, state); if (!error) pci_update_current_state(dev, state); } else { error = -ENODEV; /* Fall back to PCI_D0 if native PM is not supported */ if (!dev->pm_cap) dev->current_state = PCI_D0; }Here, from
platform_pci_set_power_state(),acpi_pci_set_power_state()is getting called and that is failing with ENODEV because of following condition:if (!handle || ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0",&tmp))) return -ENODEV;Because of that,
pci_update_current_state()is not getting called.With this patch, if device power state can not be set via
platform_pci_set_power_stateand that device does not have native pm support, then PCI device power state will be set toPCI_D0.Signed-off-by Ajaykumar Hotchandani<[email protected]>Signed-off-by Yinghai Lu<[email protected]>--- drivers/pci/pci.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/pci/pci.c +0
-0
drivers/pci/pci.c static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) · line 672
Quoted 9 lines
drivers/pci/pci.c +3
-0@@ -664,6 +664,9 @@ static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) error =platform_pci_set_power_state (dev, state);if (!error)pci_update_current_state (dev, state); +/* Fall back to PCI_D0 if native PM is not supported */ +else if (!dev->pm_cap) + dev->current_state =PCI_D0 ; }else { error = -ENODEV ;/* Fall back to PCI_D0 if native PM is not supported */
I have some vague memories that we tried that and it broke something.
How thoroughly has it been tested?
Rafael
Quoted 13 lines · Rafael J. Wysocki
Hi,
Quoted 12 lines
drivers/pci/pci.c| 3 +++1 files changed, 3 insertions(+), 0 deletions(-)drivers/pci/pci.c +1
-0@@ -664,6 +664,9 @@ static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) error = platform_pci_set_power_state(dev, state); if (!error) pci_update_current_state(dev, state); + /* Fall back to PCI_D0 if native PM is not supported */
Quoted 8 lines
drivers/pci/pci.c static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) · line 672
Quoted 6 lines
drivers/pci/pci.c +2
-0+ else if (!dev->pm_cap) + dev->current_state = PCI_D0; } else { error = -ENODEV; /* Fall back to PCI_D0 if native PM is not supported */I have some vague memories that we tried that and it broke something.
Do have any hint for us to dig out the configuration for failing?
Quoted 2 lines
How thoroughly has it been tested?
tested in setups with intel cpus from Nehalem.
Yinghai
Quoted 23 lines · Yinghai Lu
Quoted 13 lines · Rafael J. Wysocki
Hi,
Quoted 12 lines
drivers/pci/pci.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/pci/pci.c +1
-0@@ -664,6 +664,9 @@ static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) error =platform_pci_set_power_state (dev, state);if (!error)pci_update_current_state (dev, state); +/* Fall back to PCI_D0 if native PM is not supported */ Quoted 8 lines
drivers/pci/pci.c static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) · line 672
Quoted 6 lines
drivers/pci/pci.c +2
-0+else if (!dev->pm_cap) + dev->current_state =PCI_D0 ; }else { error = -ENODEV ;/* Fall back to PCI_D0 if native PM is not supported */ I have some vague memories that we tried that and it broke something.
Quoted 6 lines
Do have any hint for us to dig out the configuration for failing?
Quoted 2 lines
How thoroughly has it been tested?
tested in setups with intel cpus from Nehalem.
Can you test it on older hardware too, please?
Rafael
Quoted 24 lines · Ajaykumar Hotchandani
During test of one IB card with guest VM, found that, msi is not initialized properly.
It turns out
__write_msi_msgwill do nothing if devicecurrent_stateis notPCI_D0. And, that pci device does not havepm_capin guest VM.There is an error in setting of power state to
PCI_D0inpci_enable_device(), but error is not returned for this. Following is code flow:pci_enable_device()-->__pci_enable_device_flags()-->do_pci_enable_device()-->pci_set_power_state()-->__pci_start_power_transition()We have following condition inside__pci_start_power_transition():if (platform_pci_power_manageable(dev)) { error = platform_pci_set_power_state(dev, state); if (!error) pci_update_current_state(dev, state); } else { error = -ENODEV; /* Fall back to PCI_D0 if native PM is not supported */ if (!dev->pm_cap) dev->current_state = PCI_D0; }Here, from
platform_pci_set_power_state(),acpi_pci_set_power_state()is getting called and that is failing with ENODEV because of following condition:if (!handle || ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0",&tmp))) return -ENODEV;
Quoted 6 lines
Because of that,
pci_update_current_state()is not getting called.With this patch, if device power state can not be set via
platform_pci_set_power_stateand that device does not have native pm support, then PCI device power state will be set toPCI_D0.Signed-off-by Ajaykumar Hotchandani<[email protected]>Signed-off-by Yinghai Lu<[email protected]>
I had the same issue and sent a patch a while ago to fix it, adding
current_state = PCI_D0 in acpiphp_glue.c:register_slot
it is strange that this does not work for you:
Quoted 8 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
current_state=PCI_D0in acpiphp_glue.c:register_slotit is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
Yinghai
Quoted 5 lines · Yinghai Lu
Quoted 3 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
Quoted 7 lines
Quoted 5 lines
current_state=PCI_D0in acpiphp_glue.c:register_slotit is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that current_state = D0 in pciehp_enable_slot, like acpiphp does
Quoted 7 lines · Stefano Stabellini
Quoted 6 lines · Yinghai Lu
Quoted 4 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
current_state=PCI_D0in acpiphp_glue.c:register_slot
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
it is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that
current_state= D0 inpciehp_enable_slot, like acpiphp does
Here, acpi hotplugging is involved.
With your change in register_slot(), device will have proper power state when module is being loaded for the first time after booting. However, while unload of pci module; following is in pci_device_remove():
if (pci_dev->current_state == PCI_D0)
pci_dev->current_state = PCI_UNKNOWN;So, device power state state will remain PCI_UNKNOWN while module is loaded again. Subsequently, MSI write will do nothing.
Thanks,
Ajay
Quoted 15 lines · Ajaykumar Hotchandani
Quoted 7 lines · Stefano Stabellini
Quoted 6 lines · Yinghai Lu
Quoted 4 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
current_state=PCI_D0in acpiphp_glue.c:register_slotQuoted 7 lines
Quoted 4 lines
Quoted 3 lines
it is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that
current_state= D0 inpciehp_enable_slot, like acpiphp does
Quoted 7 lines
Here, acpi hotplugging is involved.
With your change inregister_slot(), device will have proper power state when module is being loaded for the first time after booting. However, while unload of pci module; following is inpci_device_remove():if (pci_dev->current_state == PCI_D0) pci_dev->current_state = PCI_UNKNOWN;So, device power state state will remain
PCI_UNKNOWNwhile module is loaded again. Subsequently, MSI write will do nothing.
Does this mean that this bug would actually trigger even with devices that do support _EJ0 and power management?
Because acpi_pci_set_power_state won't set current_state to PCI_D0 because the "hotplug driver will take care of _PSx" (see 10b3dcae0f275e2546e55303d64ddbb58cec7599) but the hotplug driver is not actually invoked when loading again a driver module of an existing pci device (the example you mention above)?
Quoted 21 lines · Stefano Stabellini
Quoted 20 lines · Ajaykumar Hotchandani
Quoted 5 lines · Stefano Stabellini
Quoted 4 lines · Yinghai Lu
Quoted 2 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
current_state=PCI_D0in acpiphp_glue.c:register_slotit is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that
current_state= D0 inpciehp_enable_slot, like acpiphp doesHere, acpi hotplugging is involved.
With your change inregister_slot(), device will have proper power state when module is being loaded for the first time after booting. However, while unload of pci module; following is inpci_device_remove():if (pci_dev->current_state == PCI_D0) pci_dev->current_state = PCI_UNKNOWN;
Quoted 3 lines
Quoted 1 line
So, device power state state will remain
PCI_UNKNOWNwhile module is loaded again. Subsequently, MSI write will do nothing.Does this mean that this bug would actually trigger even with devices that do support
_EJ0and power management?
Nope. I don't have system to verify. But, for the scenario you mentioned (device is hot pluggable, acpi bus power manageable and device with pm_cap supported; if I understand it correctly), following is what I think should happen:
- Inside pci_platform_power_transition(), platform_pci_power_manageable() will be successful. However, platform_pci_set_power_state() will fail (as device supports EJ0) and subsequently pci_update_current_state() will not get called.
- So, current power state of device will not be read and power state of device will remain PCI_UNKNOWN.
- Now, pci_raw_set_power_state() will get called. Here, as current power state is PCI_UNKNOWN, pci_write_config_word() will be called with pmcsr value as 0. As, last two bits of pmcsr is 0, written power state of device will be PCI_D0 now. And subsequently, dev->current_state will be assigned as PCI_D0 (by using pci_read_config_word() ).
Quoted 5 lines
Because
acpi_pci_set_power_statewon't setcurrent_statetoPCI_D0because the "hotplug driver will take care of_PSx" (see10b3dcae0f275e2546e55303d64ddbb58cec7599) but the hotplug driver is not actually invoked when loading again a driver module of an existing pci device (the example you mention above)?
Can you please elaborate more on this?acpi_bus_set_power() will update state of acpi_dev (ACPI_STATE_XX) and not pci_dev (PCI_XX). And here, issue lies with current_state of pci_dev.
Thanks,
Ajay
Quoted 25 lines · Ajaykumar Hotchandani
Quoted 21 lines · Stefano Stabellini
Quoted 20 lines · Ajaykumar Hotchandani
Quoted 5 lines · Stefano Stabellini
Quoted 4 lines · Yinghai Lu
Quoted 2 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
current_state=PCI_D0in acpiphp_glue.c:register_slotit is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that
current_state= D0 inpciehp_enable_slot, like acpiphp doesHere, acpi hotplugging is involved.
With your change inregister_slot(), device will have proper power state when module is being loaded for the first time after booting. However, while unload of pci module; following is inpci_device_remove():if (pci_dev->current_state == PCI_D0) pci_dev->current_state = PCI_UNKNOWN;Quoted 3 lines
Quoted 1 line
So, device power state state will remain
PCI_UNKNOWNwhile module is loaded again. Subsequently, MSI write will do nothing.Does this mean that this bug would actually trigger even with devices that do support
_EJ0and power management?
Quoted 15 lines
Nope. I don't have system to verify. But, for the scenario you mentioned (device is hot pluggable, acpi bus power manageable and device with
pm_capsupported; if I understand it correctly), following is what I think should happen:
- Insidepci_platform_power_transition(),platform_pci_power_manageable()will be successful. However,platform_pci_set_power_state()will fail (as device supports EJ0) and subsequentlypci_update_current_state()will not get called.
- So, current power state of device will not be read and power state of device will remainPCI_UNKNOWN.
- Now,pci_raw_set_power_state()will get called. Here, as current power state isPCI_UNKNOWN,pci_write_config_word()will be called with pmcsr value as 0. As, last two bits of pmcsr is 0, written power state of device will bePCI_D0now. And subsequently,dev->current_statewill be assigned asPCI_D0(by usingpci_read_config_word()).
OK, I understand.
Quoted 9 lines
Quoted 5 lines
Because
acpi_pci_set_power_statewon't setcurrent_statetoPCI_D0because the "hotplug driver will take care of_PSx" (see10b3dcae0f275e2546e55303d64ddbb58cec7599) but the hotplug driver is not actually invoked when loading again a driver module of an existing pci device (the example you mention above)?Can you please elaborate more on this?
acpi_bus_set_power()will update state ofacpi_dev(ACPI_STATE_XX) and notpci_dev(PCI_XX).
And here, issue lies withcurrent_stateofpci_dev.
I agree, I was merely stating again what the problem is.
BTW I am OK with your patch.
Quoted 49 lines · Stefano Stabellini
Quoted 25 lines · Ajaykumar Hotchandani
Quoted 21 lines · Stefano Stabellini
Quoted 20 lines · Ajaykumar Hotchandani
Quoted 5 lines · Stefano Stabellini
Quoted 4 lines · Yinghai Lu
Quoted 2 lines · Stefano Stabellini
I had the same issue and sent a patch a while ago to fix it, adding
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
current_state=PCI_D0in acpiphp_glue.c:register_slotit is strange that this does not work for you:
So guest os has to load acpiphp instead of pciehp?
maybe pciehp needs to make sure that
current_state= D0 inpciehp_enable_slot, like acpiphp doesHere, acpi hotplugging is involved.
With your change inregister_slot(), device will have proper power state when module is being loaded for the first time after booting. However, while unload of pci module; following is inpci_device_remove():if (pci_dev->current_state == PCI_D0) pci_dev->current_state = PCI_UNKNOWN;Quoted 3 lines
Quoted 1 line
So, device power state state will remain
PCI_UNKNOWNwhile module is loaded again. Subsequently, MSI write will do nothing.Does this mean that this bug would actually trigger even with devices that do support
_EJ0and power management?Quoted 15 lines
Nope. I don't have system to verify. But, for the scenario you mentioned (device is hot pluggable, acpi bus power manageable and device with
pm_capsupported; if I understand it correctly), following is what I think should happen:
- Insidepci_platform_power_transition(),platform_pci_power_manageable()will be successful. However,platform_pci_set_power_state()will fail (as device supports EJ0) and subsequentlypci_update_current_state()will not get called.
- So, current power state of device will not be read and power state of device will remainPCI_UNKNOWN.
- Now,pci_raw_set_power_state()will get called. Here, as current power state isPCI_UNKNOWN,pci_write_config_word()will be called with pmcsr value as 0. As, last two bits of pmcsr is 0, written power state of device will bePCI_D0now. And subsequently,dev->current_statewill be assigned asPCI_D0(by usingpci_read_config_word()).OK, I understand.
Quoted 5 lines
Quoted 5 lines
Because
acpi_pci_set_power_statewon't setcurrent_statetoPCI_D0because the "hotplug driver will take care of_PSx" (see10b3dcae0f275e2546e55303d64ddbb58cec7599) but the hotplug driver is not actually invoked when loading again a driver module of an existing pci device (the example you mention above)?
Quoted 7 lines
Quoted 4 lines
Can you please elaborate more on this?
acpi_bus_set_power()will update state ofacpi_dev(ACPI_STATE_XX) and notpci_dev(PCI_XX).
And here, issue lies withcurrent_stateofpci_dev.I agree, I was merely stating again what the problem is.
BTW I am OK with your patch.
Should we revert 47e9037ac16637cd7f12b8790ea7ce6680e42168 (your changes in register_slot() ) I think, after this change, it's not needed. Or, am I missing some scenario?
Quoted 6 lines · Ajaykumar Hotchandani
Quoted 1 line
BTW I am OK with your patch.
Should we revert
47e9037ac16637cd7f12b8790ea7ce6680e42168(your changes inregister_slot())
I think, after this change, it's not needed. Or, am I missing some scenario?
I think you are right, 47e9037ac16637cd7f12b8790ea7ce6680e42168 is not needed anymore, so I am OK with reverting it.
Quoted 8 lines · Stefano Stabellini
Quoted 5 lines · Ajaykumar Hotchandani
Quoted 1 line
BTW I am OK with your patch.
Should we revert
47e9037ac16637cd7f12b8790ea7ce6680e42168(your changes inregister_slot())
I think, after this change, it's not needed. Or, am I missing some scenario?I think you are right,
47e9037ac16637cd7f12b8790ea7ce6680e42168is not needed anymore, so I am OK with reverting it.
Thanks, following is updated patch.
[PATCH -v2] PCI: Set device power state to PCI_D0 for device without native PM support
During test of one IB card with guest VM, found that, msi is not initialized properly.
It turns out __write_msi_msg will do nothing if device current_state is not PCI_D0. And, that pci device does not have pm_cap in guest VM.
There is an error in setting of power state to PCI_D0 in pci_enable_device(), but error is not returned for this. Following is code flow:pci_enable_device() --> __pci_enable_device_flags() --> do_pci_enable_device() --> pci_set_power_state() --> __pci_start_power_transition() We have following condition inside __pci_start_power_transition():
if (platform_pci_power_manageable(dev)) {
error = platform_pci_set_power_state(dev, state);
if (!error)
pci_update_current_state(dev, state);
} else {
error = -ENODEV;
/* Fall back to PCI_D0 if native PM is not supported */
if (!dev->pm_cap)
dev->current_state = PCI_D0;
}Here, from platform_pci_set_power_state(), acpi_pci_set_power_state() is getting called and that is failing with ENODEV because of following condition:
if (!handle || ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0",&tmp)))
return -ENODEV;Because of that, pci_update_current_state() is not getting called.
With this patch, if device power state can not be set via platform_pci_set_power_state and that device does not have native pm support, then PCI device power state will be set to PCI_D0.
-v2: This also reverts 47e9037ac16637cd7f12b8790ea7ce6680e42168, as it's not needed after this change.
--- drivers/pci/hotplug/acpiphp_glue.c | 1 - drivers/pci/pci.c | 3 +++ 2 files changed, 3 insertions(+), 1 deletions(-)
drivers/pci/hotplug/acpiphp_glue.c +0 -1
@@ -212,7 +212,6 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) pdev =pci_get_slot (pbus,PCI_DEVFN (device, function));if (pdev) { - pdev->current_state =PCI_D0 ; slot->flags |= (SLOT_ENABLED |SLOT_POWEREDON );pci_dev_put (pdev); }
drivers/pci/pci.c +3 -0
@@ -664,6 +664,9 @@ static int pci_platform_power_transition(struct pci_dev *dev, pci_power_t state) error =platform_pci_set_power_state (dev, state);if (!error)pci_update_current_state (dev, state); +/* Fall back to PCI_D0 if native PM is not supported */ +if (!dev->pm_cap) + dev->current_state =PCI_D0 ; }else { error = -ENODEV ;/* Fall back to PCI_D0 if native PM is not supported */
Signature · 1 line
1.7.5.1