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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Apr 2019 12:35:51 -0500
From:   Alex G <mr.nuke.me@...il.com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        Alex_Gagniuc@...lteam.com
Cc:     bhelgaas@...gle.com, helgaas@...nel.org, linux-pci@...r.kernel.org,
        Austin.Bolen@...l.com, keith.busch@...el.com, Shyam.Iyer@...l.com,
        lukas@...ner.de, okaya@...nel.org, torvalds@...ux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: Add link_change error handler and vfio-pci user

On 4/24/19 12:19 PM, Alex Williamson wrote:
> On Wed, 24 Apr 2019 16:45:45 +0000
> <Alex_Gagniuc@...lteam.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