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@xxxxxxxxxxxxxxxxxx>
>>---
>> 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
--
Richard Yang
Help you, Help me
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
