Re: [PATCH 14/14] powerpc/eeh: Move PE tree setup into the platform
On 14/07/2020 13:08, Oliver O'Halloran wrote:
> On Tue, Jul 14, 2020 at 11:50 AM Alexey Kardashevskiy wrote:
>>
>> On 06/07/2020 11:36, Oliver O'Halloran wrote:
>>> /**
>>> * eeh_pe_tree_insert - Add EEH device to parent PE
>>> * @edev: EEH device
>>> + * @new_pe_parent: PE to create additional PEs under
>>> *
>>> - * Add EEH device to the parent PE. If the parent PE already
>>> - * exists, the PE type will be changed to EEH_PE_BUS. Otherwise,
>>> - * we have to create new PE to hold the EEH device and the new
>>> - * PE will be linked to its parent PE as well.
>>> + * Add EEH device to the PE in edev->pe_config_addr. If a PE already
>>> + * exists with that address then @edev is added to that PE. Otherwise
>>> + * a new PE is created and inserted into the PE tree as a child of
>>> + * @new_pe_parent.
>>> + *
>>> + * If @new_pe_parent is NULL then the new PE will be inserted under
>>> + * directly under the the PHB.
>>> */
>>> -int eeh_pe_tree_insert(struct eeh_dev *edev)
>>> +int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent)
>>> {
>>> struct pci_controller *hose = edev->controller;
>>> struct eeh_pe *pe, *parent;
>>
>>
>> We can ditch this "parent" in that single place now and use "pe"
>> instead. And name the new parameter simply "parent". Dunno if it
>> improves things though.
>
> I did this at some point and then decided not to. It added a bunch of
> noise to the diff and calling the parameter "parent" ended up being
> pretty unreadable. The parameter is "the parent of the PE that will be
> created to contain edev", or "parent of the parent PE". It's pretty
> unwieldy.
Ok fine but we still do not need both pe and parent in that function
(may be one day...).
>
>>> @@ -399,7 +366,7 @@ int eeh_pe_tree_insert(struct eeh_dev *edev)
>>> }
>>>
>>> eeh_edev_dbg(edev,
>>> - "Added to device PE (parent: PE#%x)\n",
>>> + "Added to existing PE (parent: PE#%x)\n",
>>>pe->parent->addr);
>>> } else {
>>> /* Mark the PE as type of PCI bus */
>>> @@ -431,10 +398,9 @@ int eeh_pe_tree_insert(struct eeh_dev *edev)
>>>* to PHB directly. Otherwise, we have to associate the
>>>* PE with its parent.
>>>*/
>>> - parent = eeh_pe_get_parent(edev);
>>> - if (!parent) {
>>> - parent = eeh_phb_pe_get(hose);
>>> - if (!parent) {
>>> + if (!new_pe_parent) {
>>> + new_pe_parent = eeh_phb_pe_get(hose);
>>> + if (!new_pe_parent) {
>>
>>
>>
>> afaict only pseries can realisticly pass new_pe_parent==NULL so this
>> chunk could go to pseries_eeh_pe_get_parent.
>
> pnv_eeh_get_upstream_pe() will never return the PHB PE so
> new_pe_parent will be NULL for the first PE created under a PowerNV
> PHB. I guess we could move the PHB PE handling into the platform too,
> but I think that just results in having to special case PHB PEs in two
> places rather than one.
>
>>> +static struct eeh_pe *pseries_eeh_pe_get_parent(struct eeh_dev *edev)
>>> +{
>>> + struct eeh_dev *parent;
>>> + struct pci_dn *pdn = eeh_dev_to_pdn(edev);
>>> +
>>> + /*
>>> + * It might have the case for the indirect parent
>>> + * EEH device already having associated PE, but
>>> + * the direct parent EEH device doesn't have yet.
>>> + */
>>> + if (edev->physfn)
>>> + pdn = pci_get_pdn(edev->physfn);
>>> + else
>>> + pdn = pdn ? pdn->parent : NULL;
>>> + while (pdn) {
>>> + /* We're poking out of PCI territory */
>>
>>
>> We are traversing up PCI hierarchy here - pci_dn->parent, how is this
>> out of PCI territory? Or I understand "out of" incorrectly?
>>
>>
>>> + parent = pdn_to_eeh_dev(pdn);
>>> + if (!parent)
>>> + return NULL;
>
> If there's no eeh dev then the node we're looking at is a PHB rather
> than an actual PCI device so it stops looking. I think. The comment
> was copied over from the existing code and I haven't spent a whole lot
> of time parsing it's meaning.
I noticed cut-n-paste. May be just ditch it if nobody can parse it?
>
>
>
>>> @@ -301,6 +343,8 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
>>> if (ret) {
>>> eeh_edev_dbg(edev, "EEH failed to enable on device (code
>>> %d)\n", ret);
>>> } else {
>>> + struct eeh_pe *parent;
>>> +
>>> /* Retrieve PE address */
>>> edev->pe_config_addr = pseries_eeh_get_pe_addr(pdn);
>>> pe.addr = edev->pe_config_addr;
>>> @@ -313,16 +357,23 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
>>> if (ret > 0 && ret != EEH_STATE_NOT_SUPPORT)
>>> enable = 1;
>>>
>>> - if (enable) {
>>> + /* This device doesn't support EEH, but it may have a
Re: [PATCH 14/14] powerpc/eeh: Move PE tree setup into the platform
On Tue, Jul 14, 2020 at 11:50 AM Alexey Kardashevskiy wrote:
>
> On 06/07/2020 11:36, Oliver O'Halloran wrote:
> > /**
> > * eeh_pe_tree_insert - Add EEH device to parent PE
> > * @edev: EEH device
> > + * @new_pe_parent: PE to create additional PEs under
> > *
> > - * Add EEH device to the parent PE. If the parent PE already
> > - * exists, the PE type will be changed to EEH_PE_BUS. Otherwise,
> > - * we have to create new PE to hold the EEH device and the new
> > - * PE will be linked to its parent PE as well.
> > + * Add EEH device to the PE in edev->pe_config_addr. If a PE already
> > + * exists with that address then @edev is added to that PE. Otherwise
> > + * a new PE is created and inserted into the PE tree as a child of
> > + * @new_pe_parent.
> > + *
> > + * If @new_pe_parent is NULL then the new PE will be inserted under
> > + * directly under the the PHB.
> > */
> > -int eeh_pe_tree_insert(struct eeh_dev *edev)
> > +int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent)
> > {
> > struct pci_controller *hose = edev->controller;
> > struct eeh_pe *pe, *parent;
>
>
> We can ditch this "parent" in that single place now and use "pe"
> instead. And name the new parameter simply "parent". Dunno if it
> improves things though.
I did this at some point and then decided not to. It added a bunch of
noise to the diff and calling the parameter "parent" ended up being
pretty unreadable. The parameter is "the parent of the PE that will be
created to contain edev", or "parent of the parent PE". It's pretty
unwieldy.
> > @@ -399,7 +366,7 @@ int eeh_pe_tree_insert(struct eeh_dev *edev)
> > }
> >
> > eeh_edev_dbg(edev,
> > - "Added to device PE (parent: PE#%x)\n",
> > + "Added to existing PE (parent: PE#%x)\n",
> >pe->parent->addr);
> > } else {
> > /* Mark the PE as type of PCI bus */
> > @@ -431,10 +398,9 @@ int eeh_pe_tree_insert(struct eeh_dev *edev)
> >* to PHB directly. Otherwise, we have to associate the
> >* PE with its parent.
> >*/
> > - parent = eeh_pe_get_parent(edev);
> > - if (!parent) {
> > - parent = eeh_phb_pe_get(hose);
> > - if (!parent) {
> > + if (!new_pe_parent) {
> > + new_pe_parent = eeh_phb_pe_get(hose);
> > + if (!new_pe_parent) {
>
>
>
> afaict only pseries can realisticly pass new_pe_parent==NULL so this
> chunk could go to pseries_eeh_pe_get_parent.
pnv_eeh_get_upstream_pe() will never return the PHB PE so
new_pe_parent will be NULL for the first PE created under a PowerNV
PHB. I guess we could move the PHB PE handling into the platform too,
but I think that just results in having to special case PHB PEs in two
places rather than one.
> > +static struct eeh_pe *pseries_eeh_pe_get_parent(struct eeh_dev *edev)
> > +{
> > + struct eeh_dev *parent;
> > + struct pci_dn *pdn = eeh_dev_to_pdn(edev);
> > +
> > + /*
> > + * It might have the case for the indirect parent
> > + * EEH device already having associated PE, but
> > + * the direct parent EEH device doesn't have yet.
> > + */
> > + if (edev->physfn)
> > + pdn = pci_get_pdn(edev->physfn);
> > + else
> > + pdn = pdn ? pdn->parent : NULL;
> > + while (pdn) {
> > + /* We're poking out of PCI territory */
>
>
> We are traversing up PCI hierarchy here - pci_dn->parent, how is this
> out of PCI territory? Or I understand "out of" incorrectly?
>
>
> > + parent = pdn_to_eeh_dev(pdn);
> > + if (!parent)
> > + return NULL;
If there's no eeh dev then the node we're looking at is a PHB rather
than an actual PCI device so it stops looking. I think. The comment
was copied over from the existing code and I haven't spent a whole lot
of time parsing it's meaning.
> > @@ -301,6 +343,8 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
> > if (ret) {
> > eeh_edev_dbg(edev, "EEH failed to enable on device (code
> > %d)\n", ret);
> > } else {
> > + struct eeh_pe *parent;
> > +
> > /* Retrieve PE address */
> > edev->pe_config_addr = pseries_eeh_get_pe_addr(pdn);
> > pe.addr = edev->pe_config_addr;
> > @@ -313,16 +357,23 @@ void pseries_eeh_init_edev(struct pci_dn *pdn)
> > if (ret > 0 && ret != EEH_STATE_NOT_SUPPORT)
> > enable = 1;
> >
> > - if (enable) {
> > + /* This device doesn't support EEH, but it may have an
> > + * EEH parent, in which case we mark it as supported.
> > + */
> > + parent = pseries_eeh_pe_get_parent(edev);
> > + if (parent && !enable)
> > + edev->pe_config_addr = parent->addr;
>
>
> What if pse
Re: [PATCH 14/14] powerpc/eeh: Move PE tree setup into the platform
On 06/07/2020 11:36, Oliver O'Halloran wrote:
> The EEH core has a concept of a "PE tree" to support PowerNV. The PE tree
> follows the PCI bus structures because a reset asserted on an upstream
> bridge will be propagated to the downstream bridges. On pseries there's a
> 1-1 correspondence between what the guest sees are a PHB and a PE so the
> "tree" is really just a single node.
>
> Current the EEH core is reponsible for setting up this PE tree which it
> does by traversing the pci_dn tree. The structure of the pci_dn tree
> matches the bus tree on PowerNV which leads to the PE tree being "correct"
> this setup method doesn't make a whole lot of sense and it's actively
> confusing for the pseries case where it doesn't really do anything.
>
> We want to remove the dependence on pci_dn anyway so this patch move
> choosing where to insert a new PE into the platform code rather than
> being part of the generic EEH code. For PowerNV this simplifies the
> tree building logic and removes the use of pci_dn. For pseries we
> keep the existing logic. I'm not really convinced it does anything
> due to the 1-1 PE-to-PHB correspondence so every device under that
> PHB should be in the same PE, but I'd rather not remove it entirely
> until we've had a chance to look at it more deeply.
>
> Signed-off-by: Oliver O'Halloran
> ---
> arch/powerpc/include/asm/eeh.h | 2 +-
> arch/powerpc/kernel/eeh_pe.c | 70 ++--
> arch/powerpc/platforms/powernv/eeh-powernv.c | 27 +++-
> arch/powerpc/platforms/pseries/eeh_pseries.c | 59 +++--
> 4 files changed, 101 insertions(+), 57 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 8d34e5b790c2..1cab629dbc74 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -283,7 +283,7 @@ struct eeh_pe *eeh_phb_pe_get(struct pci_controller *phb);
> struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, struct eeh_pe *root);
> struct eeh_pe *eeh_pe_get(struct pci_controller *phb,
> int pe_no, int config_addr);
> -int eeh_pe_tree_insert(struct eeh_dev *edev);
> +int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent);
> int eeh_pe_tree_remove(struct eeh_dev *edev);
> void eeh_pe_update_time_stamp(struct eeh_pe *pe);
> void *eeh_pe_traverse(struct eeh_pe *root,
> diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c
> index 898205829a8f..ea2f8b362d18 100644
> --- a/arch/powerpc/kernel/eeh_pe.c
> +++ b/arch/powerpc/kernel/eeh_pe.c
> @@ -318,53 +318,20 @@ struct eeh_pe *eeh_pe_get(struct pci_controller *phb,
> return pe;
> }
>
> -/**
> - * eeh_pe_get_parent - Retrieve the parent PE
> - * @edev: EEH device
> - *
> - * The whole PEs existing in the system are organized as hierarchy
> - * tree. The function is used to retrieve the parent PE according
> - * to the parent EEH device.
> - */
> -static struct eeh_pe *eeh_pe_get_parent(struct eeh_dev *edev)
> -{
> - struct eeh_dev *parent;
> - struct pci_dn *pdn = eeh_dev_to_pdn(edev);
> -
> - /*
> - * It might have the case for the indirect parent
> - * EEH device already having associated PE, but
> - * the direct parent EEH device doesn't have yet.
> - */
> - if (edev->physfn)
> - pdn = pci_get_pdn(edev->physfn);
> - else
> - pdn = pdn ? pdn->parent : NULL;
> - while (pdn) {
> - /* We're poking out of PCI territory */
> - parent = pdn_to_eeh_dev(pdn);
> - if (!parent)
> - return NULL;
> -
> - if (parent->pe)
> - return parent->pe;
> -
> - pdn = pdn->parent;
> - }
> -
> - return NULL;
> -}
> -
> /**
> * eeh_pe_tree_insert - Add EEH device to parent PE
> * @edev: EEH device
> + * @new_pe_parent: PE to create additional PEs under
> *
> - * Add EEH device to the parent PE. If the parent PE already
> - * exists, the PE type will be changed to EEH_PE_BUS. Otherwise,
> - * we have to create new PE to hold the EEH device and the new
> - * PE will be linked to its parent PE as well.
> + * Add EEH device to the PE in edev->pe_config_addr. If a PE already
> + * exists with that address then @edev is added to that PE. Otherwise
> + * a new PE is created and inserted into the PE tree as a child of
> + * @new_pe_parent.
> + *
> + * If @new_pe_parent is NULL then the new PE will be inserted under
> + * directly under the the PHB.
> */
> -int eeh_pe_tree_insert(struct eeh_dev *edev)
> +int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent)
> {
> struct pci_controller *hose = edev->controller;
> struct eeh_pe *pe, *parent;
We can ditch this "parent" in that single place now and use "pe"
instead. And name the new parameter simply "parent". Dunno if it
improves things though.
> @@ -399,7 +366,7 @@ int eeh_pe_tree_insert(struct eeh_dev
[PATCH 14/14] powerpc/eeh: Move PE tree setup into the platform
The EEH core has a concept of a "PE tree" to support PowerNV. The PE tree
follows the PCI bus structures because a reset asserted on an upstream
bridge will be propagated to the downstream bridges. On pseries there's a
1-1 correspondence between what the guest sees are a PHB and a PE so the
"tree" is really just a single node.
Current the EEH core is reponsible for setting up this PE tree which it
does by traversing the pci_dn tree. The structure of the pci_dn tree
matches the bus tree on PowerNV which leads to the PE tree being "correct"
this setup method doesn't make a whole lot of sense and it's actively
confusing for the pseries case where it doesn't really do anything.
We want to remove the dependence on pci_dn anyway so this patch move
choosing where to insert a new PE into the platform code rather than
being part of the generic EEH code. For PowerNV this simplifies the
tree building logic and removes the use of pci_dn. For pseries we
keep the existing logic. I'm not really convinced it does anything
due to the 1-1 PE-to-PHB correspondence so every device under that
PHB should be in the same PE, but I'd rather not remove it entirely
until we've had a chance to look at it more deeply.
Signed-off-by: Oliver O'Halloran
---
arch/powerpc/include/asm/eeh.h | 2 +-
arch/powerpc/kernel/eeh_pe.c | 70 ++--
arch/powerpc/platforms/powernv/eeh-powernv.c | 27 +++-
arch/powerpc/platforms/pseries/eeh_pseries.c | 59 +++--
4 files changed, 101 insertions(+), 57 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 8d34e5b790c2..1cab629dbc74 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -283,7 +283,7 @@ struct eeh_pe *eeh_phb_pe_get(struct pci_controller *phb);
struct eeh_pe *eeh_pe_next(struct eeh_pe *pe, struct eeh_pe *root);
struct eeh_pe *eeh_pe_get(struct pci_controller *phb,
int pe_no, int config_addr);
-int eeh_pe_tree_insert(struct eeh_dev *edev);
+int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent);
int eeh_pe_tree_remove(struct eeh_dev *edev);
void eeh_pe_update_time_stamp(struct eeh_pe *pe);
void *eeh_pe_traverse(struct eeh_pe *root,
diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c
index 898205829a8f..ea2f8b362d18 100644
--- a/arch/powerpc/kernel/eeh_pe.c
+++ b/arch/powerpc/kernel/eeh_pe.c
@@ -318,53 +318,20 @@ struct eeh_pe *eeh_pe_get(struct pci_controller *phb,
return pe;
}
-/**
- * eeh_pe_get_parent - Retrieve the parent PE
- * @edev: EEH device
- *
- * The whole PEs existing in the system are organized as hierarchy
- * tree. The function is used to retrieve the parent PE according
- * to the parent EEH device.
- */
-static struct eeh_pe *eeh_pe_get_parent(struct eeh_dev *edev)
-{
- struct eeh_dev *parent;
- struct pci_dn *pdn = eeh_dev_to_pdn(edev);
-
- /*
-* It might have the case for the indirect parent
-* EEH device already having associated PE, but
-* the direct parent EEH device doesn't have yet.
-*/
- if (edev->physfn)
- pdn = pci_get_pdn(edev->physfn);
- else
- pdn = pdn ? pdn->parent : NULL;
- while (pdn) {
- /* We're poking out of PCI territory */
- parent = pdn_to_eeh_dev(pdn);
- if (!parent)
- return NULL;
-
- if (parent->pe)
- return parent->pe;
-
- pdn = pdn->parent;
- }
-
- return NULL;
-}
-
/**
* eeh_pe_tree_insert - Add EEH device to parent PE
* @edev: EEH device
+ * @new_pe_parent: PE to create additional PEs under
*
- * Add EEH device to the parent PE. If the parent PE already
- * exists, the PE type will be changed to EEH_PE_BUS. Otherwise,
- * we have to create new PE to hold the EEH device and the new
- * PE will be linked to its parent PE as well.
+ * Add EEH device to the PE in edev->pe_config_addr. If a PE already
+ * exists with that address then @edev is added to that PE. Otherwise
+ * a new PE is created and inserted into the PE tree as a child of
+ * @new_pe_parent.
+ *
+ * If @new_pe_parent is NULL then the new PE will be inserted under
+ * directly under the the PHB.
*/
-int eeh_pe_tree_insert(struct eeh_dev *edev)
+int eeh_pe_tree_insert(struct eeh_dev *edev, struct eeh_pe *new_pe_parent)
{
struct pci_controller *hose = edev->controller;
struct eeh_pe *pe, *parent;
@@ -399,7 +366,7 @@ int eeh_pe_tree_insert(struct eeh_dev *edev)
}
eeh_edev_dbg(edev,
-"Added to device PE (parent: PE#%x)\n",
+"Added to existing PE (parent: PE#%x)\n",
pe->parent->addr);
} else {
/* Mark the PE as type of PCI bus */
@@
