Re: [PATCH] Bugfix: powerpc/eeh: Create eeh sysfs entry in post_init()

From: Gavin Shan <gwshan@linux.vnet.ibm.com>
To: Wei Yang <weiyang@linux.vnet.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org,
	Mike Qiu <qiudayu@linux.vnet.ibm.com>,
	Gavin Shan <gwshan@linux.vnet.ibm.com>,
	kvm-ppc@vger.kernel.org
Subject: Re: [PATCH] Bugfix: powerpc/eeh: Create eeh sysfs entry in post_init()
Date: Wed, 25 Jun 2014 16:37:22 +1000	[thread overview]
Message-ID: <20140625063721.GA5081@shangw> (raw)
In-Reply-To: <20140625062353.GB15078@richard>

On Wed, Jun 25, 2014 at 02:23:53PM +0800, Wei Yang wrote:
>On Wed, Jun 25, 2014 at 03:33:12PM +1000, Gavin Shan wrote:
>>On Tue, Jun 24, 2014 at 11:32:07PM -0400, Mike Qiu wrote:
>>
>>[ cc Richard ]
>>
>>>Eeh sysfs entry created must be after EEH_ENABLED been set
>>>in eeh_subsystem_flags.
>>>
>>>In PowerNV platform, it try to create sysfs entry before
>>>EEH_ENABLED been set, when boot up. So nothing will be
>>>created for eeh in sysfs.
>>>
>>
>>Could you please make the commit log more clear? :-)
>>
>>I guess the issue is introduced by commit 2213fb1 ("
>>powerpc/eeh: Skip eeh sysfs when eeh is disabled"). The
>>commit checks EEH is enabled while creating PCI device
>>EEH sysfs files. If not, the sysfs files won't be created.
>>That's to avoid warning reported during PCI hotplug.
>>
>>The problem you're reporting (if I understand completely):
>>You don't see the sysfs files after the system boots up.
>>If it's the case, you probably need following changes in
>>arch/powerpc/platforms/powernv/pci.c::pnv_pci_ioda_fixup().
>>Could you have a try with it?
>>
>>#ifdef CONFIG_EEH
>>	eeh_probe_mode_set(EEH_PROBE_MODE_DEV);
>>-	eeh_addr_cache_build();
>>	eeh_init();
>>+	eeh_addr_cache_build();
>>#endif
>>
>
>I think this is a more proper fix.
>
>BTW, I have one confusion in this mode set.
>
>eeh_init()
>  -> eeh_ops->dev_probe()
>     -> powernv_eeh_dev_probe()
>        -> eeh_set_enable(true)   <- here the eeh is marked enabled
>
>We can see this flag would be set for each pci_dev. So is it possible to make
>this "set" only once?
>

It shouldn't be a problem because there might not have PCI devices
supporting EEH in the guest. All PCI devices are emulated.

>>Eventually PowerNV/pSeries have same function call sequence:
>>
>>- Set EEH probe mode
>>- Doing probe (with device node or PCI device)
>>- Build address cache.
>>
>>>Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>>---
>>> arch/powerpc/platforms/powernv/eeh-ioda.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>>index 8ad0c5b..5f95581 100644
>>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>>@@ -136,6 +136,9 @@ static int ioda_eeh_post_init(struct pci_controller *hose)
>>> 	struct pnv_phb *phb = hose->private_data;
>>> 	int ret;
>>>
>>>+	/* Creat sysfs after EEH_ENABLED been set */
>>>+	eeh_add_sysfs_files(hose->bus);
>>>+
>>> 	/* Register OPAL event notifier */
>>> 	if (!ioda_eeh_nb_init) {
>>> 		ret = opal_notifier_register(&ioda_eeh_nb);

Thanks,
Gavin