On Wed, Feb 03, 2016 at 09:54:51AM -0200, Guilherme G. Piccoli wrote:
>On 02/02/2016 08:44 PM, Gavin Shan wrote:
>>>/**
>>>+ * eeh_available - Checks for the availability of EEH based on running
>>>+ * architecture.
>>>+ *
>>>+ * This routine should be used in case we need to check if EEH is
>>>+ * available in some situation, regardless if EEH is enabled or not.
>>>+ * For example, if we hotplug-add a PCI device on a machine with no
>>>+ * other PCI device, EEH won't be enabled, yet it's available if the
>>>+ * arch supports it.
>>>+ */
>>>+static inline bool eeh_available(void)
>>>+{
>>>+ if (machine_is(pseries) || machine_is(powernv))
>>>+ return true;
>>>+ return false;
>>>+}
>>>+
>>
>>As I was told by somebody else before, the comments for static function
>>needn't to be exported.
>>
>
>Thanks for the advice Gavin, but I didn't get it. What means comments not
>being exported? For sure I can change the comment if desired, but I can't see
>any issue with it.
>
The comments in "/** ... */" can be exposed to readable document by makefile.
I think it's necessary to have it for a static function :-)
>>>+/**
>>> * eeh_add_device_early - Enable EEH for the indicated device node
>>> * @pdn: PCI device node for which to set up EEH
>>> *
>>>@@ -1072,7 +1089,7 @@ void eeh_add_device_early(struct pci_dn *pdn)
>>> struct pci_controller *phb;
>>> struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
>>>
>>>- if (!edev || !eeh_enabled())
>>>+ if (!edev || !eeh_available())
>>> return;
>>>
>>> if (!eeh_has_flag(EEH_PROBE_MODE_DEVTREE))
>>
>>The change here seems not correct enough. Before the changes,
>>eeh_add_device_early()
>>does nothing if EEH is disabled on pSeries. With the changes applied, the EEH
>>device
>>(edev) will be scanned even EEH is disabled on pSeries.
>>
>> From the code changes, I didn't figure out the real problem you try to fix.
>> Cell
>>platform doesn't have flag EEH_PROBE_MODE_DEVTREE. So the function does
>>nothing
>>on Cell platform except calling into pdn_to_eeh_dev(). I'm not sure if the
>>kernel
>>crashed in pdn_to_eeh_dev() on Cell platform?
>
>Gavin, thanks for the comments. Seems your commit d91dafc02f4 ("powerpc/eeh:
>Delay probing EEH device during hotplug") solved the problem with Cell arch.
>Notice that Michael pointed the issue with Cell and fixed it by commit
>89a51df5ab1d ("powerpc/eeh: Fix crash in eeh_add_device_early() on Cell").
>
>But you commit is recent than Michael's and since it adds the check for
>EEH_PROBE_MODE_DEVTREE, the Cell crash doesn't happen anymore. It's my bad, I
>should have checked the dates of commits to figure your commit is recent than
>Michael's one.
>
>Anyway, the modification proposed by this patch is useless if we revert
>Michael's commit then. Gavin and Michael, are you OK if I revert it?
>
>The reason for the revert is that we can't check for eeh_enabled() here -
>imagine the following scenario: we boot a machine with no PCI device, then,
>we hotplug-add a PCI device. When we reach eeh_add_device_early(), the
>function eeh_enabled() is false because at boot time we had no PCI devices so
>EEH is not initialized/enabled.
>
Yeah, eeh_enabled() returns false and we don't have EEH_PROBE_MODE_DEVTREE on
Cell
platform. That means those two checks are duplicated. I think eeh_enabled()
check
can be removed if it really helps to avoid the crash.
Thanks,
Gavin
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
