Re: [PATCH 04/11] powerpc/powernv/ioda: Release opencapi device

[Frederic Barrat]

Le 10/09/2019 à 02:56, Alastair D'Silva a écrit :
On Mon, 2019-09-09 at 17:45 +0200, Frederic Barrat wrote:
With hotplug, an opencapi device can now go away. It needs to be
released, mostly to clean up its PE state. We were previously not
defining any device callback. We can reuse the standard PCI release
callback, it does a bit too much for an opencapi device, but it's
harmless, and only needs minor tuning.

Also separate the undo of the PELT-V code in a separate function, it
is not needed for NPU devices and it improves a bit the readability
of
the code.

Signed-off-by: Frederic Barrat <fbar...@linux.ibm.com>
---
  arch/powerpc/platforms/powernv/pci-ioda.c | 59 +++++++++++++++----
----
  1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
b/arch/powerpc/platforms/powernv/pci-ioda.c
index 06ce7ddaa0cf..e5895c05efae 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -188,7 +188,7 @@ static void pnv_ioda_free_pe(struct pnv_ioda_pe
*pe)
        unsigned int pe_num = pe->pe_number;
  
  	WARN_ON(pe->pdev);
-       WARN_ON(pe->npucomp); /* NPUs are not supposed to be freed */
+       WARN_ON(pe->npucomp); /* NPUs for nvlink are not supposed to be
freed */
        kfree(pe->npucomp);
        memset(pe, 0, sizeof(struct pnv_ioda_pe));
        clear_bit(pe_num, phb->ioda.pe_alloc);
@@ -777,6 +777,34 @@ static int pnv_ioda_set_peltv(struct pnv_phb
*phb,
        return 0;
  }
  
+static void pnv_ioda_unset_peltv(struct pnv_phb *phb,
+                                struct pnv_ioda_pe *pe,
+                                struct pci_dev *parent)
+{
+       int64_t rc;
+
+       while (parent) {
+               struct pci_dn *pdn = pci_get_pdn(parent);
+
+               if (pdn && pdn->pe_number != IODA_INVALID_PE) {
+                       rc = opal_pci_set_peltv(phb->opal_id, pdn-
pe_number,
+                                               pe->pe_number,
+                                               OPAL_REMOVE_PE_FROM_DOM
AIN);
+                       /* XXX What to do in case of error ? */

Can we take the opportunity to address this comment?


Probably like the original author, I'm not sure what we could do better 
here. We already log a warning below if opal returns an error when 
dissociating the PE (admittedly not the same thing as the parent). Note 
that this code is not executed for an opencapi device, so I'm just 
keeping thing as is.

 Fred



+               }
+               parent = parent->bus->self;
+       }
+
+       opal_pci_eeh_freeze_clear(phb->opal_id, pe->pe_number,
+                                 OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
+
+       /* Disassociate PE in PELT */
+       rc = opal_pci_set_peltv(phb->opal_id, pe->pe_number,
+                               pe->pe_number,
OPAL_REMOVE_PE_FROM_DOMAIN);
+       if (rc)
+               pe_warn(pe, "OPAL error %lld remove self from PELTV\n",
rc);
+}
+
  static int pnv_ioda_deconfigure_pe(struct pnv_phb *phb, struct
pnv_ioda_pe *pe)
  {
        struct pci_dev *parent;
@@ -827,25 +855,13 @@ static int pnv_ioda_deconfigure_pe(struct
pnv_phb *phb, struct pnv_ioda_pe *pe)
        for (rid = pe->rid; rid < rid_end; rid++)
                phb->ioda.pe_rmap[rid] = IODA_INVALID_PE;
  
-	/* Release from all parents PELT-V */
-       while (parent) {
-               struct pci_dn *pdn = pci_get_pdn(parent);
-               if (pdn && pdn->pe_number != IODA_INVALID_PE) {
-                       rc = opal_pci_set_peltv(phb->opal_id, pdn-
pe_number,
-                                               pe->pe_number,
OPAL_REMOVE_PE_FROM_DOMAIN);
-                       /* XXX What to do in case of error ? */
-               }
-               parent = parent->bus->self;
-       }
-
-       opal_pci_eeh_freeze_clear(phb->opal_id, pe->pe_number,
-                                 OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
+       /*
+        * Release from all parents PELT-V. NPUs don't have a PELTV
+        * table
+        */
+       if (phb->type != PNV_PHB_NPU_NVLINK && phb->type !=
PNV_PHB_NPU_OCAPI)
+               pnv_ioda_unset_peltv(phb, pe, parent);
  
-	/* Disassociate PE in PELT */
-       rc = opal_pci_set_peltv(phb->opal_id, pe->pe_number,
-                               pe->pe_number,
OPAL_REMOVE_PE_FROM_DOMAIN);
-       if (rc)
-               pe_warn(pe, "OPAL error %lld remove self from PELTV\n",
rc);
        rc = opal_pci_set_pe(phb->opal_id, pe->pe_number, pe->rid,
                             bcomp, dcomp, fcomp, OPAL_UNMAP_PE);
        if (rc)
@@ -3540,6 +3556,8 @@ static void pnv_ioda_release_pe(struct
pnv_ioda_pe *pe)
        case PNV_PHB_IODA2:
                pnv_pci_ioda2_release_pe_dma(pe);
                break;
+       case PNV_PHB_NPU_OCAPI:
+               break;
        default:
                WARN_ON(1);
        }
@@ -3592,7 +3610,7 @@ static void pnv_pci_release_device(struct
pci_dev *pdev)
        pe = &phb->ioda.pe_array[pdn->pe_number];
        pdn->pe_number = IODA_INVALID_PE;
  
-	WARN_ON(--pe->device_count < 0);
+       WARN_ON((pe->flags != PNV_IODA_PE_DEV) && (--pe->device_count <
0));
        if (pe->device_count == 0)
                pnv_ioda_release_pe(pe);
  }
@@ -3641,6 +3659,7 @@ static const struct pci_controller_ops
pnv_npu_ioda_controller_ops = {
  
  static const struct pci_controller_ops
pnv_npu_ocapi_ioda_controller_ops = {
        .enable_device_hook     = pnv_ocapi_enable_device_hook,
+       .release_device         = pnv_pci_release_device,
        .window_alignment       = pnv_pci_window_alignment,
        .reset_secondary_bus    = pnv_pci_reset_secondary_bus,
        .shutdown               = pnv_pci_ioda_shutdown,