Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 08:26:44 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Vidya Sagar <vidyas@nvidia.com>
Cc:     "Kenneth R. Crudup" <kenny@panix.com>, bhelgaas@google.com,
        lorenzo.pieralisi@arm.com, hkallweit1@gmail.com,
        wangxiongfeng2@huawei.com, mika.westerberg@linux.intel.com,
        kai.heng.feng@canonical.com, chris.packham@alliedtelesis.co.nz,
        yangyicong@hisilicon.com, treding@nvidia.com, jonathanh@nvidia.com,
        abhsahu@nvidia.com, sagupta@nvidia.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, kthota@nvidia.com,
        mmaddireddy@nvidia.com, sagar.tv@gmail.com,
        Ricky Wu <ricky_wu@realtek.com>,
        Rajat Jain <rajatja@google.com>,
        Prasad Malisetty <quic_pmaliset@quicinc.com>,
        Victor Ding <victording@google.com>
Subject: Re: [PATCH V1] PCI/ASPM: Save/restore L1SS Capability for
 suspend/resume
Message-ID: <20220413132644.GA673431@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220412225047.GA627910@bhelgaas>
Precedence: bulk

On Tue, Apr 12, 2022 at 05:50:47PM -0500, Bjorn Helgaas wrote:
> ...
> I think we should try this patch again with some changes and maybe
> some debug logging:
> 
>   - I wonder if we should integrate the LTR, L1 SS, and Link Control
>     ASPM restore instead of having them spread around through
>     pci_restore_ltr_state(), pci_restore_aspm_l1ss_state(), and
>     pci_restore_pcie_state().  Maybe a new pci_restore_aspm() that
>     would be called from pci_restore_pcie_state()?

Sorry, this was a dumb idea, please ignore it.  Maybe it would be
useful in the future sometime, but right now when we're trying to
understand the issue, the code churn would just confuse things.  I
think we need minimal changes right now (like 4257f7e008ea).

>   - For L1 PM Substates configuration, sec 5.5.4 says that both ports
>     must be configured while ASPM L1 is disabled, but I don't think we
>     currently guarantee this: we restore all the upstream component
>     state first, and we don't know the ASPM state of the downstream
>     one.  Maybe we need to:
> 
>       * When restoring upstream component,
>           + disable its ASPM
> 
>       * When restoring downstream component,
>           + disable its ASPM
> 	  + restore upstream component's LTR, L1SS
> 	  + restore downstream component's LTR, L1SS
> 	  + restore upstream component's ASPM
> 	  + restore downstream component's ASPM
> 
>       This seems pretty messy, but seems like what the spec requires.

Actually, I think it's unlikely that a downstream device would have
ASPM enabled while we're restoring state of the upstream device, since 
we're probably restoring state after a reset or coming back from
D3cold.  But we may still need to wait to enable the upstream device
ASPM until after configuring it in the downstream device.

Logging of the previous & new state of these registers might help
untangle things.

>     - Add some pci_dbg() logging of all these save/restore values to
>       help debug any issues.