From: Gavin Shan <gwshan@linux.vnet.ibm.com> To: Wei Yang <weiyang@linux.vnet.ibm.com> Cc: gwshan@linux.vnet.ibm.com, bhelgaas@google.com, linuxppc-dev@lists.ozlabs.org, linux-pci@vger.kernel.org Subject: Re: [PATCH V3 5/9] powerpc/eeh: create EEH_PE_VF for VF PE Date: Mon, 11 May 2015 12:37:07 +1000 [thread overview] Message-ID: <20150511023707.GA8816@gwshan> (raw) In-Reply-To: <1430723258-21299-6-git-send-email-weiyang@linux.vnet.ibm.com> On Mon, May 04, 2015 at 03:07:34PM +0800, Wei Yang wrote: Please reorder PATCH[6] with this one because the EEH device is expected to be created before EEH PE. >On powernv platform, VF PE is a special PE which is different from the Bus >PE. On the EEH side, it needs a corresponding concept to handle the VF PE >properly. For example, we need to create VF PE when VF's pci_dev is >initialized in kernel. And add a flag to mark it is a VF PF. ^^^^^ > >From above commit log, my understanding is that you're adding a flag to identify VF PE, which is handled differently from bus PE. You missed the details on the difference between them and the speical treament to VF PE. Could you help add those information in the commit log to make it looks complete? >This patch introduces the EEH_PE_VF type for VF PE and creates it for a VF. >At the mean time, it creates the sysfs and address cache for VF PE. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it creates the sysfs and address cache for VF PE at PCI device final fixup time. > >Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >--- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/kernel/eeh_pe.c | 12 ++++++++++-- > arch/powerpc/platforms/powernv/eeh-powernv.c | 12 ++++++++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > >diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h >index a52db28..56e8cd9 100644 >--- a/arch/powerpc/include/asm/eeh.h >+++ b/arch/powerpc/include/asm/eeh.h >@@ -70,6 +70,7 @@ struct pci_dn; > #define EEH_PE_PHB (1 << 1) /* PHB PE */ > #define EEH_PE_DEVICE (1 << 2) /* Device PE */ > #define EEH_PE_BUS (1 << 3) /* Bus PE */ >+#define EEH_PE_VF (1 << 4) /* VF PE */ > > #define EEH_PE_ISOLATED (1 << 0) /* Isolated PE */ > #define EEH_PE_RECOVERING (1 << 1) /* Recovering PE */ >diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >index 35f0b62..edfe63a 100644 >--- a/arch/powerpc/kernel/eeh_pe.c >+++ b/arch/powerpc/kernel/eeh_pe.c >@@ -299,7 +299,12 @@ static struct eeh_pe *eeh_pe_get_parent(struct eeh_dev *edev) > * EEH device already having associated PE, but > * the direct parent EEH device doesn't have yet. > */ >- pdn = pdn ? pdn->parent : NULL; >+#ifdef CONFIG_PCI_IOV >+ if (edev->mode & EEH_DEV_VF) >+ pdn = pci_get_pdn(edev->physfn); >+ else >+#endif >+ pdn = pdn ? pdn->parent : NULL; [A] > while (pdn) { > /* We're poking out of PCI territory */ > parent = pdn_to_eeh_dev(pdn); >@@ -382,7 +387,10 @@ int eeh_add_to_parent_pe(struct eeh_dev *edev) > } > > /* Create a new EEH PE */ >- pe = eeh_pe_alloc(edev->phb, EEH_PE_DEVICE); >+ if (edev->mode & EEH_DEV_VF) >+ pe = eeh_pe_alloc(edev->phb, EEH_PE_VF); >+ else >+ pe = eeh_pe_alloc(edev->phb, EEH_PE_DEVICE); You don't have CONFIG_PCI_IOV here to protect the code, but you had that at [A]. In order to keep the code look consistent, you either add it or remove it for all places. I prefer to remove it, which we don't need CONFIG_PCI_IOV. > if (!pe) { > pr_err("%s: out of memory!\n", __func__); > return -ENOMEM; >diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c >index 622f08c..5447481 100644 >--- a/arch/powerpc/platforms/powernv/eeh-powernv.c >+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c >@@ -1540,3 +1540,15 @@ static int __init eeh_powernv_init(void) > return ret; > } > machine_early_initcall(powernv, eeh_powernv_init); >+ >+#ifdef CONFIG_PCI_IOV >+static void pnv_pci_fixup_vf_eeh(struct pci_dev *pdev) >+{ Please rename it to pnv_eeh_vf_final_fixup(). Names of all functions in this file expect prefix "pnv_eeh_". With "_final_", it's clearly to tell it's called on PCI device final fixup time. >+ /* sysfs files should only be added after devices are added */ It's nice to explain why here: sysfs for the PCI device isn't populated and the MMIO resource isn't finalized for the PCI device yet. >+ if (pdev->is_virtfn) { >+ eeh_add_device_late(pdev); >+ eeh_sysfs_add_device(pdev); >+ } >+} The nested ifdef can be avoided as: if (!pdev->is_virtfn) return; eeh_add_device_late(pdev); eeh_sysfs_add_device(pdev); >+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pnv_pci_fixup_vf_eeh); >+#endif /* CONFIG_PCI_IOV */ Thanks, Gavin
WARNING: multiple messages have this Message-ID (diff)
From: Gavin Shan <gwshan@linux.vnet.ibm.com> To: Wei Yang <weiyang@linux.vnet.ibm.com> Cc: bhelgaas@google.com, linux-pci@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, gwshan@linux.vnet.ibm.com Subject: Re: [PATCH V3 5/9] powerpc/eeh: create EEH_PE_VF for VF PE Date: Mon, 11 May 2015 12:37:07 +1000 [thread overview] Message-ID: <20150511023707.GA8816@gwshan> (raw) In-Reply-To: <1430723258-21299-6-git-send-email-weiyang@linux.vnet.ibm.com> On Mon, May 04, 2015 at 03:07:34PM +0800, Wei Yang wrote: Please reorder PATCH[6] with this one because the EEH device is expected to be created before EEH PE. >On powernv platform, VF PE is a special PE which is different from the Bus >PE. On the EEH side, it needs a corresponding concept to handle the VF PE >properly. For example, we need to create VF PE when VF's pci_dev is >initialized in kernel. And add a flag to mark it is a VF PF. ^^^^^ > >From above commit log, my understanding is that you're adding a flag to identify VF PE, which is handled differently from bus PE. You missed the details on the difference between them and the speical treament to VF PE. Could you help add those information in the commit log to make it looks complete? >This patch introduces the EEH_PE_VF type for VF PE and creates it for a VF. >At the mean time, it creates the sysfs and address cache for VF PE. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ it creates the sysfs and address cache for VF PE at PCI device final fixup time. > >Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com> >--- > arch/powerpc/include/asm/eeh.h | 1 + > arch/powerpc/kernel/eeh_pe.c | 12 ++++++++++-- > arch/powerpc/platforms/powernv/eeh-powernv.c | 12 ++++++++++++ > 3 files changed, 23 insertions(+), 2 deletions(-) > >diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h >index a52db28..56e8cd9 100644 >--- a/arch/powerpc/include/asm/eeh.h >+++ b/arch/powerpc/include/asm/eeh.h >@@ -70,6 +70,7 @@ struct pci_dn; > #define EEH_PE_PHB (1 << 1) /* PHB PE */ > #define EEH_PE_DEVICE (1 << 2) /* Device PE */ > #define EEH_PE_BUS (1 << 3) /* Bus PE */ >+#define EEH_PE_VF (1 << 4) /* VF PE */ > > #define EEH_PE_ISOLATED (1 << 0) /* Isolated PE */ > #define EEH_PE_RECOVERING (1 << 1) /* Recovering PE */ >diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c >index 35f0b62..edfe63a 100644 >--- a/arch/powerpc/kernel/eeh_pe.c >+++ b/arch/powerpc/kernel/eeh_pe.c >@@ -299,7 +299,12 @@ static struct eeh_pe *eeh_pe_get_parent(struct eeh_dev *edev) > * EEH device already having associated PE, but > * the direct parent EEH device doesn't have yet. > */ >- pdn = pdn ? pdn->parent : NULL; >+#ifdef CONFIG_PCI_IOV >+ if (edev->mode & EEH_DEV_VF) >+ pdn = pci_get_pdn(edev->physfn); >+ else >+#endif >+ pdn = pdn ? pdn->parent : NULL; [A] > while (pdn) { > /* We're poking out of PCI territory */ > parent = pdn_to_eeh_dev(pdn); >@@ -382,7 +387,10 @@ int eeh_add_to_parent_pe(struct eeh_dev *edev) > } > > /* Create a new EEH PE */ >- pe = eeh_pe_alloc(edev->phb, EEH_PE_DEVICE); >+ if (edev->mode & EEH_DEV_VF) >+ pe = eeh_pe_alloc(edev->phb, EEH_PE_VF); >+ else >+ pe = eeh_pe_alloc(edev->phb, EEH_PE_DEVICE); You don't have CONFIG_PCI_IOV here to protect the code, but you had that at [A]. In order to keep the code look consistent, you either add it or remove it for all places. I prefer to remove it, which we don't need CONFIG_PCI_IOV. > if (!pe) { > pr_err("%s: out of memory!\n", __func__); > return -ENOMEM; >diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c >index 622f08c..5447481 100644 >--- a/arch/powerpc/platforms/powernv/eeh-powernv.c >+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c >@@ -1540,3 +1540,15 @@ static int __init eeh_powernv_init(void) > return ret; > } > machine_early_initcall(powernv, eeh_powernv_init); >+ >+#ifdef CONFIG_PCI_IOV >+static void pnv_pci_fixup_vf_eeh(struct pci_dev *pdev) >+{ Please rename it to pnv_eeh_vf_final_fixup(). Names of all functions in this file expect prefix "pnv_eeh_". With "_final_", it's clearly to tell it's called on PCI device final fixup time. >+ /* sysfs files should only be added after devices are added */ It's nice to explain why here: sysfs for the PCI device isn't populated and the MMIO resource isn't finalized for the PCI device yet. >+ if (pdev->is_virtfn) { >+ eeh_add_device_late(pdev); >+ eeh_sysfs_add_device(pdev); >+ } >+} The nested ifdef can be avoided as: if (!pdev->is_virtfn) return; eeh_add_device_late(pdev); eeh_sysfs_add_device(pdev); >+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pnv_pci_fixup_vf_eeh); >+#endif /* CONFIG_PCI_IOV */ Thanks, Gavin
next prev parent reply other threads:[~2015-05-11 2:38 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-05-04 7:07 [PATCH V3 0/9] VF EEH on Power8 Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-04 7:07 ` [PATCH V3 1/9] pci/iov: rename and export virtfn_add/virtfn_remove Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 2:13 ` Gavin Shan 2015-05-11 2:13 ` Gavin Shan 2015-05-04 7:07 ` [PATCH V3 2/9] powerpc/pci_dn: cache vf_index in pci_dn Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 2:21 ` Gavin Shan 2015-05-11 2:21 ` Gavin Shan 2015-05-11 5:54 ` Wei Yang 2015-05-11 5:54 ` Wei Yang 2015-05-12 6:15 ` Gavin Shan 2015-05-12 6:15 ` Gavin Shan 2015-05-12 7:29 ` Wei Yang 2015-05-12 7:29 ` Wei Yang 2015-05-04 7:07 ` [PATCH V3 3/9] powerpc/pci: remove PCI devices in reverse order Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-04 7:07 ` [PATCH V3 4/9] powerpc/eeh: cache address range just for normal device Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-04 7:07 ` [PATCH V3 5/9] powerpc/eeh: create EEH_PE_VF for VF PE Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 2:37 ` Gavin Shan [this message] 2015-05-11 2:37 ` Gavin Shan 2015-05-11 6:25 ` Wei Yang 2015-05-11 6:25 ` Wei Yang 2015-05-12 6:28 ` Gavin Shan 2015-05-12 6:28 ` Gavin Shan 2015-05-12 7:52 ` Wei Yang 2015-05-12 7:52 ` Wei Yang 2015-05-04 7:07 ` [PATCH V3 6/9] powerpc/powernv: create/release eeh_dev for VF Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 2:48 ` Gavin Shan 2015-05-11 2:48 ` Gavin Shan 2015-05-12 8:06 ` Wei Yang 2015-05-12 8:06 ` Wei Yang 2015-05-12 23:09 ` Gavin Shan 2015-05-12 23:09 ` Gavin Shan 2015-05-04 7:07 ` [PATCH V3 7/9] powerpc/powernv: Support EEH reset for VFs Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 3:03 ` Gavin Shan 2015-05-11 3:03 ` Gavin Shan 2015-05-04 7:07 ` [PATCH V3 8/9] powerpc/powernv: Support PCI config restore " Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-11 4:22 ` Gavin Shan 2015-05-11 4:22 ` Gavin Shan 2015-05-12 1:31 ` Wei Yang 2015-05-12 1:31 ` Wei Yang 2015-05-12 6:34 ` Gavin Shan 2015-05-12 6:34 ` Gavin Shan 2015-05-12 8:16 ` Wei Yang 2015-05-12 8:16 ` Wei Yang 2015-05-12 23:16 ` Gavin Shan 2015-05-12 23:16 ` Gavin Shan 2015-05-04 7:07 ` [PATCH V3 9/9] powerpc/eeh: handle VF PE properly Wei Yang 2015-05-04 7:07 ` Wei Yang 2015-05-13 1:16 ` Gavin Shan 2015-05-13 1:16 ` Gavin Shan 2015-05-14 9:35 ` Wei Yang 2015-05-14 9:35 ` Wei Yang 2015-05-14 12:15 ` Gavin Shan 2015-05-14 12:15 ` Gavin Shan 2015-05-14 10:02 ` Wei Yang 2015-05-14 10:02 ` Wei Yang 2015-05-14 12:30 ` Gavin Shan 2015-05-14 12:30 ` Gavin Shan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150511023707.GA8816@gwshan \ --to=gwshan@linux.vnet.ibm.com \ --cc=bhelgaas@google.com \ --cc=linux-pci@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=weiyang@linux.vnet.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.