| Message ID | 1402365399-5121-8-git-send-email-weiyang@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 23, 2014 at 03:27:21PM +1000, Gavin Shan wrote: >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. > Hmm... the deconfiguration function is the counterpart of the configuration function, so it will release the resource which are allocated in configuration. The DMA, IO/MMIO, MSI is not assigned in the configuration function, so it would not proper to release those resources by 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 :-) Ok. > >>+ 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. > Ok, so there is a bug in the original code, will fix this. >>+ /* 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"). > Sounds correct, this is missed. >>+ >>+ /* 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
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) +{ + struct pci_dev *parent; + uint8_t bcomp, dcomp, fcomp; + int64_t rc; + long rid_end, rid; + 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); + /* Do an exact match only */ + bcomp = OpalPciBusAll; + } + rid_end = pe->rid + (count << 8); + }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; + } + + /* 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;
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(+)