On Monday, March 14, 2011, Naga Chumbalkar wrote:
>
> I believe that the assumption made in commit
> 41cd766b065970ff6f6c89dd1cf55fa706c84a3d -- which is that
> pci_enable_device() will result in re-configuring ASPM is no longer
> valid. This is due to commit 97c145f7c87453cec90e91238fba5fe2c1561b32
> which resets dev->current_state usually to D0.
>
> Prior to this commit dev->current_state used to be PCI_UNKNOWN. This
> resulted in pcie_aspm_pm_state_change() getting called from
> pci_raw_set_pci_power_state().
>
> However, now that dev->current_state is D0, the equality check (below)
> ensures that the ASPM state change routine is never called.
> ./drivers/pci/pci.c: pci_raw_set_pci_power_state()
> 546 /* Check if we're already there */
> 547 if (dev->current_state == state)
> 548 return 0;
>
> So the PCIe links remain not configured.
>
> If you use a slightly older kernel that doesn't contain commit
> 97c145f7c87453cec90e91238fba5fe2c1561b32 then the above problem doesn't
> exist.
>
> In order to not break commit 4a865905f685eaefaedf6ade362323dc52aa703b
> where we want pci_set_power_state() to return success for PCI devices
> without any PM support, here is a patch to fix the problem:
>
> Signed-off-by: Naga Chumbalkar <nagananda.chumbalkar@xxxxxx>
I actually am not the maintainer of the code in question and patches like
these should be sent to linux-pci (now CCed).
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b714d78..2ec1766 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -543,12 +543,15 @@ static int pci_raw_set_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)
> - return 0;
I don't really think this change is correct, because it will make carry out
the state change even if the old state is equal to the new one, which had not
been done before. This may result in some unexpected kinds of breakage.
> - if (!dev->pm_cap)
> - return -EIO;
> + /* For devices without any PM support ensure that we return success
> + * because they may already be in D0.
> + */
> + if (!dev->pm_cap) {
> + if (dev->current_state == state)
> + return 0;
> + else
> + return -EIO;
> + }
>
> if (state < PCI_D0 || state > PCI_D3hot)
> return -EINVAL;
I think you should change the ASPM enabling logic which appears to be broken
instead of making changes affecting users who don't care about ASPM.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
