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

[Ocean HY1 He]

Hi Bjorn,

I file a bug today: https://bugzilla.kernel.org/show_bug.cgi?id=118941

My patch passed in my test machine, but it's sad that Lenovo PA team still find reset issue when do batch test even applied my patch.
So please ignore it at this time. I am sorry for it. But I will spend more time to find if the patch is still right but has no impact on above bug report.

The section "5.4.1.1.1. Entry into the L0s State" of PCIE spec says: "Software must not enable L0s in either direction on a given Link unless components on both sides of the Link each support L0s; otherwise, the result is undefined."
So I think the issue may be caused by different ASPM L0s setting in each side of the link. After disable both of them, the issue gone.

And I find both sides of the link always keep ASPM L1 the same. Why ASPM L0s is treated in different way even PCIE spec has concern?
Is it valuable and acceptable to let ASPM L0s always keep the same in both sides of a link?

Thanks!

Ocean He / 何海洋
SW Development Dept. 
Beijing Design Center
Enterprise Product Group
Mobile: 18911778926
E-mail: hehy1@xxxxxxxxxx
No.6 Chuang Ye Road, Haidian District, Beijing, China 100085


-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx] 
Sent: Tuesday, May 24, 2016 7:54 PM
To: Ocean HY1 He
Cc: bhelgaas@xxxxxxxxxx; wangyijing@xxxxxxxxxx; luto@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; prarit@xxxxxxxxxx; jcm@xxxxxxxxxx; Nagananda Chumbalkar
Subject: Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

Hi Ocean,

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.

Random resets of an endpoint sounds like a pretty bad problem.  Do you
have a bug report?  We've had lots of issues with ASPM; I wonder if
this could account for some of them.

> Signed-off-by: Ocean He <hehy1@xxxxxxxxxx>
> ---
>  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;
> -- 
> 1.8.3.1
> --
> 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