[PATCH V7 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[Rajat Jain]

On Fri, Apr 7, 2017 at 2:03 AM, Patel, Mayurkumar
<mayurkumar.patel at intel.com> wrote:
> Hi Rajat
>
>
>>On Thu, Apr 6, 2017 at 8:19 AM, Sinan Kaya <okaya at codeaurora.org> wrote:
>>>
>>> On 4/6/2017 9:23 AM, Patel, Mayurkumar wrote:
>>> > Actually, The enabling/configuring of ASPM L1.2 is opposite then ASPM L1.
>>> > In case of L1, Upstream must configure first L1 and then downstream according to PCIe spec.
>>> > In case of L1.2, Downstream must configure L1.2 and then upstream according (to L1.2 ECN spec. -> 5.5.4. L1 PM
>>> > substates Configuration).
>>> >
>>> > @Bjorn:
>>> > I even found that programming of pcie_config_aspm_l1ss() sub-states is done in the opposite way than described in
>>> > the spec.,
>>> >
>>> > The spec. says following and correct me If I am wrong or I misinterpret the spec. :
>>> >
>>> > 5.5.4. L1 PM Substates Configuration
>>> >
>>> > The Setting of any enable bit must be performed at the Downstream Port before the
>>> > corresponding bit is permitted to be Set at the Upstream Port. If any L1 PM Substates enable
>>> > bit is at a later time to be cleared, the enable bit(s) must be cleared in the Upstream Port
>>> > before the corresponding enable bit(s) are permitted to be cleared in the Downstream Port.
>>> >
>>
>>Thanks for bringing to attention. My understanding / interpretation of
>>"Downstream port" was the port pointing downwards (from the "Upstream"
>>component). E.g. when an EP connects to a hub port, PCIe text refers
>>to the hub port as the "downstream port". Similarly "upstream port" is
>>used for referring to a switch's port that "points" upwards towards
>>the root port. Thus, I interpreted the text to mean that I need to
>>enable it in the "downstream port" (in the root port / switch port)
>>followed by the "upstream port" (in the device).
>>
>>It would have been great if the PCIe spec was as clear for L1
>>substates as it was for L1:
>>----------------------------
>>ASPM L1 must be enabled by software in the Upstream
>>component on a Link prior to enabling ASPM L1 in the
>>Downstream component on that Link.
>>-----------------------------
>>For ASPM L1, the spec clearly says "Upstream component" which can only
>>mean the switch's "downstream" port. I'm open to changing it if there
>>is consensus that my interpretation is wrong.
>
> In fact, Your understanding seems to be correct. It was my mistake that I raised it without
> actually, keeping in mind or understanding port/component difference which I didn't notice.
> My sincere apologies for that.

No worries, yes, it is tricky and even I had missed it when I
implemented it first. :-)

>
>>
>>I'm actually not sure if I understood what is the problem that is
>>being seen with L1 PM substates. Can you please elaborate?
>>
>
>
> The actual problem, what Sinan and me were seeing with ASPM L1 with POLICY_DEFAULT
> as described in https://patchwork.kernel.org/patch/9548321/

OK, I understood this problem that you are trying to solve. Lets call
this problem (1). Sorry, I haven't yet looked at your patches, and I
will take a look.

>
> Sinan worked out patches to resolve the issue but on my platform now I am starting to see
> that ASPM L1SS gets impacted by these patches.

OK, lets call this problem (2) - are you saying that this only impacts
L1SS and not L1? Sorry, just trying to understand because I can't
think of anyway L1 and L1SS bits are handled differently..

> Basically, with his patches, when Policy is set to default,
> and if no PCIe EP is connected to Root Port during boot up, BIOS does not configure L1SS for it, on my
> platform. So the link->aspm_default in aspm.c stores configuration without L1SS.

I understand uptil here:
No Device on boot, ASPM Policy=default => BIOS does not enable any
ASPM bits => kernel does the same.

> When the EP gets
> connected afterwards, due to link->aspm_default set to non L1SS configuration, even if BIOS
> sets ASPM L1SS for Root Port and Endpoint both,

Sorry, I don't think I followed this part. You are saying when the EP
gets hot-plugged later (after booting up with no EP), the BIOS sets
ASPM L1SS for root port & endpoint at that time??

> it gets cleared in the aspm driver.
>
> Without the patches Sinan has worked, it was a different issue as described above, and L1/L1SS were
> getting always enabled by the BIOS for endpoint and RP, but kernel enabled/disabled them alternatively.
>
> So basically, I am not currently 100% sure, what is the proper fix for this. Whether BIOS should
> enable L1SS on Root Port no matter Endpoint is connected or not during boot?

I'd think not.

While I don't understand the problem (2) currently, wouldn't you face
the same problem (2) with L1? Are you seeing L1 & L1SS bits handled
differently in your use case / problem case?

Thanks,

Rajat

>
> Or I am not sure, whether could we should still update link->aspm_default partly for L1SS in the old patch way
> https://patchwork.ozlabs.org/patch/736771/
>
> Any feedback would be helpful then may be if required I can or if Sinan can work out the patches for L1SS also fine for me.
>
>
>>
>>>
>>>
>>> Thanks for testing.
>>>
>>> commit a142f4d3e5c54db5e942fa6ee5f3dc0e8c83207b
>>> Author: Rajat Jain <rajatja at google.com>
>>> Date:   Mon Jan 2 22:34:15 2017 -0800
>>>
>>>     PCI/ASPM: Add comment about L1 substate latency
>>>
>>>     Since the exit latencies for L1 substates are not advertised by a device,
>>>     it is not clear in spec how to do a L1 substate exit latency check.  We
>>>     assume that the L1 exit latencies advertised by a device include L1
>>>     substate latencies (and hence do not do any check).  If that is not true,
>>>     we should do some sort of check here.
>>>
>>>     (I'm not clear about what that check should like currently. I'd be glad to
>>>     take up any suggestions).
>>>
>>>     Signed-off-by: Rajat Jain <rajatja at google.com>
>>>     Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
>>>
>>> I added Rajat for discussion on L1SS. I think this deserves its own discussion.
>>
>>Thanks, The above commit specifically adds a comment to
>>pcie_aspm_check_latency(), because I wanted to leave a note
>>highlighting that potentially, we could add a more stringent check for
>>exit latency for L1SS. But that has nothing to do with how we are
>>configuring or enabling / disabling the L1 substates.
>>
>>Thanks & Best Regards,
>>
>>Rajat
>>
>>> I'll look at the other part of your email and move things around a little bit
>>> less aggressively for the aspm_default.
>>>
>>>
>>> --
>>> Sinan Kaya
>>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
>>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Christian Lamprechter
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928