Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

[Alex G <mr.nuke.me@gmail.com>]

On 4/24/19 12:19 PM, Alex Williamson wrote:
> On Wed, 24 Apr 2019 16:45:45 +0000
> <Alex_Gagniuc@Dellteam.com> wrote:
>> On 4/23/2019 5:42 PM, Alex Williamson wrote:
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 7e12d0163863..233cd4b5b6e8 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)
>>
>> I don't think you want to change pcie_report_downtraining(). You're
>> advertising to "report" something, by nomenclature, but then go around
>> and also call a notification callback. This is also used during probe,
>> and you've now just killed your chance to notice you've booted with a
>> degraded link.
>> If what you want to do is silence the bandwidth notification, you want
>> to modify the threaded interrupt that calls this.
> 
> During probe, ie. discovery, a device wouldn't have a driver attached,
> so we'd fall through to simply printing the link status.  Nothing
> lost afaict.  The "report" verb doesn't have a subject here, report to
> whom?  Therefore I thought it reasonable that a driver ask that it be
> reported to them via a callback.  I don't see that as such a stretch of
> the interface.

That's just stretching the logic, and IMO makes the intent harder to 
understand. The argument relies on the state of the PCI device and 
logic, which is not obvious to the casual observer. If you want to 
bypass the bandwidth notification, then bypass the notification.

>>>    	if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
>>>    		return;
>>>    
>>> +	/*
>>> +	 * If driver handles link_change event, defer to driver.  PCIe drivers
>>> +	 * can call pcie_print_link_status() to print current link info.
>>> +	 */
>>> +	device_lock(&dev->dev);
>>> +	if (dev->driver && dev->driver->err_handler &&
>>> +	    dev->driver->err_handler->link_change) {
>>> +		dev->driver->err_handler->link_change(dev);
>>> +		device_unlock(&dev->dev);
>>> +		return;
>>> +	}
>>> +	device_unlock(&dev->dev);
>>
>> Can we write this such that there is a single lock()/unlock() pair?
> 
> Not without introducing a tracking variable, ex.
[snip bad code]
> That's not markedly better imo, but if it's preferred I can send a v2.

How about:

if (!invoke_link_changed_handler(pdev))
	very_useful_downtraining_message(pdev);

> Alex
Alex