Re: [PATCH 1/2] powerpc/eeh: Check for EEH availability in eeh_add_device_early()

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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