Re: [PATCH 04/14] powerpc/eeh: Check slot presence state in eeh_handle_normal_event()

[Sam Bobroff <sbobroff@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 5051 bytes --]

On Tue, Sep 03, 2019 at 08:15:55PM +1000, Oliver O'Halloran wrote:
> When a device is surprise removed while undergoing IO we will probably
> get an EEH PE freeze due to MMIO timeouts and other errors. When a freeze
> is detected we send a recovery event to the EEH worker thread which will
> notify drivers, and perform recovery as needed.
> 
> In the event of a hot-remove we don't want recovery to occur since there
> isn't a device to recover. The recovery process is fairly long due to
> the number of wait states (required by PCIe) which causes problems when
> devices are removed and replaced (e.g. hot swapping of U.2 NVMe drives).
> 
> To determine if we need to skip the recovery process we can use the
> get_adapter_state() operation of the hotplug_slot to determine if the
> slot contains a device or not, and if the slot is empty we can skip
> recovery entirely.
> 
> One thing to note is that the slot being EEH frozen does not prevent the
> hotplug driver from working. We don't have the EEH recovery thread
> remove any of the devices since it's assumed that the hotplug driver
> will handle tearing down the slot state.
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>

Looks good, but some comments, below.

> ---
>  arch/powerpc/kernel/eeh_driver.c | 60 ++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 18a69fac4d80..52ce7584af43 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -27,6 +27,7 @@
>  #include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/pci_hotplug.h>
>  #include <asm/eeh.h>
>  #include <asm/eeh_event.h>
>  #include <asm/ppc-pci.h>
> @@ -769,6 +770,46 @@ static void eeh_pe_cleanup(struct eeh_pe *pe)
>  	}
>  }
>  
> +/**
> + * eeh_check_slot_presence - Check if a device is still present in a slot
> + * @pdev: pci_dev to check
> + *
> + * This function may return a false positive if we can't determine the slot's
> + * presence state. This might happen for for PCIe slots if the PE containing
> + * the upstream bridge is also frozen, or the bridge is part of the same PE
> + * as the device.
> + *
> + * This shouldn't happen often, but you might see it if you hotplug a PCIe
> + * switch.
> + */

I don't think the function name is very good; it does check the slot but
it doesn't tell you what a true result means -- but I don't see an
obviously great alternative either. If it can return false positives, it's
really testing for empty so maybe 'eeh_slot_definitely_empty()' or
'eeh_slot_maybe_populated()'?

> +static bool eeh_slot_presence_check(struct pci_dev *pdev)
> +{
> +	const struct hotplug_slot_ops *ops;
> +	struct pci_slot *slot;
> +	u8 state;
> +	int rc;
> +
> +	if (!pdev)
> +		return false;
> +
> +	if (pdev->error_state == pci_channel_io_perm_failure)
> +		return false;
> +
> +	slot = pdev->slot;
> +	if (!slot || !slot->hotplug)
> +		return true;
> +
> +	ops = slot->hotplug->ops;
> +	if (!ops || !ops->get_adapter_status)
> +		return true;
> +
> +	rc = ops->get_adapter_status(slot->hotplug, &state);
> +	if (rc)
> +		return true;
> +
> +	return !!state;
> +}
> +
>  /**
>   * eeh_handle_normal_event - Handle EEH events on a specific PE
>   * @pe: EEH PE - which should not be used after we return, as it may
> @@ -799,6 +840,7 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
>  	enum pci_ers_result result = PCI_ERS_RESULT_NONE;
>  	struct eeh_rmv_data rmv_data =
>  		{LIST_HEAD_INIT(rmv_data.removed_vf_list), 0};
> +	int devices = 0;
>  
>  	bus = eeh_pe_bus_get(pe);
>  	if (!bus) {
> @@ -807,6 +849,23 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
>  		return;
>  	}
>  
> +	/*
> +	 * When devices are hot-removed we might get an EEH due to
> +	 * a driver attempting to touch the MMIO space of a removed
> +	 * device. In this case we don't have a device to recover
> +	 * so suppress the event if we can't find any present devices.
> +	 *
> +	 * The hotplug driver should take care of tearing down the
> +	 * device itself.
> +	 */
> +	eeh_for_each_pe(pe, tmp_pe)
> +		eeh_pe_for_each_dev(tmp_pe, edev, tmp)
> +			if (eeh_slot_presence_check(edev->pdev))
> +				devices++;

In other parts of the EEH code we do a get_device() on edev->pdev before
passing it around, it might be good to do that here too.

> +
> +	if (!devices)
> +		goto out; /* nothing to recover */

Does this handle an empty, but frozen, PHB correctly? (Can that happen?)

> +
>  	eeh_pe_update_time_stamp(pe);
>  	pe->freeze_count++;
>  	if (pe->freeze_count > eeh_max_freezes) {
> @@ -997,6 +1056,7 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
>  		}
>  	}
>  
> +out:
>  	/*
>  	 * Clean up any PEs without devices. While marked as EEH_PE_RECOVERYING
>  	 * we don't want to modify the PE tree structure so we do it here.
> -- 
> 2.21.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]