diff mbox series

[6/9] powerpc/eeh: Clarify arguments to eeh_reset_device()

Message ID c9191611543d221d5f003fad4a68dd5f86839404.1520294174.git.sam.bobroff@au1.ibm.com (mailing list archive)
State Superseded
Headers show
Series EEH refactoring 1 | expand

Commit Message

Sam Bobroff March 5, 2018, 11:59 p.m. UTC
It is currently difficult to understand the behaviour of
eeh_reset_device() due to the way it's parameters are used. In
particular, when 'bus' is NULL, it's value is still necessary so the
same value is looked up again locally under a different name
('frozen_bus') but behaviour is changed.

To clarify this, add a new parameter 'eeh_aware_driver', and have the
caller set it when it would have passed NULL for 'bus' and always pass
a value for 'bus'. Then change any test that was on 'bus' to one on
'!eeh_aware_driver' and replace uses of 'frozen_bus' with 'bus'.

Also update the function's comment.

This should not change behaviour.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 arch/powerpc/kernel/eeh_driver.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Russell Currey March 6, 2018, 4:37 a.m. UTC | #1
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> It is currently difficult to understand the behaviour of
> eeh_reset_device() due to the way it's parameters are used. In
> particular, when 'bus' is NULL, it's value is still necessary so the
> same value is looked up again locally under a different name
> ('frozen_bus') but behaviour is changed.
> 
> To clarify this, add a new parameter 'eeh_aware_driver', and have the
> caller set it when it would have passed NULL for 'bus' and always
> pass
> a value for 'bus'. Then change any test that was on 'bus' to one on
> '!eeh_aware_driver' and replace uses of 'frozen_bus' with 'bus'.
> 
> Also update the function's comment.
> 
> This should not change behaviour.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
>  arch/powerpc/kernel/eeh_driver.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index cb584d72b0a5..6c3577133223 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -619,17 +619,19 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>  
>  /**
>   * eeh_reset_device - Perform actual reset of a pci slot
> + * @eeh_aware_driver: Does the device's driver provide EEH support?
>   * @pe: EEH PE
>   * @bus: PCI bus corresponding to the isolcated slot
> + * @rmv_data: Optional, list to record removed devices
>   *
>   * This routine must be called to do reset on the indicated PE.
>   * During the reset, udev might be invoked because those affected
>   * PCI devices will be removed and then added.
>   */

Just a nitpick here, I would prefer that the PE remain the first
argument of the function since that's one thing that's pretty standard
across the EEH implementation.  I'd also say that "driver_aware" is
clearer than "aware_driver".

Reviewed-by: Russell Currey <ruscur@russell.cc>
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index cb584d72b0a5..6c3577133223 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -619,17 +619,19 @@  int eeh_pe_reset_and_recover(struct eeh_pe *pe)
 
 /**
  * eeh_reset_device - Perform actual reset of a pci slot
+ * @eeh_aware_driver: Does the device's driver provide EEH support?
  * @pe: EEH PE
  * @bus: PCI bus corresponding to the isolcated slot
+ * @rmv_data: Optional, list to record removed devices
  *
  * This routine must be called to do reset on the indicated PE.
  * During the reset, udev might be invoked because those affected
  * PCI devices will be removed and then added.
  */
-static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
-				struct eeh_rmv_data *rmv_data)
+static int eeh_reset_device(bool eeh_aware_driver,
+			    struct eeh_pe *pe, struct pci_bus *bus,
+			    struct eeh_rmv_data *rmv_data)
 {
-	struct pci_bus *frozen_bus = eeh_pe_bus_get(pe);
 	time64_t tstamp;
 	int cnt, rc;
 	struct eeh_dev *edev;
@@ -645,7 +647,7 @@  static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
 	 * into pci_hp_add_devices().
 	 */
 	eeh_pe_state_mark(pe, EEH_PE_KEEP);
-	if (bus) {
+	if (!eeh_aware_driver) {
 		if (pe->type & EEH_PE_VF) {
 			eeh_pe_dev_traverse(pe, eeh_rmv_device, NULL);
 		} else {
@@ -653,7 +655,7 @@  static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
 			pci_hp_remove_devices(bus);
 			pci_unlock_rescan_remove();
 		}
-	} else if (frozen_bus) {
+	} else if (bus) {
 		eeh_pe_dev_traverse(pe, eeh_rmv_device, rmv_data);
 	}
 
@@ -689,7 +691,7 @@  static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
 	 * the device up before the scripts have taken it down,
 	 * potentially weird things happen.
 	 */
-	if (bus) {
+	if (!eeh_aware_driver) {
 		pr_info("EEH: Sleep 5s ahead of complete hotplug\n");
 		ssleep(5);
 
@@ -706,7 +708,7 @@  static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
 			eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
 			pci_hp_add_devices(bus);
 		}
-	} else if (frozen_bus && rmv_data->removed) {
+	} else if (bus && rmv_data->removed) {
 		pr_info("EEH: Sleep 5s ahead of partial hotplug\n");
 		ssleep(5);
 
@@ -715,7 +717,7 @@  static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
 		if (pe->type & EEH_PE_VF)
 			eeh_add_virt_device(edev, NULL);
 		else
-			pci_hp_add_devices(frozen_bus);
+			pci_hp_add_devices(bus);
 	}
 	eeh_pe_state_clear(pe, EEH_PE_KEEP);
 
@@ -820,7 +822,7 @@  void eeh_handle_normal_event(struct eeh_pe *pe)
 	 */
 	if (result == PCI_ERS_RESULT_NONE) {
 		pr_info("EEH: Reset with hotplug activity\n");
-		rc = eeh_reset_device(pe, bus, NULL);
+		rc = eeh_reset_device(false, pe, bus, NULL);
 		if (rc) {
 			pr_warn("%s: Unable to reset, err=%d\n",
 				__func__, rc);
@@ -872,7 +874,7 @@  void eeh_handle_normal_event(struct eeh_pe *pe)
 	/* If any device called out for a reset, then reset the slot */
 	if (result == PCI_ERS_RESULT_NEED_RESET) {
 		pr_info("EEH: Reset without hotplug activity\n");
-		rc = eeh_reset_device(pe, NULL, &rmv_data);
+		rc = eeh_reset_device(true, pe, bus, &rmv_data);
 		if (rc) {
 			pr_warn("%s: Cannot reset, err=%d\n",
 				__func__, rc);