On Tue, 2018-09-25 at 09:35 -0600, Keith Busch wrote: > On Mon, Sep 24, 2018 at 06:10:01PM -0700, Benjamin Herrenschmidt wrote: > > On Tue, 2018-09-18 at 17:57 -0600, Keith Busch wrote: > > > > .../... > > > > Any reason why you don't do cmpxchg as I originally suggested (sorry > > I've been away and may have missed some previous emails) > > That was to block a device hot removal on error_detected and error_resume, > or vice versa. > > > > -/* pci_dev priv_flags */ > > > -#define PCI_DEV_DISCONNECTED 0 > > > -#define PCI_DEV_ADDED 1 > > > +/** > > > + * pci_dev_set_io_state - Set the new error state if possible. > > > + * > > > + * @dev - pci device to set new error_state > > > + * @new - the state we want dev to be in > > > + * > > > + * Must be called with device_lock held. > > > > This won't work for PowerPC EEH. We will change the state from a > > readl() so at interrupt time or any other context. > > > > We really need the cmpxchg variant. > > These are private interfaces. EEH can't call them from any context. That means EEH will have to re-implement that logic to change the IO state, which doesn't make sense. There should be a single interface for use by everybody who can set the IO state and enforces the various state transition rules. That interface should use a cmpxchg. It doesn't make sense to require the device_lock in that low level state setter. That you may want to do it in the higher level code to prevent device removal vs. calling the error callbacks is fine, but it's orthogonal to changing the IO state variable. Cheers, Ben.
