Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
> In pcie_config_aspm_link(), when convert ASPM state to
> upstream/downstream ASPM register state, the upstream variable and
> dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
> even it should be disabled and PCI/E endpoint may reset randomly.
> 
> Signed-off-by: Ocean He <hehy1@lenovo.com>
> ---
>  drivers/pci/pcie/aspm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2dfe7fd..3f8a44d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -439,9 +439,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		return;
>  	/* Convert ASPM state to upstream/downstream ASPM register state */
>  	if (state & ASPM_STATE_L0S_UP)
> -		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> -	if (state & ASPM_STATE_L0S_DW)
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> +	if (state & ASPM_STATE_L0S_DW)
> +		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
>  	if (state & ASPM_STATE_L1) {
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
>  		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;

I think the current code is correct.  Here's my reasoning, please
check and see if you agree:

  #define ASPM_STATE_L0S_UP       (1)
  #define ASPM_STATE_L0S_DW       (2)
  #define ASPM_STATE_L0S          (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)

  pcie_aspm_cap_init
  {
    ...
    if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
      link->aspm_support |= ASPM_STATE_L0S;
    link->aspm_capable = link->aspm_support;

Now "aspm_capable" has ASPM_STATE_L0S set only if both ends support L0s.

  pcie_config_aspm_link(link, state)
  {
    ...
    state &= (link->aspm_capable & ...)

This clears ASPM_STATE_L0S in "state" unless both ends support L0s.

    if (state & ASPM_STATE_L0S_UP)
      dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;

If the caller of pcie_config_aspm_link() requested ASPM_STATE_L0S_UP
*and* both ends support L0s, we set PCI_EXP_LNKCTL_ASPM_L0S in
"dwstream".

    pcie_config_aspm_dev(child, dwstream);

Now we enable the downstream component's transmitter to enter L0s.
Per PCIe spec r3.0, sec 7.8.7, the receiver, i.e., the upstream
component, must be capable of entering L0s even when its transmitter
is disabled from entering L0s.

The way I read this,

  - We can only enable L0s when both ends of the link support L0s, and
  - We only need to enable L0s on the transmitting end.

ASPM_STATE_L0S_UP refers to L0s in the upstream direction, so that
would mean enabling L0s in the downstream component's transmitter.

Bjorn