On Fri, Nov 09, 2018 at 08:29:53AM +0100, Lukas Wunner wrote:

On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote:

quoted

On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote:

quoted

I'm having second thoughts about this.  One thing I'm uncomfortable
with is that sprinkling pci_dev_is_disconnected() around feels ad hoc

I think my stance always has been that this call is not good at all
because once you call it you never really know if it is still true as
the device could have been removed right afterward.

So almost any code that relies on it is broken, there is no locking and
it can and will race and you will loose.

Hm, to be honest if that's your impression I think you must have missed a
large portion of the discussion we've been having over the past 2 years.

Please consider reading this LWN article, particularly the "Surprise
removal" section, to get up to speed:

https://lwn.net/Articles/767885/

You seem to be assuming that all we care about is the *return value* of
an mmio read.  However a transaction to a surprise removed device has
side effects beyond returning all ones, such as a Completion Timeout
which, with thousands of transactions in flight, added up to many seconds
to handle removal of an NVMe array and occasionally caused MCEs.

Again, I still claim this is broken hardware/firmware :)

It is not an option to just blindly carry out device accesses even though
it is known the device is gone, Completion Timeouts be damned.

I don't disagree with you at all, and your other email is great with
summarizing the issues here.

What I do object to is somehow relying on that function call as knowing
that the device really is present or not.  It's a good hint, yes, but
driver authors still have to be able to handle the bad data coming back
from when the call races with the device being removed.

However there is more to it than just Completion Timeouts, this is all
detailed in the LWN article.

And that's a great article and your work here is much appreciated.  I
think we are in violent agreement :)

thanks,

greg k-h