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

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.

The patch also replaces printk(KERN_WARNING ...) with pr_warn() in
eeh_sysfs.c

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/eeh_sysfs.c | 61 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

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.

And why are there two states, state and edev->pe->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?

And should it roll back, ie. unthaw MMIO?


cheers

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