On Friday 25 June 2021 15:19:36 Bjorn Helgaas wrote:

On Mon, Jun 21, 2021 at 04:28:55PM +0200, Pali Rohár wrote:

quoted

On Wednesday 16 June 2021 16:38:19 Bjorn Helgaas wrote:

quoted

On Wed, Jun 02, 2021 at 09:03:02PM +0200, Pali Rohár wrote:

quoted

On Wednesday 02 June 2021 10:55:59 Bjorn Helgaas wrote:

quoted

On Wed, Jun 02, 2021 at 02:08:16PM +0200, Pali Rohár wrote:

quoted

On Tuesday 01 June 2021 19:00:36 Bjorn Helgaas wrote:

quoted

quoted

I wonder if this could be restructured as a generic quirk
in quirks.c that simply set the bridge's TLS to 2.5 GT/s
during enumeration.  Or would the retrain fail even in
that case?

If I understand it correctly then PCIe link is already up
when kernel starts enumeration. So setting Bridge TLS to 2.5
GT/s does not change anything here.

Moreover it would have side effect that cards which are
already set to 5+ GT/s would be downgraded to 2.5 GT/s
during enumeration and for increasing speed would be needed
another round of "enumeration" to set a new TLS and retrain
link again. As TLS affects link only after link goes into
Recovery state.

So this would just complicate card enumeration and settings.

The current quirk complicates the ASPM code.  I'm hoping that
if we set the bridge's Target Link Speed during enumeration,
the link retrain will "just work" without complicating the
ASPM code.

An enumeration quirk wouldn't have to set the bridge's TLS to
2.5 GT/s; the quirk would be attached to specific endpoint
devices and could set the bridge's TLS to whatever the
endpoint supports.

Now I see what you mean. Yes, I agree this is a good idea and
can simplify code. Quirk is not related to ASPM code and
basically has nothing with it, just I put it into aspm.c because
this is the only place where link retraining was activated.

But with this proposal there is one issue. Some kernel drivers
already overwrite PCI_EXP_LNKCTL2_TLS value. So if PCI
enumeration code set some value into PCI_EXP_LNKCTL2_TLS bits
then drivers can change it and once ASPM will try to retrain
link this may cause this issue.

I guess you mean the amdgpu, radeon, and hfi1 drivers.  They
really shouldn't be mucking with that stuff anyway.  But they do
and are unlikely to change because we don't have any good
alternative.

Yea, these are examples of such drivers... Maybe it is a good idea
to ask those people why changing PCI_EXP_LNKCTL2_TLS is needed. As
these drivers are often derived from codebase of shared multisystem
drivers or from common documentation, it is possible that original
source has this code as a workaround or common pattern used in other
operating systems, not related to linux...

quoted

One way around that would be to add some quirk code to
pcie_capability_write_word().  Ugly, but we do have something sort
of similar in pcie_capability_read_word() already.

Bjorn, do you really want such ugly hack in
pcie_capability_write_word?  It is common code used and called from
lot of places so it may affect whole system if in future somebody
changes it again...

I don't know which is uglier, a quirk in pcie_capability_write_word()
or a quirk in aspm.c that has nothing to do with ASPM.  They're both
ugly :)

Ok :-)

FWIW, in pcie_capability_write_word() I would envision not a check for
Atheros, but rather something like a "dev->max_target_link_speed" that
could be set by an Atheros quirk.  It does get uglier if we want to
restrict the bridge's link speed via a quirk, then unrestrict it when
the endpoint is unplugged.

I know pcie_downgrade_link_to_gen1() only returns failure for corner
cases that "should not occur,"

It is not only corner case. It happens _always_ with at least two
pci drivers.

As I wrote in other email due to this issue, some quirk code which
allows / disallows link retraining is required in aspm.c file.

but I don't like the fact that it's
possible to change Common Clock Configuration without doing the
retrain.  That would leave us with incorrect ASPM exit latencies,
which is really hard to debug.

I see... Any idea how to solve this issue?

Here's the relevant text in the spec (PCIe r5.0):

  7.5.3.6 Link Capabilities

    L0s Exit Latency - This field indicates the L0s exit latency for
    the given PCI Express Link. The value reported indicates the
    length of time this Port requires to complete transition from L0s
    to L0. ...

    Note that exit latencies may be influenced by PCI Express
    reference clock configuration depending upon whether a component
    uses a common or separate reference clock.

  7.5.3.6 Link Control
    Common Clock Configuration - When Set, this bit indicates that
    this component and the component at the opposite end of this Link
    are operating with a distributed common reference clock. ...

    After changing the value in this bit in both components on a Link,
    software must trigger the Link to retrain by writing a 1b to the
    Retrain Link bit of the Downstream Port.