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)
> -/* 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.
> + * Returns true if state has been changed to the requested state.
> + */
> +static inline bool pci_dev_set_io_state(struct pci_dev *dev,
> + pci_channel_state_t new)
> +{
> + bool changed = false;
> +
> + device_lock_assert(&dev->dev);
> + switch (new) {
> + case pci_channel_io_perm_failure:
> + switch (dev->error_state) {
> + case pci_channel_io_frozen:
> + case pci_channel_io_normal:
> + case pci_channel_io_perm_failure:
> + changed = true;
> + break;
> + }
> + break;
> + case pci_channel_io_frozen:
> + switch (dev->error_state) {
> + case pci_channel_io_frozen:
> + case pci_channel_io_normal:
> + changed = true;
> + break;
> + }
> + break;
> + case pci_channel_io_normal:
> + switch (dev->error_state) {
> + case pci_channel_io_frozen:
> + case pci_channel_io_normal:
> + changed = true;
> + break;
> + }
> + break;
> + }
> + if (changed)
> + dev->error_state = new;
> + return changed;
> +}
>
> static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
> {
> - set_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
> + device_lock(&dev->dev);
> + pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
> + device_unlock(&dev->dev);
> +
> return 0;
> }
>
> static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
> {
> - return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
> + return dev->error_state == pci_channel_io_perm_failure;
> }
>
> +/* pci_dev priv_flags */
> +#define PCI_DEV_ADDED 0
> +
> static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
> {
> assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 31e8a4314384..4da2a62b4f77 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -52,9 +52,8 @@ static int report_error_detected(struct pci_dev *dev,
> const struct pci_error_handlers *err_handler;
>
> device_lock(&dev->dev);
> - dev->error_state = state;
> -
> - if (!dev->driver ||
> + if (!pci_dev_set_io_state(dev, state) ||
> + !dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->error_detected) {
> /*
> @@ -130,9 +129,8 @@ static int report_resume(struct pci_dev *dev, void *data)
> const struct pci_error_handlers *err_handler;
>
> device_lock(&dev->dev);
> - dev->error_state = pci_channel_io_normal;
> -
> - if (!dev->driver ||
> + if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> + !dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->resume)
> goto out;
