Re: [2/5] powerpc/eeh: Add eeh_pe_state sysfs entry

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

On Thu, Sep 25, 2014 at 02:09:58PM +1000, Michael Ellerman wrote:
>On Sun, 2014-17-08 at 03:02:26 UTC, Gavin Shan wrote:
>> The patch adds sysfs entry "eeh_pe_state". Reading on it returns
>> the PE's state while writing to it clears the frozen state. It's
>> used to check or clear the PE frozen state from userland for
>> debugging purpose.
>> 
>> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
>> index e2595ba..e69bcbb 100644
>> --- a/arch/powerpc/kernel/eeh_sysfs.c
>> +++ b/arch/powerpc/kernel/eeh_sysfs.c
>> @@ -54,6 +54,63 @@ EEH_SHOW_ATTR(eeh_mode,            mode,            "0x%x");
>>  EEH_SHOW_ATTR(eeh_config_addr,     config_addr,     "0x%x");
>>  EEH_SHOW_ATTR(eeh_pe_config_addr,  pe_config_addr,  "0x%x");
>>  
>> +static ssize_t eeh_pe_state_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>> +	int state;
>> +
>> +	if (!edev || !edev->pe)
>> +		return 0;
>> +
>> +	state = eeh_ops->get_state(edev->pe, NULL);
>> +	return sprintf(buf, "PHB#%d-PE#%d: 0x%08x 0x%08x\n",
>> +		       edev->pe->phb->global_number,
>> +		       edev->pe->addr, state, edev->pe->state);
>
>Shouldn't this only display the state, ie not the number and addr etc.
>

Yes, I'll remove PHB#%d-PE#%d in next revision. Another sysfs entry
gives the PE number: /sys/bus/pci/devices/xxxx:xx:xx.x/eeh_pe_config_addr

>And why are there two states, state and edev->pe->state ?
>

state is from hardware, edev->pe->state is software maintained state.

>> +static ssize_t eeh_pe_state_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>> +	int ret;
>> +
>> +	if (!edev || !edev->pe)
>> +		return 0;
>
>Shouldn't that be an error?
>
>> +	/* Nothing to do if it's not frozen */
>> +	if (!(edev->pe->state & EEH_PE_ISOLATED))
>> +		return 0;
>> +
>> +	/* Enable MMIO */
>> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_MMIO);
>> +	if (ret) {
>> +		pr_warn("%s: Failure %d enabling MMIO for PHB#%d-PE#%d\n",
>> +			__func__, ret, edev->pe->phb->global_number,
>> +			edev->pe->addr);
>> +		return 0;
>
>Error ?
>
>> +	}
>> +
>> +	/* Enable DMA */
>> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_DMA);
>> +	if (ret) {
>> +		pr_warn("%s: Failure %d enabling DMA for PHB#%d-PE#%d\n",
>> +			__func__, ret, edev->pe->phb->global_number,
>> +			edev->pe->addr);
>> +		return 0;
>
>Error?
>

Yes, I'll fix all "Error" cases.

>And should it roll back, ie. unthaw MMIO?
>

It's not necessary as it's only for debugging purpose. The main
purpose is to keep dumping the PE hardware/software state when
recovering one specific PE.

Thanks,
Gavin