diff mbox

[V3,5/9] powerpc/eeh: create EEH_PE_VF for VF PE

Message ID 1430723258-21299-6-git-send-email-weiyang@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Yang May 4, 2015, 7:07 a.m. UTC
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.

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.

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(-)

Comments

Gavin Shan May 11, 2015, 2:37 a.m. UTC | #1
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

--
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
Wei Yang May 11, 2015, 6:25 a.m. UTC | #2
On Mon, May 11, 2015 at 12:37:07PM +1000, Gavin Shan wrote:
>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.

That's a good idea.

>
>>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 just introduce the VF PE. For those differences, we have another
patch "handle VF PE properly" to cover. In the log of that patch, I listed
those differences. Do you think this is fine?

>>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.
>

Ok, that's fine to remove it.

BTW, if remove the CONFIG_PCI_IOV, we need to remove it around the physfn in
eeh_dev definition. That's fine?

>> 	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.
>

ok

>>+	/* 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.
>

Don't get your point.

sysfs of the PCI device is populated at this point.

>>+	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);
>

Ok.

>>+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pnv_pci_fixup_vf_eeh);
>>+#endif /* CONFIG_PCI_IOV */
>
>Thanks,
>Gavin
Gavin Shan May 12, 2015, 6:28 a.m. UTC | #3
On Mon, May 11, 2015 at 02:25:49PM +0800, Wei Yang wrote:
>On Mon, May 11, 2015 at 12:37:07PM +1000, Gavin Shan wrote:
>>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.
>
>That's a good idea.
>
>>
>>>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 just introduce the VF PE. For those differences, we have another
>patch "handle VF PE properly" to cover. In the log of that patch, I listed
>those differences. Do you think this is fine?
>

It's fine to me.

>>>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.
>>
>
>Ok, that's fine to remove it.
>
>BTW, if remove the CONFIG_PCI_IOV, we need to remove it around the physfn in
>eeh_dev definition. That's fine?
>

It's fine to me.

>>> 	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.
>>
>
>ok
>
>>>+	/* 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.
>>
>
>Don't get your point.
>
>sysfs of the PCI device is populated at this point.
>

You have two operations here: (A) add the PCI device to EEH address cache;
(B) add EEH related sysfs entries. (A) requires that the resources (MMIO
on PHB3) of the VF is finalized. (B) requires VF's sysfs entries have been
populated. So you need two conditions here to make sure (A) and (B) work
correctly: sysfs files are created and MMIO resources are populated. I was
saying your comments is a bit confusing. Could you have something like this:

       /*
        * The following operations will fail if VF's sysfs files aren't
        * created or its resources aren't finalized.
        */ 

Also, the PCI device's sysfs files aren't created at this point (final
fixup time). Note that, PCI final fixup is invoked separately from PCI
enumeration by fs_initcall_sync().

>>>+	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);
>>
>
>Ok.
>
>>>+DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, pnv_pci_fixup_vf_eeh);
>>>+#endif /* CONFIG_PCI_IOV */

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
Wei Yang May 12, 2015, 7:52 a.m. UTC | #4
On Tue, May 12, 2015 at 04:28:23PM +1000, Gavin Shan wrote:
>On Mon, May 11, 2015 at 02:25:49PM +0800, Wei Yang wrote:
>>On Mon, May 11, 2015 at 12:37:07PM +1000, Gavin Shan wrote:
>>>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.
>>
>>That's a good idea.
>>
>>>
>>>>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 just introduce the VF PE. For those differences, we have another
>>patch "handle VF PE properly" to cover. In the log of that patch, I listed
>>those differences. Do you think this is fine?
>>
>
>It's fine to me.
>
>>>>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.
>>>
>>
>>Ok, that's fine to remove it.
>>
>>BTW, if remove the CONFIG_PCI_IOV, we need to remove it around the physfn in
>>eeh_dev definition. That's fine?
>>
>
>It's fine to me.
>
>>>> 	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.
>>>
>>
>>ok
>>
>>>>+	/* 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.
>>>
>>
>>Don't get your point.
>>
>>sysfs of the PCI device is populated at this point.
>>
>
>You have two operations here: (A) add the PCI device to EEH address cache;
>(B) add EEH related sysfs entries. (A) requires that the resources (MMIO
>on PHB3) of the VF is finalized. (B) requires VF's sysfs entries have been
>populated. So you need two conditions here to make sure (A) and (B) work
>correctly: sysfs files are created and MMIO resources are populated. I was
>saying your comments is a bit confusing. Could you have something like this:
>
>       /*
>        * The following operations will fail if VF's sysfs files aren't
>        * created or its resources aren't finalized.
>        */ 
>

Will change the comment with this.

--
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
diff mbox

Patch

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;
 	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);
 	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)
+{
+	/* sysfs files should only be added after devices are added */
+	if (pdev->is_virtfn) {
+		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 */