Re: [PATCHv4 next 0/3] Limiting pci access

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sat, Jan 21, 2017 at 08:31:40AM +0100, Lukas Wunner wrote:
> On Fri, Jan 20, 2017 at 04:35:50PM -0500, Keith Busch wrote:
> > On Tue, Dec 13, 2016 at 04:19:32PM -0500, Keith Busch wrote:
> > > On Tue, Dec 13, 2016 at 02:50:12PM -0600, Bjorn Helgaas wrote:
> > > > And we're apparently still doing a lot of these accesses?  I'm still
> > > > curious about exactly what these are, because it may be that we're
> > > > doing more than necessary.
> > > 
> > > It's the MSI-x masking that's our next highest contributor. Masking
> > > vectors still requires non-posted commands, and since they're not going
> > > through managed API accessors like config space uses, the removed flag
> > > is needed for checking before doing significant MMIO.
> > 
> > Hi Bjorn,
> > 
> > Just wanted to do another check with you on this. We'd still like to fence
> > off all config access with appropriate error codes, and short cut the most
> > significant MMIO access to improve surprise removal. There may still be
> > other offenders, but these are the most important ones we've identified.
> > 
> > I've updated the series to make the new flag an atomic accessor as
> > requested, and improved the change logs with more information compelling
> > the change. Otherwise it's much the same as before. I know you weren't
> > keen on capturing all the access under the umbrella of improving device
> > unbinding time, but the general concensus among device makers is that
> > it's a good thing to have software return an error early rather than
> > send a command we know will fail. Any other thoughts I should consider
> > before posting v5?
> 
> I think Bjorn was pondering whether a flag to indicate surprise removal
> should be put in struct device rather than struct pci_dev, so as to
> cover other buses capable of surprise removal.  There's already an
> "offline" flag in struct device which is set when user space initiates
> a safe hot removal via sysfs.
> 
> Bjorn cc'ed his e-mails of Dec 13 to Greg KH and Alan Stern but got no
> replies.

Sorry, I don't recall seeing those :(

> @Greg KH:
> Would you entertain a patch adding a bit to struct device which indicates
> the device was surprise removed?  The PCIe Hotplug and PCIe Downstream
> Port Containment drivers are both able to recognize surprise removal and
> can set the bit.
> 
> When removing the device we currently perform numerous accesses to config
> space in the PCI core.  Additionally the driver for the removed device
> (e.g. network driver, GPU driver) performs mmio accesses to orderly shut
> down the device.  E.g. when unplugging the Apple Thunderbolt Ethernet
> adapter the kernel currently locks up as the tg3 driver tries to shutdown
> the removed device.  If we had a bit to indicate surprise removal we could
> handle this properly in the PCI core and device driver ->remove hooks.
> 
> For comparison, this is what macOS recommends to driver developers:
> 
>        "PCI device drivers are typically developed with the expectation
> 	that the device will not be removed from the PCI bus during its
> 	operation. However, Thunderbolt technology allows PCI data to be
> 	tunneled through a Thunderbolt connection, and the Thunderbolt
> 	cables may be unplugged from the host or device at any time.
> 	Therefore, the system must be able to cope with the removal of
> 	PCI devices by the user at any time.
> 
> 	The PCI device drivers used with Thunderbolt devices may need to
> 	be updated in order to handle surprise or unplanned removal.
> 	In particular, MMIO cycles and PCI Configuration accesses require
> 	special attention. [...] As a basic guideline, developers should
> 	modify their drivers to handle a return value of 0xFFFFFFFF.
> 	If any thread, callback, interrupt filter, or code path in a
> 	driver receives 0xFFFFFFFF indicating the device has been
> 	unplugged, then all threads, callbacks, interrupt filters,
> 	interrupt handlers, and other code paths in that driver must
> 	cease MMIO reads and writes immediately and prepare for
> 	termination. [...]
> 
> 	Once it has been determined that a device is no longer connected,
> 	do not try to clean up or reset the hardware as attempts to
> 	communicate with the hardware may lead to further delays. [...]
> 	A typical way for a developer to solve this problem is to provide
> 	a single bottleneck routine for all MMIO reads and have that
> 	routine check the status of the device before beginning the actual
> 	transaction."
> 
> 	Source: https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics02/Basics02.html
> 
> We lack a comparable functionality and the question is whether to
> support it only in the PCI core or in a more general fashion in the
> driver core.  Other buses (such as USB) have to support surprise
> removal as well.

PCI devices have _ALWAYS_ had to handle supprise removal, and the MacOS
guidelines are the exact same thing that we have been telling Linux
kernel developers for years.

So no, a supprise removal flag should not be needed, your driver should
already be handling this problem today (if it isn't, it needs to be
fixed.)

Don't try to rely on some out-of-band notification that your device is
removed, just do what you write above and handle it that way in your
driver.  There are lots of examples of this in the kernel today, are you
concerned about any specific driver that does not do this properly?

And really, all busses need to handle their device being removed at any
point in time anyway, so the reliance on a "supprise" flag would not
help much, as usually that is only known _after_ the fact and the device
would have already started to report other types of errors.

So what "benifit" would a supprise removal flag provide?  Other than
some PCI busses could set this, and many others could not, so a driver
would have to be changed to now support both types of detection,
_adding_ work to drivers, not making it easier by any means :(

thanks,

greg k-h