Re: [PATCH 05/14] powerpc/eeh: Kill off eeh_ops->get_pe_addr()
On Mon, Jul 13, 2020 at 7:54 PM Alexey Kardashevskiy 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
> > ---
> > 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
> > #include
> >
> > +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();
> > + 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
Re: [PATCH 05/14] powerpc/eeh: Kill off eeh_ops->get_pe_addr()
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
> ---
> 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
> #include
>
> +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();
> + 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
> + 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__,
[PATCH 05/14] powerpc/eeh: Kill off eeh_ops->get_pe_addr()
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
---
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
#include
+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();
+ 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 =
