Re: [RFC PATCH V3 07/17] ppc/pnv: Add function to deconfig a PE

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Tue, Jun 10, 2014 at 09:56:29AM +0800, Wei Yang wrote:
>On PowerNV platform, it will support dynamic PE allocation and deallocation.
>
>This patch adds a function to release those resources related to a PE.
>
>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>---
> arch/powerpc/platforms/powernv/pci-ioda.c |   77 +++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>index 8ca3926..87cb3089 100644
>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>@@ -330,6 +330,83 @@ static struct pnv_ioda_pe *pnv_ioda_get_pe(struct pci_dev *dev)
> }
> #endif /* CONFIG_PCI_MSI */
>
>+static int pnv_ioda_deconfigure_pe(struct pnv_phb *phb, struct pnv_ioda_pe *pe)
>+{

Richard, it seems that the deconfiguration is incomplete. Something seems
missed: DMA, IO and MMIO, MSI. If I understand correctly, pnv_ioda_deconfigure_pe()
won't tear down DMA, IO and MMIO, MSI properly. For MSI/MSIx, it wouldn't
be a problem as the VF driver should disable them before calling this function.

>+	struct pci_dev *parent;
>+	uint8_t bcomp, dcomp, fcomp;
>+	int64_t rc;
>+	long rid_end, rid;

Blank line needed here to separate variable declaration and logic. And I think
we won't run into case "if (pe->pbus)" for now. So it's worthy to have some
comments to explain it for a bit :-)

>+	if (pe->pbus) {
>+		int count;
>+
>+		dcomp = OPAL_IGNORE_RID_DEVICE_NUMBER;
>+		fcomp = OPAL_IGNORE_RID_FUNCTION_NUMBER;
>+		parent = pe->pbus->self;
>+		if (pe->flags & PNV_IODA_PE_BUS_ALL)
>+			count = pe->pbus->busn_res.end - pe->pbus->busn_res.start + 1;
>+		else
>+			count = 1;
>+
>+		switch(count) {
>+		case  1: bcomp = OpalPciBusAll;         break;
>+		case  2: bcomp = OpalPciBus7Bits;       break;
>+		case  4: bcomp = OpalPciBus6Bits;       break;
>+		case  8: bcomp = OpalPciBus5Bits;       break;
>+		case 16: bcomp = OpalPciBus4Bits;       break;
>+		case 32: bcomp = OpalPciBus3Bits;       break;
>+		default:
>+			pr_err("%s: Number of subordinate busses %d"
>+			       " unsupported\n",
>+			       pci_name(pe->pbus->self), count);

I guess it's not safe to do "pci_name(pe->pbus->self)" root root bus.

>+			/* Do an exact match only */
>+			bcomp = OpalPciBusAll;
>+		}
>+		rid_end = pe->rid + (count << 8);
>+	}else {

	} else {

>+		parent = pe->pdev->bus->self;
>+		bcomp = OpalPciBusAll;
>+		dcomp = OPAL_COMPARE_RID_DEVICE_NUMBER;
>+		fcomp = OPAL_COMPARE_RID_FUNCTION_NUMBER;
>+		rid_end = pe->rid + 1;
>+	}
>+
>+	/* Disable MVT on IODA1 */
>+	if (phb->type == PNV_PHB_IODA1) {
>+		rc = opal_pci_set_mve_enable(phb->opal_id,
>+					     pe->mve_number, OPAL_DISABLE_MVE);
>+		if (rc) {
>+			pe_err(pe, "OPAL error %ld enabling MVE %d\n",
>+			       rc, pe->mve_number);
>+			pe->mve_number = -1;
>+		}
>+	}
>+	/* Clear the reverse map */
>+	for (rid = pe->rid; rid < rid_end; rid++)
>+		phb->ioda.pe_rmap[rid] = 0;
>+
>+	/* 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;
>+	}

It seems that you missed removing the PE from its own PELTV, which was
introduced by commit 631ad69 ("powerpc/powernv: Add PE to its own PELTV").

>+
>+	/* Dissociate PE in PELT */
>+	rc = opal_pci_set_pe(phb->opal_id, pe->pe_number, pe->rid,
>+			     bcomp, dcomp, fcomp, OPAL_UNMAP_PE);
>+	if (rc)
>+		pe_err(pe, "OPAL error %ld trying to setup PELT table\n", rc);
>+
>+	pe->pbus = NULL;
>+	pe->pdev = NULL;
>+
>+	return 0;
>+}
>+
> static int pnv_ioda_configure_pe(struct pnv_phb *phb, struct pnv_ioda_pe *pe)
> {
> 	struct pci_dev *parent;

Thanks,
Gavin