From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f42.google.com ([209.85.220.42]:36138 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751023AbbEINlM (ORCPT ); Sat, 9 May 2015 09:41:12 -0400 Received: by pabsx10 with SMTP id sx10so72799377pab.3 for ; Sat, 09 May 2015 06:41:11 -0700 (PDT) Message-ID: <554E0E71.2080200@ozlabs.ru> Date: Sat, 09 May 2015 23:41:05 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 To: Gavin Shan , linuxppc-dev@lists.ozlabs.org CC: linux-pci@vger.kernel.org, benh@kernel.crashing.org, bhelgaas@google.com Subject: Re: [PATCH v4 09/21] powerpc/powernv: Use PCI slot reset infrastructure References: <1430460188-31343-1-git-send-email-gwshan@linux.vnet.ibm.com> <1430460188-31343-10-git-send-email-gwshan@linux.vnet.ibm.com> In-Reply-To: <1430460188-31343-10-git-send-email-gwshan@linux.vnet.ibm.com> Content-Type: text/plain; charset=koi8-r; format=flowed Sender: linux-pci-owner@vger.kernel.org List-ID: On 05/01/2015 04:02 PM, Gavin Shan wrote: > For PowerNV platform, running on top of skiboot, all PE level reset > should be routed to firmware if the bridge of the PE primary bus has > device-node property "ibm,reset-by-firmware". Otherwise, the kernel > has to issue hot reset on PE's primary bus despite the requested reset > types, which is the behaviour before the firmware supports PCI slot > reset. So the changes don't depend on the PCI slot reset capability > exposed from the firmware. > > Signed-off-by: Gavin Shan > --- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/include/asm/opal.h | 4 +- > arch/powerpc/platforms/powernv/eeh-powernv.c | 206 +++++++++++++-------------- > 3 files changed, 102 insertions(+), 109 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index c5eb86f..2793d24 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -190,6 +190,7 @@ enum { > #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */ > #define EEH_RESET_HOT 1 /* Hot reset */ > #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */ > +#define EEH_RESET_COMPLETE 4 /* PHB complete reset */ > #define EEH_LOG_TEMP 1 /* EEH temporary error log */ > #define EEH_LOG_PERM 2 /* EEH permanent error log */ > > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h > index 042af1a..6d467df 100644 > --- a/arch/powerpc/include/asm/opal.h > +++ b/arch/powerpc/include/asm/opal.h > @@ -129,7 +129,7 @@ int64_t opal_pci_map_pe_dma_window(uint64_t phb_id, uint16_t pe_number, uint16_t > int64_t opal_pci_map_pe_dma_window_real(uint64_t phb_id, uint16_t pe_number, > uint16_t dma_window_number, uint64_t pci_start_addr, > uint64_t pci_mem_size); > -int64_t opal_pci_reset(uint64_t phb_id, uint8_t reset_scope, uint8_t assert_state); > +int64_t opal_pci_reset(uint64_t id, uint8_t reset_scope, uint8_t assert_state); > > int64_t opal_pci_get_hub_diag_data(uint64_t hub_id, void *diag_buffer, > uint64_t diag_buffer_len); > @@ -145,7 +145,7 @@ int64_t opal_get_epow_status(__be64 *status); > int64_t opal_set_system_attention_led(uint8_t led_action); > int64_t opal_pci_next_error(uint64_t phb_id, __be64 *first_frozen_pe, > __be16 *pci_error_type, __be16 *severity); > -int64_t opal_pci_poll(uint64_t phb_id); > +int64_t opal_pci_poll(uint64_t id, uint8_t *val); > int64_t opal_return_cpu(void); > int64_t opal_check_token(uint64_t token); > int64_t opal_reinit_cpus(uint64_t flags); > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index ce738ab..3c01095 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -742,12 +742,12 @@ static int pnv_eeh_get_state(struct eeh_pe *pe, int *delay) > return ret; > } > > -static s64 pnv_eeh_phb_poll(struct pnv_phb *phb) > +static s64 pnv_eeh_poll(uint64_t id) > { > s64 rc = OPAL_HARDWARE; > > while (1) { > - rc = opal_pci_poll(phb->opal_id); > + rc = opal_pci_poll(id, NULL); > if (rc <= 0) > break; > > @@ -763,84 +763,38 @@ static s64 pnv_eeh_phb_poll(struct pnv_phb *phb) > int pnv_eeh_phb_reset(struct pci_controller *hose, int option) > { > struct pnv_phb *phb = hose->private_data; > + uint8_t scope; > s64 rc = OPAL_HARDWARE; > > pr_debug("%s: Reset PHB#%x, option=%d\n", > __func__, hose->global_number, option); > - > - /* Issue PHB complete reset request */ > - if (option == EEH_RESET_FUNDAMENTAL || > - option == EEH_RESET_HOT) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_COMPLETE, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_DEACTIVATE) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_COMPLETE, > - OPAL_DEASSERT_RESET); > - if (rc < 0) > - goto out; > - > - /* > - * Poll state of the PHB until the request is done > - * successfully. The PHB reset is usually PHB complete > - * reset followed by hot reset on root bus. So we also > - * need the PCI bus settlement delay. > - */ > - rc = pnv_eeh_phb_poll(phb); > - if (option == EEH_RESET_DEACTIVATE) { > - if (system_state < SYSTEM_RUNNING) > - udelay(1000 * EEH_PE_RST_SETTLE_TIME); > - else > - msleep(EEH_PE_RST_SETTLE_TIME); These udelay() and msleep() are gone. How come they are not needed anymore? Worth commenting in the commit log or remove those in a separate patch. I just remember you mentioning some missing delays somewhere which caused NVIDIA device to issue EEH and I do not want those to disappear :) > + switch (option) { > + case EEH_RESET_HOT: > + scope = OPAL_RESET_PCI_HOT; > + break; > + case EEH_RESET_FUNDAMENTAL: > + scope = OPAL_RESET_PCI_FUNDAMENTAL; > + break; > + case EEH_RESET_COMPLETE: > + scope = OPAL_RESET_PHB_COMPLETE; > + break; > + case EEH_RESET_DEACTIVATE: > + return 0; > + default: > + pr_warn("%s: Unsupported option %d\n", > + __func__, option); > + return -EINVAL; > } > -out: > - if (rc != OPAL_SUCCESS) > - return -EIO; > > - return 0; > -} > - > -static int pnv_eeh_root_reset(struct pci_controller *hose, int option) > -{ > - struct pnv_phb *phb = hose->private_data; > - s64 rc = OPAL_HARDWARE; > + /* Issue reset and poll until it's completed */ > + rc = opal_pci_reset(phb->opal_id, scope, OPAL_ASSERT_RESET); > + if (rc > 0) > + rc = pnv_eeh_poll(phb->opal_id); > > - pr_debug("%s: Reset PHB#%x, option=%d\n", > - __func__, hose->global_number, option); > - > - /* > - * During the reset deassert time, we needn't care > - * the reset scope because the firmware does nothing > - * for fundamental or hot reset during deassert phase. > - */ > - if (option == EEH_RESET_FUNDAMENTAL) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_FUNDAMENTAL, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_HOT) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_HOT, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_DEACTIVATE) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_HOT, > - OPAL_DEASSERT_RESET); > - if (rc < 0) > - goto out; > - > - /* Poll state of the PHB until the request is done */ > - rc = pnv_eeh_phb_poll(phb); > - if (option == EEH_RESET_DEACTIVATE) > - msleep(EEH_PE_RST_SETTLE_TIME); > -out: > - if (rc != OPAL_SUCCESS) > - return -EIO; > - > - return 0; > + return (rc == OPAL_SUCCESS) ? 0 : -EIO; > } > > -static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > +static int __pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > { > struct pci_dn *pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn); > struct eeh_dev *edev = pdn_to_eeh_dev(pdn); > @@ -891,14 +845,57 @@ static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > return 0; > } > > +static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > +{ > + struct pci_controller *hose; > + struct pnv_phb *phb; > + struct device_node *dn = dev ? pci_device_to_OF_node(dev) : NULL; > + uint64_t id = (0x1ul << 60); > + uint8_t scope; > + s64 rc; int64_t for @rc? > + > + /* > + * If the firmware can't handle it, we will issue hot reset > + * on the secondary bus despite the requested reset type > + */ > + if (!dn || !of_get_property(dn, "ibm,reset-by-firmware", NULL)) > + return __pnv_eeh_bridge_reset(dev, option); > + > + /* The firmware can handle the request */ > + switch (option) { > + case EEH_RESET_HOT: > + scope = OPAL_RESET_PCI_HOT; > + break; > + case EEH_RESET_FUNDAMENTAL: > + scope = OPAL_RESET_PCI_FUNDAMENTAL; > + break; > + case EEH_RESET_DEACTIVATE: > + return 0; > + case EEH_RESET_COMPLETE: > + default: > + pr_warn("%s: Unsupported option %d on device %s\n", > + __func__, option, pci_name(dev)); > + return -EINVAL; > + } This is the same switch as earlier in this patch (slightly different order). Move it and opal_pci_reset() into a helper and call it pnv_opal_pci_reset()? > + > + hose = pci_bus_to_host(dev->bus); > + phb = hose->private_data; Previously you would initialize @hose and @phb where you declared those but not here. If you did the same thing as before, the patch could have been smaller and easier to read. > + id |= (dev->bus->number << 24) | (dev->devfn << 16) | phb->opal_id; > + rc = opal_pci_reset(id, scope, OPAL_ASSERT_RESET); > + if (rc > 0) > + rc = pnv_eeh_poll(id); > + > + return (rc == OPAL_SUCCESS) ? 0 : -EIO; > +} > + > void pnv_pci_reset_secondary_bus(struct pci_dev *dev) > { > struct pci_controller *hose; > > if (pci_is_root_bus(dev->bus)) { > hose = pci_bus_to_host(dev->bus); > - pnv_eeh_root_reset(hose, EEH_RESET_HOT); > - pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); > + pnv_eeh_phb_reset(hose, EEH_RESET_HOT); > + pnv_eeh_phb_reset(hose, EEH_RESET_DEACTIVATE); > } else { > pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); > pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); > @@ -920,8 +917,9 @@ void pnv_pci_reset_secondary_bus(struct pci_dev *dev) > static int pnv_eeh_reset(struct eeh_pe *pe, int option) > { > struct pci_controller *hose = pe->phb; > + struct pnv_phb *phb; > struct pci_bus *bus; > - int ret; > + s64 rc; > > /* > * For PHB reset, we always have complete reset. For those PEs whose > @@ -937,43 +935,37 @@ static int pnv_eeh_reset(struct eeh_pe *pe, int option) > * reset. The side effect is that EEH core has to clear the frozen > * state explicitly after BAR restore. > */ > - if (pe->type & EEH_PE_PHB) { > - ret = pnv_eeh_phb_reset(hose, option); > - } else { > - struct pnv_phb *phb; > - s64 rc; > + if (pe->type & EEH_PE_PHB) I would keep "{" in the line above .... > + return pnv_eeh_phb_reset(hose, EEH_RESET_COMPLETE); ...put "} else {" here... and the chunk below would become 1) very small 2) very trivial... And then you could make a trivial patch which would do scope removal but without functional changes. Or vice versa. > > - /* > - * The frozen PE might be caused by PAPR error injection > - * registers, which are expected to be cleared after hitting > - * frozen PE as stated in the hardware spec. Unfortunately, > - * that's not true on P7IOC. So we have to clear it manually > - * to avoid recursive EEH errors during recovery. > - */ > - phb = hose->private_data; > - if (phb->model == PNV_PHB_MODEL_P7IOC && > - (option == EEH_RESET_HOT || > - option == EEH_RESET_FUNDAMENTAL)) { > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_ERROR, > - OPAL_ASSERT_RESET); > - if (rc != OPAL_SUCCESS) { > - pr_warn("%s: Failure %lld clearing " > - "error injection registers\n", > - __func__, rc); > - return -EIO; > - } > + /* > + * The frozen PE might be caused by PAPR error injection > + * registers, which are expected to be cleared after hitting > + * frozen PE as stated in the hardware spec. Unfortunately, > + * that's not true on P7IOC. So we have to clear it manually > + * to avoid recursive EEH errors during recovery. > + */ > + phb = hose->private_data; > + if (phb->model == PNV_PHB_MODEL_P7IOC && > + (option == EEH_RESET_HOT || > + option == EEH_RESET_FUNDAMENTAL)) { > + rc = opal_pci_reset(phb->opal_id, > + OPAL_RESET_PHB_ERROR, > + OPAL_ASSERT_RESET); > + if (rc != OPAL_SUCCESS) { > + pr_warn("%s: Failure %lld clearing error " > + "injection registers on PHB#%d\n", > + __func__, rc, hose->global_number); > + return -EIO; > } > - > - bus = eeh_pe_bus_get(pe); > - if (pci_is_root_bus(bus) || > - pci_is_root_bus(bus->parent)) > - ret = pnv_eeh_root_reset(hose, option); > - else > - ret = pnv_eeh_bridge_reset(bus->self, option); > } > > - return ret; > + /* Route the reset request to PHB or upstream bridge */ > + bus = eeh_pe_bus_get(pe); > + if (pci_is_root_bus(bus)) > + return pnv_eeh_phb_reset(hose, option); > + > + return pnv_eeh_bridge_reset(bus->self, option); > } > > /** > -- Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f42.google.com (mail-pa0-f42.google.com [209.85.220.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 72B421A0E2B for ; Sat, 9 May 2015 23:41:13 +1000 (AEST) Received: by pacwv17 with SMTP id wv17so72846599pac.0 for ; Sat, 09 May 2015 06:41:11 -0700 (PDT) Message-ID: <554E0E71.2080200@ozlabs.ru> Date: Sat, 09 May 2015 23:41:05 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 To: Gavin Shan , linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v4 09/21] powerpc/powernv: Use PCI slot reset infrastructure References: <1430460188-31343-1-git-send-email-gwshan@linux.vnet.ibm.com> <1430460188-31343-10-git-send-email-gwshan@linux.vnet.ibm.com> In-Reply-To: <1430460188-31343-10-git-send-email-gwshan@linux.vnet.ibm.com> Content-Type: text/plain; charset=koi8-r; format=flowed Cc: bhelgaas@google.com, linux-pci@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 05/01/2015 04:02 PM, Gavin Shan wrote: > For PowerNV platform, running on top of skiboot, all PE level reset > should be routed to firmware if the bridge of the PE primary bus has > device-node property "ibm,reset-by-firmware". Otherwise, the kernel > has to issue hot reset on PE's primary bus despite the requested reset > types, which is the behaviour before the firmware supports PCI slot > reset. So the changes don't depend on the PCI slot reset capability > exposed from the firmware. > > Signed-off-by: Gavin Shan > --- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/include/asm/opal.h | 4 +- > arch/powerpc/platforms/powernv/eeh-powernv.c | 206 +++++++++++++-------------- > 3 files changed, 102 insertions(+), 109 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index c5eb86f..2793d24 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -190,6 +190,7 @@ enum { > #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */ > #define EEH_RESET_HOT 1 /* Hot reset */ > #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */ > +#define EEH_RESET_COMPLETE 4 /* PHB complete reset */ > #define EEH_LOG_TEMP 1 /* EEH temporary error log */ > #define EEH_LOG_PERM 2 /* EEH permanent error log */ > > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h > index 042af1a..6d467df 100644 > --- a/arch/powerpc/include/asm/opal.h > +++ b/arch/powerpc/include/asm/opal.h > @@ -129,7 +129,7 @@ int64_t opal_pci_map_pe_dma_window(uint64_t phb_id, uint16_t pe_number, uint16_t > int64_t opal_pci_map_pe_dma_window_real(uint64_t phb_id, uint16_t pe_number, > uint16_t dma_window_number, uint64_t pci_start_addr, > uint64_t pci_mem_size); > -int64_t opal_pci_reset(uint64_t phb_id, uint8_t reset_scope, uint8_t assert_state); > +int64_t opal_pci_reset(uint64_t id, uint8_t reset_scope, uint8_t assert_state); > > int64_t opal_pci_get_hub_diag_data(uint64_t hub_id, void *diag_buffer, > uint64_t diag_buffer_len); > @@ -145,7 +145,7 @@ int64_t opal_get_epow_status(__be64 *status); > int64_t opal_set_system_attention_led(uint8_t led_action); > int64_t opal_pci_next_error(uint64_t phb_id, __be64 *first_frozen_pe, > __be16 *pci_error_type, __be16 *severity); > -int64_t opal_pci_poll(uint64_t phb_id); > +int64_t opal_pci_poll(uint64_t id, uint8_t *val); > int64_t opal_return_cpu(void); > int64_t opal_check_token(uint64_t token); > int64_t opal_reinit_cpus(uint64_t flags); > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index ce738ab..3c01095 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -742,12 +742,12 @@ static int pnv_eeh_get_state(struct eeh_pe *pe, int *delay) > return ret; > } > > -static s64 pnv_eeh_phb_poll(struct pnv_phb *phb) > +static s64 pnv_eeh_poll(uint64_t id) > { > s64 rc = OPAL_HARDWARE; > > while (1) { > - rc = opal_pci_poll(phb->opal_id); > + rc = opal_pci_poll(id, NULL); > if (rc <= 0) > break; > > @@ -763,84 +763,38 @@ static s64 pnv_eeh_phb_poll(struct pnv_phb *phb) > int pnv_eeh_phb_reset(struct pci_controller *hose, int option) > { > struct pnv_phb *phb = hose->private_data; > + uint8_t scope; > s64 rc = OPAL_HARDWARE; > > pr_debug("%s: Reset PHB#%x, option=%d\n", > __func__, hose->global_number, option); > - > - /* Issue PHB complete reset request */ > - if (option == EEH_RESET_FUNDAMENTAL || > - option == EEH_RESET_HOT) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_COMPLETE, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_DEACTIVATE) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_COMPLETE, > - OPAL_DEASSERT_RESET); > - if (rc < 0) > - goto out; > - > - /* > - * Poll state of the PHB until the request is done > - * successfully. The PHB reset is usually PHB complete > - * reset followed by hot reset on root bus. So we also > - * need the PCI bus settlement delay. > - */ > - rc = pnv_eeh_phb_poll(phb); > - if (option == EEH_RESET_DEACTIVATE) { > - if (system_state < SYSTEM_RUNNING) > - udelay(1000 * EEH_PE_RST_SETTLE_TIME); > - else > - msleep(EEH_PE_RST_SETTLE_TIME); These udelay() and msleep() are gone. How come they are not needed anymore? Worth commenting in the commit log or remove those in a separate patch. I just remember you mentioning some missing delays somewhere which caused NVIDIA device to issue EEH and I do not want those to disappear :) > + switch (option) { > + case EEH_RESET_HOT: > + scope = OPAL_RESET_PCI_HOT; > + break; > + case EEH_RESET_FUNDAMENTAL: > + scope = OPAL_RESET_PCI_FUNDAMENTAL; > + break; > + case EEH_RESET_COMPLETE: > + scope = OPAL_RESET_PHB_COMPLETE; > + break; > + case EEH_RESET_DEACTIVATE: > + return 0; > + default: > + pr_warn("%s: Unsupported option %d\n", > + __func__, option); > + return -EINVAL; > } > -out: > - if (rc != OPAL_SUCCESS) > - return -EIO; > > - return 0; > -} > - > -static int pnv_eeh_root_reset(struct pci_controller *hose, int option) > -{ > - struct pnv_phb *phb = hose->private_data; > - s64 rc = OPAL_HARDWARE; > + /* Issue reset and poll until it's completed */ > + rc = opal_pci_reset(phb->opal_id, scope, OPAL_ASSERT_RESET); > + if (rc > 0) > + rc = pnv_eeh_poll(phb->opal_id); > > - pr_debug("%s: Reset PHB#%x, option=%d\n", > - __func__, hose->global_number, option); > - > - /* > - * During the reset deassert time, we needn't care > - * the reset scope because the firmware does nothing > - * for fundamental or hot reset during deassert phase. > - */ > - if (option == EEH_RESET_FUNDAMENTAL) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_FUNDAMENTAL, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_HOT) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_HOT, > - OPAL_ASSERT_RESET); > - else if (option == EEH_RESET_DEACTIVATE) > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PCI_HOT, > - OPAL_DEASSERT_RESET); > - if (rc < 0) > - goto out; > - > - /* Poll state of the PHB until the request is done */ > - rc = pnv_eeh_phb_poll(phb); > - if (option == EEH_RESET_DEACTIVATE) > - msleep(EEH_PE_RST_SETTLE_TIME); > -out: > - if (rc != OPAL_SUCCESS) > - return -EIO; > - > - return 0; > + return (rc == OPAL_SUCCESS) ? 0 : -EIO; > } > > -static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > +static int __pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > { > struct pci_dn *pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn); > struct eeh_dev *edev = pdn_to_eeh_dev(pdn); > @@ -891,14 +845,57 @@ static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > return 0; > } > > +static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option) > +{ > + struct pci_controller *hose; > + struct pnv_phb *phb; > + struct device_node *dn = dev ? pci_device_to_OF_node(dev) : NULL; > + uint64_t id = (0x1ul << 60); > + uint8_t scope; > + s64 rc; int64_t for @rc? > + > + /* > + * If the firmware can't handle it, we will issue hot reset > + * on the secondary bus despite the requested reset type > + */ > + if (!dn || !of_get_property(dn, "ibm,reset-by-firmware", NULL)) > + return __pnv_eeh_bridge_reset(dev, option); > + > + /* The firmware can handle the request */ > + switch (option) { > + case EEH_RESET_HOT: > + scope = OPAL_RESET_PCI_HOT; > + break; > + case EEH_RESET_FUNDAMENTAL: > + scope = OPAL_RESET_PCI_FUNDAMENTAL; > + break; > + case EEH_RESET_DEACTIVATE: > + return 0; > + case EEH_RESET_COMPLETE: > + default: > + pr_warn("%s: Unsupported option %d on device %s\n", > + __func__, option, pci_name(dev)); > + return -EINVAL; > + } This is the same switch as earlier in this patch (slightly different order). Move it and opal_pci_reset() into a helper and call it pnv_opal_pci_reset()? > + > + hose = pci_bus_to_host(dev->bus); > + phb = hose->private_data; Previously you would initialize @hose and @phb where you declared those but not here. If you did the same thing as before, the patch could have been smaller and easier to read. > + id |= (dev->bus->number << 24) | (dev->devfn << 16) | phb->opal_id; > + rc = opal_pci_reset(id, scope, OPAL_ASSERT_RESET); > + if (rc > 0) > + rc = pnv_eeh_poll(id); > + > + return (rc == OPAL_SUCCESS) ? 0 : -EIO; > +} > + > void pnv_pci_reset_secondary_bus(struct pci_dev *dev) > { > struct pci_controller *hose; > > if (pci_is_root_bus(dev->bus)) { > hose = pci_bus_to_host(dev->bus); > - pnv_eeh_root_reset(hose, EEH_RESET_HOT); > - pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); > + pnv_eeh_phb_reset(hose, EEH_RESET_HOT); > + pnv_eeh_phb_reset(hose, EEH_RESET_DEACTIVATE); > } else { > pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); > pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); > @@ -920,8 +917,9 @@ void pnv_pci_reset_secondary_bus(struct pci_dev *dev) > static int pnv_eeh_reset(struct eeh_pe *pe, int option) > { > struct pci_controller *hose = pe->phb; > + struct pnv_phb *phb; > struct pci_bus *bus; > - int ret; > + s64 rc; > > /* > * For PHB reset, we always have complete reset. For those PEs whose > @@ -937,43 +935,37 @@ static int pnv_eeh_reset(struct eeh_pe *pe, int option) > * reset. The side effect is that EEH core has to clear the frozen > * state explicitly after BAR restore. > */ > - if (pe->type & EEH_PE_PHB) { > - ret = pnv_eeh_phb_reset(hose, option); > - } else { > - struct pnv_phb *phb; > - s64 rc; > + if (pe->type & EEH_PE_PHB) I would keep "{" in the line above .... > + return pnv_eeh_phb_reset(hose, EEH_RESET_COMPLETE); ...put "} else {" here... and the chunk below would become 1) very small 2) very trivial... And then you could make a trivial patch which would do scope removal but without functional changes. Or vice versa. > > - /* > - * The frozen PE might be caused by PAPR error injection > - * registers, which are expected to be cleared after hitting > - * frozen PE as stated in the hardware spec. Unfortunately, > - * that's not true on P7IOC. So we have to clear it manually > - * to avoid recursive EEH errors during recovery. > - */ > - phb = hose->private_data; > - if (phb->model == PNV_PHB_MODEL_P7IOC && > - (option == EEH_RESET_HOT || > - option == EEH_RESET_FUNDAMENTAL)) { > - rc = opal_pci_reset(phb->opal_id, > - OPAL_RESET_PHB_ERROR, > - OPAL_ASSERT_RESET); > - if (rc != OPAL_SUCCESS) { > - pr_warn("%s: Failure %lld clearing " > - "error injection registers\n", > - __func__, rc); > - return -EIO; > - } > + /* > + * The frozen PE might be caused by PAPR error injection > + * registers, which are expected to be cleared after hitting > + * frozen PE as stated in the hardware spec. Unfortunately, > + * that's not true on P7IOC. So we have to clear it manually > + * to avoid recursive EEH errors during recovery. > + */ > + phb = hose->private_data; > + if (phb->model == PNV_PHB_MODEL_P7IOC && > + (option == EEH_RESET_HOT || > + option == EEH_RESET_FUNDAMENTAL)) { > + rc = opal_pci_reset(phb->opal_id, > + OPAL_RESET_PHB_ERROR, > + OPAL_ASSERT_RESET); > + if (rc != OPAL_SUCCESS) { > + pr_warn("%s: Failure %lld clearing error " > + "injection registers on PHB#%d\n", > + __func__, rc, hose->global_number); > + return -EIO; > } > - > - bus = eeh_pe_bus_get(pe); > - if (pci_is_root_bus(bus) || > - pci_is_root_bus(bus->parent)) > - ret = pnv_eeh_root_reset(hose, option); > - else > - ret = pnv_eeh_bridge_reset(bus->self, option); > } > > - return ret; > + /* Route the reset request to PHB or upstream bridge */ > + bus = eeh_pe_bus_get(pe); > + if (pci_is_root_bus(bus)) > + return pnv_eeh_phb_reset(hose, option); > + > + return pnv_eeh_bridge_reset(bus->self, option); > } > > /** > -- Alexey