Re: [PATCH v9 00/14] pci: Work around ASMedia ASM2824 PCIe link training failures

Maciej W. Rozycki
2023-06-16T12:28:04Z

On Thu, 15 Jun 2023, Bjorn Helgaas wrote:

> > If doing it this way, which I actually like, I think it would be a little
> > bit better performance- and style-wise if this was written as:
> >
> > if (pci_is_pcie(dev)) {
> > bridge = pci_upstream_bridge(dev);
> > retrain = !!bridge;
> > }
> >
> > (or "retrain = bridge != NULL" if you prefer this style), and then we
> > don't have to repeatedly check two variables iff (pcie && !bridge) in the
> > loop below:
>
> Done, thanks, I do like that better. I did:
>
> bridge = pci_upstream_bridge(dev);
> if (bridge)
> retrain = true;
>
> because it seems like it flows more naturally when reading.

Perfect, and good timing too, as I have just started checking your tree
as your message arrived. I ran my usual tests with and w/o PCI_QUIRKS
enabled and results were as expected. As before I didn't check hot plug
and reset paths as these features are awkward with the HiFive Unmatched
system involved.

I have skimmed over the changes as committed to pci/enumeration and found
nothing suspicious. I have verified that the tree builds as at each of
them with my configuration.

As per my earlier remark:

> I think making a system halfway-fixed would make little sense, but with
> the actual fix actually made last as you suggested I think this can be
> split off, because it'll make no functional change by itself.

I am not perfectly happy with your rearrangement to fold the !PCI_QUIRKS
stub into the change carrying the actual workaround and then have the
reset path update with a follow-up change only, but I won't fight over it.
It's only one tree revision that will be in this halfway-fixed state and
I'll trust your judgement here.

Let me know if anything pops up related to these changes anytime and I'll
be happy to look into it. The system involved is nearing two years since
its deployment already, but hopefully it has many years to go yet and will
continue being ready to verify things. It's not that there's lots of real
RISC-V hardware available, let alone with PCI/e connectivity.

Thank you for staying with me and reviewing this patch series through all
the iterations.

Maciej