On Thu, Jun 15, 2023 at 01:41:10AM +0100, Maciej W. Rozycki wrote:
> On Wed, 14 Jun 2023, Bjorn Helgaas wrote:
>
> > > This is v9 of the change to work around a PCIe link training phenomenon
> > > where a pair of devices both capable of operating at a link speed above
> > > 2.5GT/s seems unable to negotiate the link speed and continues training
> > > indefinitely with the Link Training bit switching on and off repeatedly
> > > and the data link layer never reaching the active state.
> > >
> > > With several requests addressed and a few extra issues spotted this
> > > version has now grown to 14 patches. It has been verified for device
> > > enumeration with and without PCI_QUIRKS enabled, using the same piece of
> > > RISC-V hardware as previously. Hot plug or reset events have not been
> > > verified, as this is difficult if at all feasible with hardware in
> > > question.
> > static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout)
> > {
> > - bool retrain = true;
> > int delay = 1;
> > + bool retrain = false;
> > + struct pci_dev *bridge;
> > +
> > + if (pci_is_pcie(dev)) {
> > + retrain = true;
> > + bridge = pci_upstream_bridge(dev);
> > + }
>
> 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.
Bjorn
