| Message ID | 20200706013619.459420-6-oohall@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/14] powerpc/eeh: Remove eeh_dev_phb_init_dynamic() | expand |
On 06/07/2020 11:36, Oliver O'Halloran wrote: > This is used in precisely one place which is in pseries specific platform > code. There's no need to have the callback in eeh_ops since the platform > chooses the EEH PE addresses anyway. The PowerNV implementation has always > been a stub too so remove it. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > arch/powerpc/include/asm/eeh.h | 1 - > arch/powerpc/platforms/powernv/eeh-powernv.c | 13 ------------ > arch/powerpc/platforms/pseries/eeh_pseries.c | 22 ++++++++++---------- > 3 files changed, 11 insertions(+), 25 deletions(-) > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > index 3d648e042835..1bddc0dfe099 100644 > --- a/arch/powerpc/include/asm/eeh.h > +++ b/arch/powerpc/include/asm/eeh.h > @@ -220,7 +220,6 @@ struct eeh_ops { > int (*init)(void); > struct eeh_dev *(*probe)(struct pci_dev *pdev); > int (*set_option)(struct eeh_pe *pe, int option); > - int (*get_pe_addr)(struct eeh_pe *pe); > int (*get_state)(struct eeh_pe *pe, int *delay); > int (*reset)(struct eeh_pe *pe, int option); > int (*get_log)(struct eeh_pe *pe, int severity, char *drv_log, unsigned long len); > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index 79409e005fcd..bcd0515d8f79 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -535,18 +535,6 @@ static int pnv_eeh_set_option(struct eeh_pe *pe, int option) > return 0; > } > > -/** > - * pnv_eeh_get_pe_addr - Retrieve PE address > - * @pe: EEH PE > - * > - * Retrieve the PE address according to the given tranditional > - * PCI BDF (Bus/Device/Function) address. > - */ > -static int pnv_eeh_get_pe_addr(struct eeh_pe *pe) > -{ > - return pe->addr; > -} > - > static void pnv_eeh_get_phb_diag(struct eeh_pe *pe) > { > struct pnv_phb *phb = pe->phb->private_data; > @@ -1670,7 +1658,6 @@ static struct eeh_ops pnv_eeh_ops = { > .init = pnv_eeh_init, > .probe = pnv_eeh_probe, > .set_option = pnv_eeh_set_option, > - .get_pe_addr = pnv_eeh_get_pe_addr, > .get_state = pnv_eeh_get_state, > .reset = pnv_eeh_reset, > .get_log = pnv_eeh_get_log, > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > index 18a2522b9b5e..088771fa38be 100644 > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > @@ -32,6 +32,8 @@ > #include <asm/ppc-pci.h> > #include <asm/rtas.h> > > +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn); > + > /* RTAS tokens */ > static int ibm_set_eeh_option; > static int ibm_set_slot_reset; > @@ -301,7 +303,7 @@ void pseries_eeh_init_edev(struct pci_dn *pdn) > eeh_edev_dbg(edev, "EEH failed to enable on device (code %d)\n", ret); > } else { > /* Retrieve PE address */ > - edev->pe_config_addr = eeh_ops->get_pe_addr(&pe); > + edev->pe_config_addr = pseries_eeh_get_pe_addr(pdn); > pe.addr = edev->pe_config_addr; > > /* Some older systems (Power4) allow the ibm,set-eeh-option > @@ -431,8 +433,10 @@ static int pseries_eeh_set_option(struct eeh_pe *pe, int option) > * It's notable that zero'ed return value means invalid PE config > * address. > */ > -static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn) > { > + int config_addr = rtas_config_addr(pdn->busno, pdn->devfn, 0); Why not use pe->config_addr (and why we have two addresses in eeh_pe anyway)? Ah, I guess I just trust you with this one :) Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > + int buid = pdn->phb->buid; > int ret = 0; > int rets[3]; > > @@ -443,18 +447,16 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > * meaningless. > */ > ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, > - pe->config_addr, BUID_HI(pe->phb->buid), > - BUID_LO(pe->phb->buid), 1); > + config_addr, BUID_HI(buid), BUID_LO(buid), 1); > if (ret || (rets[0] == 0)) > return 0; > > /* Retrieve the associated PE config address */ > ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, > - pe->config_addr, BUID_HI(pe->phb->buid), > - BUID_LO(pe->phb->buid), 0); > + config_addr, BUID_HI(buid), BUID_LO(buid), 0); > if (ret) { > pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", > - __func__, pe->phb->global_number, pe->config_addr); > + __func__, pdn->phb->global_number, config_addr); > return 0; > } > > @@ -463,11 +465,10 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > > if (ibm_get_config_addr_info != RTAS_UNKNOWN_SERVICE) { > ret = rtas_call(ibm_get_config_addr_info, 4, 2, rets, > - pe->config_addr, BUID_HI(pe->phb->buid), > - BUID_LO(pe->phb->buid), 0); > + config_addr, BUID_HI(buid), BUID_LO(buid), 0); > if (ret) { > pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", > - __func__, pe->phb->global_number, pe->config_addr); > + __func__, pdn->phb->global_number, config_addr); > return 0; > } > > @@ -839,7 +840,6 @@ static struct eeh_ops pseries_eeh_ops = { > .init = pseries_eeh_init, > .probe = pseries_eeh_probe, > .set_option = pseries_eeh_set_option, > - .get_pe_addr = pseries_eeh_get_pe_addr, > .get_state = pseries_eeh_get_state, > .reset = pseries_eeh_reset, > .get_log = pseries_eeh_get_log, >
On Mon, Jul 13, 2020 at 7:54 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 06/07/2020 11:36, Oliver O'Halloran wrote: > > This is used in precisely one place which is in pseries specific platform > > code. There's no need to have the callback in eeh_ops since the platform > > chooses the EEH PE addresses anyway. The PowerNV implementation has always > > been a stub too so remove it. > > > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > > --- > > arch/powerpc/include/asm/eeh.h | 1 - > > arch/powerpc/platforms/powernv/eeh-powernv.c | 13 ------------ > > arch/powerpc/platforms/pseries/eeh_pseries.c | 22 ++++++++++---------- > > 3 files changed, 11 insertions(+), 25 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > > index 3d648e042835..1bddc0dfe099 100644 > > --- a/arch/powerpc/include/asm/eeh.h > > +++ b/arch/powerpc/include/asm/eeh.h > > @@ -220,7 +220,6 @@ struct eeh_ops { > > int (*init)(void); > > struct eeh_dev *(*probe)(struct pci_dev *pdev); > > int (*set_option)(struct eeh_pe *pe, int option); > > - int (*get_pe_addr)(struct eeh_pe *pe); > > int (*get_state)(struct eeh_pe *pe, int *delay); > > int (*reset)(struct eeh_pe *pe, int option); > > int (*get_log)(struct eeh_pe *pe, int severity, char *drv_log, unsigned long len); > > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > > index 79409e005fcd..bcd0515d8f79 100644 > > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > > @@ -535,18 +535,6 @@ static int pnv_eeh_set_option(struct eeh_pe *pe, int option) > > return 0; > > } > > > > -/** > > - * pnv_eeh_get_pe_addr - Retrieve PE address > > - * @pe: EEH PE > > - * > > - * Retrieve the PE address according to the given tranditional > > - * PCI BDF (Bus/Device/Function) address. > > - */ > > -static int pnv_eeh_get_pe_addr(struct eeh_pe *pe) > > -{ > > - return pe->addr; > > -} > > - > > static void pnv_eeh_get_phb_diag(struct eeh_pe *pe) > > { > > struct pnv_phb *phb = pe->phb->private_data; > > @@ -1670,7 +1658,6 @@ static struct eeh_ops pnv_eeh_ops = { > > .init = pnv_eeh_init, > > .probe = pnv_eeh_probe, > > .set_option = pnv_eeh_set_option, > > - .get_pe_addr = pnv_eeh_get_pe_addr, > > .get_state = pnv_eeh_get_state, > > .reset = pnv_eeh_reset, > > .get_log = pnv_eeh_get_log, > > diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c > > index 18a2522b9b5e..088771fa38be 100644 > > --- a/arch/powerpc/platforms/pseries/eeh_pseries.c > > +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c > > @@ -32,6 +32,8 @@ > > #include <asm/ppc-pci.h> > > #include <asm/rtas.h> > > > > +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn); > > + > > /* RTAS tokens */ > > static int ibm_set_eeh_option; > > static int ibm_set_slot_reset; > > @@ -301,7 +303,7 @@ void pseries_eeh_init_edev(struct pci_dn *pdn) > > eeh_edev_dbg(edev, "EEH failed to enable on device (code %d)\n", ret); > > } else { > > /* Retrieve PE address */ > > - edev->pe_config_addr = eeh_ops->get_pe_addr(&pe); > > + edev->pe_config_addr = pseries_eeh_get_pe_addr(pdn); > > pe.addr = edev->pe_config_addr; > > > > /* Some older systems (Power4) allow the ibm,set-eeh-option > > @@ -431,8 +433,10 @@ static int pseries_eeh_set_option(struct eeh_pe *pe, int option) > > * It's notable that zero'ed return value means invalid PE config > > * address. > > */ > > -static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > > +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn) > > { > > + int config_addr = rtas_config_addr(pdn->busno, pdn->devfn, 0); > > Why not use pe->config_addr I wanted to get rid of the PE argument. The only caller (pseries_eeh_init_edev()) doesn't even pass a real PE, just the "fake" PE which only has one initialised field which is... sketch IMO. The other reason is for Wen's post-kdump pseries PHB reset patch. In that situation we want the reset to be done before we've done any PCI setup so there won't be any eeh_pe structures available. We will however have pci_dn's since they're set up before we start scanning PHBs. I also think some of the "fake pe" stuff in pseries_eeh_init_edev() is broken so the fewer users of that we have the better. > (and why we have two addresses in eeh_pe anyway)? I don't know :( It's one of those things I've been meaning to look at but haven't found the will to jump down that particular rabbit hole. I did take a cursory look and there's some comments about pe->addr being zero in some cases so EEH falls back to matching on pe->config_addr when searching for a PE. IIRC when I looked I couldn't work out why pe->config_addr would ever be zero. On PowerNV zero is a valid PE address and we set the EEH_VALID_PE_ZERO flag to disable that fallback logic so the reason is probably some weird pseries thing. > Ah, I guess I just trust you with this one :) > > > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > > > + int buid = pdn->phb->buid; > > int ret = 0; > > int rets[3]; > > > > @@ -443,18 +447,16 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > > * meaningless. > > */ > > ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, > > - pe->config_addr, BUID_HI(pe->phb->buid), > > - BUID_LO(pe->phb->buid), 1); > > + config_addr, BUID_HI(buid), BUID_LO(buid), 1); > > if (ret || (rets[0] == 0)) > > return 0; > > > > /* Retrieve the associated PE config address */ > > ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, > > - pe->config_addr, BUID_HI(pe->phb->buid), > > - BUID_LO(pe->phb->buid), 0); > > + config_addr, BUID_HI(buid), BUID_LO(buid), 0); > > if (ret) { > > pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", > > - __func__, pe->phb->global_number, pe->config_addr); > > + __func__, pdn->phb->global_number, config_addr); > > return 0; > > } > > > > @@ -463,11 +465,10 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) > > > > if (ibm_get_config_addr_info != RTAS_UNKNOWN_SERVICE) { > > ret = rtas_call(ibm_get_config_addr_info, 4, 2, rets, > > - pe->config_addr, BUID_HI(pe->phb->buid), > > - BUID_LO(pe->phb->buid), 0); > > + config_addr, BUID_HI(buid), BUID_LO(buid), 0); > > if (ret) { > > pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", > > - __func__, pe->phb->global_number, pe->config_addr); > > + __func__, pdn->phb->global_number, config_addr); > > return 0; > > } > > > > @@ -839,7 +840,6 @@ static struct eeh_ops pseries_eeh_ops = { > > .init = pseries_eeh_init, > > .probe = pseries_eeh_probe, > > .set_option = pseries_eeh_set_option, > > - .get_pe_addr = pseries_eeh_get_pe_addr, > > .get_state = pseries_eeh_get_state, > > .reset = pseries_eeh_reset, > > .get_log = pseries_eeh_get_log, > > > > -- > Alexey
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h index 3d648e042835..1bddc0dfe099 100644 --- a/arch/powerpc/include/asm/eeh.h +++ b/arch/powerpc/include/asm/eeh.h @@ -220,7 +220,6 @@ struct eeh_ops { int (*init)(void); struct eeh_dev *(*probe)(struct pci_dev *pdev); int (*set_option)(struct eeh_pe *pe, int option); - int (*get_pe_addr)(struct eeh_pe *pe); int (*get_state)(struct eeh_pe *pe, int *delay); int (*reset)(struct eeh_pe *pe, int option); int (*get_log)(struct eeh_pe *pe, int severity, char *drv_log, unsigned long len); diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index 79409e005fcd..bcd0515d8f79 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -535,18 +535,6 @@ static int pnv_eeh_set_option(struct eeh_pe *pe, int option) return 0; } -/** - * pnv_eeh_get_pe_addr - Retrieve PE address - * @pe: EEH PE - * - * Retrieve the PE address according to the given tranditional - * PCI BDF (Bus/Device/Function) address. - */ -static int pnv_eeh_get_pe_addr(struct eeh_pe *pe) -{ - return pe->addr; -} - static void pnv_eeh_get_phb_diag(struct eeh_pe *pe) { struct pnv_phb *phb = pe->phb->private_data; @@ -1670,7 +1658,6 @@ static struct eeh_ops pnv_eeh_ops = { .init = pnv_eeh_init, .probe = pnv_eeh_probe, .set_option = pnv_eeh_set_option, - .get_pe_addr = pnv_eeh_get_pe_addr, .get_state = pnv_eeh_get_state, .reset = pnv_eeh_reset, .get_log = pnv_eeh_get_log, diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c index 18a2522b9b5e..088771fa38be 100644 --- a/arch/powerpc/platforms/pseries/eeh_pseries.c +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c @@ -32,6 +32,8 @@ #include <asm/ppc-pci.h> #include <asm/rtas.h> +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn); + /* RTAS tokens */ static int ibm_set_eeh_option; static int ibm_set_slot_reset; @@ -301,7 +303,7 @@ void pseries_eeh_init_edev(struct pci_dn *pdn) eeh_edev_dbg(edev, "EEH failed to enable on device (code %d)\n", ret); } else { /* Retrieve PE address */ - edev->pe_config_addr = eeh_ops->get_pe_addr(&pe); + edev->pe_config_addr = pseries_eeh_get_pe_addr(pdn); pe.addr = edev->pe_config_addr; /* Some older systems (Power4) allow the ibm,set-eeh-option @@ -431,8 +433,10 @@ static int pseries_eeh_set_option(struct eeh_pe *pe, int option) * It's notable that zero'ed return value means invalid PE config * address. */ -static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) +static int pseries_eeh_get_pe_addr(struct pci_dn *pdn) { + int config_addr = rtas_config_addr(pdn->busno, pdn->devfn, 0); + int buid = pdn->phb->buid; int ret = 0; int rets[3]; @@ -443,18 +447,16 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) * meaningless. */ ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, - pe->config_addr, BUID_HI(pe->phb->buid), - BUID_LO(pe->phb->buid), 1); + config_addr, BUID_HI(buid), BUID_LO(buid), 1); if (ret || (rets[0] == 0)) return 0; /* Retrieve the associated PE config address */ ret = rtas_call(ibm_get_config_addr_info2, 4, 2, rets, - pe->config_addr, BUID_HI(pe->phb->buid), - BUID_LO(pe->phb->buid), 0); + config_addr, BUID_HI(buid), BUID_LO(buid), 0); if (ret) { pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", - __func__, pe->phb->global_number, pe->config_addr); + __func__, pdn->phb->global_number, config_addr); return 0; } @@ -463,11 +465,10 @@ static int pseries_eeh_get_pe_addr(struct eeh_pe *pe) if (ibm_get_config_addr_info != RTAS_UNKNOWN_SERVICE) { ret = rtas_call(ibm_get_config_addr_info, 4, 2, rets, - pe->config_addr, BUID_HI(pe->phb->buid), - BUID_LO(pe->phb->buid), 0); + config_addr, BUID_HI(buid), BUID_LO(buid), 0); if (ret) { pr_warn("%s: Failed to get address for PHB#%x-PE#%x\n", - __func__, pe->phb->global_number, pe->config_addr); + __func__, pdn->phb->global_number, config_addr); return 0; } @@ -839,7 +840,6 @@ static struct eeh_ops pseries_eeh_ops = { .init = pseries_eeh_init, .probe = pseries_eeh_probe, .set_option = pseries_eeh_set_option, - .get_pe_addr = pseries_eeh_get_pe_addr, .get_state = pseries_eeh_get_state, .reset = pseries_eeh_reset, .get_log = pseries_eeh_get_log,
This is used in precisely one place which is in pseries specific platform code. There's no need to have the callback in eeh_ops since the platform chooses the EEH PE addresses anyway. The PowerNV implementation has always been a stub too so remove it. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- arch/powerpc/include/asm/eeh.h | 1 - arch/powerpc/platforms/powernv/eeh-powernv.c | 13 ------------ arch/powerpc/platforms/pseries/eeh_pseries.c | 22 ++++++++++---------- 3 files changed, 11 insertions(+), 25 deletions(-)