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

[Michael Ellerman <mpe@ellerman.id.au>]

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