Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected

[Oliver O'Halloran]

On Fri, 2018-11-09 at 08:11 +0100, Lukas Wunner wrote:
> On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote:
> > +	/*
> > +	 * If an MMIO read from the device returns ~0 data, that data may
> > +	 * be valid, or it may indicate a bus error.  If config space is
> > +	 * readable, assume it's valid data; otherwise, assume a bus error.
> > +	 */
> > +	if (val == ~0) {
> > +		pci_read_config_dword(dev, PCI_VENDOR_ID, &id);
> > +		if (id == ~0)
> > +			pci_dev_set_disconnected(dev, NULL);
> > +	}
> 
> This isn't safe unfortunately because "all ones" may occur for other
> reasons besides disconnectedness.  E.g. on an Uncorrectable Error,
> the device may likewise respond with all ones, but revert to valid
> responses if the error can be recovered through a Secondary Bus Reset.
> In such a case, marking the device disconnected would be inappropriate.

I don't really see why we're trying to make a distinction between
recoverable errors and disconnected devices at this stage. In either
case we should assume the device is broken and shouldn't be accessed
until we perform some kind of recovery action.

Bjorn's MMIO wrappers are more-or-less an opt-in software emulation of
the freeze-MMIO-on-error behaviour that the EEH mechanism provides on 
IBM hardware so I think it makes sense. It also has the nice side
effect of giving driver writers an error injection mechanism so they
might actually test how their drivers deal with errors.

> Accessing a device in D3cold would be another example where all ones
> is returned both from mmio and config space despite the device still
> being present and future accesses having a chance to succeed.

Is doing a MMIO to a device in D3cold (or hot) ever a valid thing to
do?

> In fact, in v2 of Keith's patches adding pci_dev_set_disconnected()
> he attempted the same as what you're doing here and caused issues
> for me with devices in D3cold:
> 
> https://spinics.net/lists/linux-pci/msg54337.html
> 
> 
> > One thing I'm uncomfortable with is that [...].  Another is that the
> > only place we call pci_dev_set_disconnected() is in pciehp and acpiphp,
> > so the only "disconnected" case we catch is if hotplug happens to be
> > involved.
> 
> Yes, that's because the hotplug drivers are the only ones who can
> identify removal authoritatively and unambiguously.  They *know*
> when the device is gone and don't have to resort to heuristics
> such as all ones.  (ISTR that dpc also marks devices disconnected.)

The herustics shouldn't be used to work out when the device is gone,
rather they should be used to work out when we need to check on the
device.

One of the grosser bits of EEH support is a hook in readl() and friends
that checks for a 0xFF response. If it finds one, it looks at the EEH
state and will start the recovery process if the device is marked as
frozen.

(don't look at the code. you won't like what you find)

> > sprinkling pci_dev_is_disconnected() around feels ad hoc
> > instead of systematic, in the sense that I don't know how we convince
> > ourselves that this (and only this) is the correct place to put it.
> 
> We need to add documentation for driver authors how to deal with
> surprise removal.  Briefly:
> 
> * If (pdev->error_state == pci_channel_io_perm_failure), the device
>   is definitely gone and any further device access can be skipped.
>   Otherwise presence of the device is likely, but not guaranteed.
>
> * If a device access can significantly delay device removal due to
>   Completion Timeouts, or can cause an infinite loop, MCE or crash,
>   do check pdev->error_state before carrying out the device access.
> 
> * Always be prepared that a device access may fail due to surprise
>   removal, do not blindly trust mmio or config space reads or
>   assume success of writes.

Completely agree. We really need better documentation of what drivers
should be doing.

Oliver