Re: [PATCH V2 1/1] PCI/ASPM: Work around link retrain errata of Pericom PCIe-to-PCI bridges

[Stefan Mätje <Stefan.Maetje@xxxxxx>]

Am 06.03.19 um 08:03 schrieb Andy Shevchenko:
> On Tue, Mar 05, 2019 at 06:31:22PM +0100, Stefan Mätje wrote:
:
:
>> The patch introduces a new flag clear_retrain_link in the struct pci_dev. This
>> flag is set in quirk_enable_clear_retrain_link() for the affected devices in
>> the pci_fixup_header in quirks.c
>>
>> The link retraining code is moved to pcie_retrain_link(). This function now
>> applies the work around to clear the PCI_EXP_LNKCTL_RL bit again if
>> clear_retrain_link bit is set in the pci_dev structure of the link parent
>> device.
> 
> 
>> +	/* Wait for link training end. Break out after waiting for timeout */
>> +	start_jiffies = jiffies;
>> +	for (;;) {
>> +		pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
>> +		if (!(reg16 & PCI_EXP_LNKSTA_LT))
>> +			break;
>> +		if (time_after(jiffies, start_jiffies + LINK_RETRAIN_TIMEOUT))
>> +			break;
>> +		msleep(1);
>> +	}
> 
> I see this is existing code, though two comments (perhaps for future clean up):
> 	- msleep(1) is not guaranteed to be 1 ms at all (in some cases it might be less)

	Even if msleep(1) would return after a very short time it would not do any harm
	here. It is only needed to do "some" sleeping in this status reading loop.

> 	- better to use do {} while (time_before()) loop

	I will change that.

> 
>> +static void quirk_enable_clear_retrain_link(struct pci_dev *dev)
>> +{
>> +	dev->clear_retrain_link = 1;
> 
>> +	pci_info(dev, "Enable Pericom link retrain quirk\n");
> 
> If some other device would appear with same issue, but from another vendor this
> becomes misleading.
> 
> I would recommend to refer to an issue the quirk is about, and not the vendor.

I think that would be better, too. I'll change that.

>> +}
>