[v2] powernv/pci: Add PHB register dump debugfs handle

On EEH events the kernel will print a dump of relevant registers.
If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform
doesn't have EEH support, etc) this information isn't readily available.

Add a new debugfs handler to trigger a PHB register dump, so that this
information can be made available on demand.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
V2 changes:
	- use a simple attribute instead of full fops thanks to mpe
	- miscellanous fixes thanks to Gavin
	- rename from "regdump" to "dump_regs"
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 43 ++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

On Wed, Jul 27, 2016 at 04:14:04PM +1000, Russell Currey wrote:
>On EEH events the kernel will print a dump of relevant registers.
>If EEH is unavailable (i.e. CONFIG_EEH is disabled, a new platform
>doesn't have EEH support, etc) this information isn't readily available.
>
>Add a new debugfs handler to trigger a PHB register dump, so that this
>information can be made available on demand.
>
>Signed-off-by: Russell Currey <ruscur@russell.cc>

Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
>V2 changes:
>	- use a simple attribute instead of full fops thanks to mpe
>	- miscellanous fixes thanks to Gavin
>	- rename from "regdump" to "dump_regs"
>---
> arch/powerpc/platforms/powernv/pci-ioda.c | 43 ++++++++++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>index 891fc4a..2b9f114 100644
>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>@@ -3018,6 +3018,42 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe)
> 	}
> }
>
>+#ifdef CONFIG_DEBUG_FS
>+static int pnv_pci_diag_data_set(void *data, u64 val)
>+{
>+	struct pci_controller *hose;
>+	struct pnv_phb *phb;
>+	int ret;

s/int/int64_t

>+
>+	if (val != 1)
>+		return -EINVAL;

if (val != 1ULL)

>+
>+	hose = (struct pci_controller *)data;
>+	if (!hose)
>+		return -EFAULT;
>+
>+	phb = hose->private_data;
>+	if (!phb)
>+		return -EFAULT;

hose = (struct pci_controller *)data;
if (!hose || !hose->private_data)
	return -ENODEV;

phb = hose->private_data;

>+
>+	/* Retrieve the diag data from firmware */

Unnecessary comments as the code is obvious.

>+	ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>+					  PNV_PCI_DIAG_BUF_SIZE);
>+	if (ret != OPAL_SUCCESS)
>+		return -EIO;
>+
>+	/* Print the diag data to the kernel log */

Same as above.

>+	pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob);
>+	return 0;
>+}
>+
>+DEFINE_SIMPLE_ATTRIBUTE(pnv_pci_diag_data_fops,
>+			NULL,
>+			pnv_pci_diag_data_set,
>+			"%llu\n");
>+

Can be squeezed to 2 lines:

DEFINE_SIMPLE_ATTRIBUTE(pnv_pci_diag_data_fops, NULL,
			pnv_pci_diag_data_set, "%llu\n");

>+#endif /* CONFIG_DEBUG_FS */
>+
> static void pnv_pci_ioda_create_dbgfs(void)
> {
> #ifdef CONFIG_DEBUG_FS
>@@ -3033,9 +3069,14 @@ static void pnv_pci_ioda_create_dbgfs(void)
>
> 		sprintf(name, "PCI%04x", hose->global_number);
> 		phb->dbgfs = debugfs_create_dir(name, powerpc_debugfs_root);
>-		if (!phb->dbgfs)
>+		if (!phb->dbgfs) {
> 			pr_warning("%s: Error on creating debugfs on PHB#%x\n",
> 				__func__, hose->global_number);
>+			continue;
>+		}
>+
>+		debugfs_create_file("dump_regs", 0200, phb->dbgfs, hose,
>+				    &pnv_pci_diag_data_fops);

I still think "diag-data" is more indicative. It's also consistent with
the handler's name (pnv_pci_diag_data_set())?

> 	}
> #endif /* CONFIG_DEBUG_FS */
> }

Thanks,
Gavin

Gavin Shan <gwshan@linux.vnet.ibm.com> writes:

> On Wed, Jul 27, 2016 at 04:14:04PM +1000, Russell Currey wrote:
>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>index 891fc4a..2b9f114 100644
>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>@@ -3018,6 +3018,42 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe)
>> 	}
>> }
>>
>>+#ifdef CONFIG_DEBUG_FS
>>+static int pnv_pci_diag_data_set(void *data, u64 val)
>>+{
>>+	struct pci_controller *hose;
>>+	struct pnv_phb *phb;
>>+	int ret;
>
> s/int/int64_t

Actually please stick to the kernel types, so s64.

>>+	/* Retrieve the diag data from firmware */
>
> Unnecessary comments as the code is obvious.

Only if you know the OPAL API, for the rest of us that comment is fine IMHO.

>>+	ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>>+					  PNV_PCI_DIAG_BUF_SIZE);
>>+	if (ret != OPAL_SUCCESS)
>>+		return -EIO;
>>+
>>+	/* Print the diag data to the kernel log */

And that comment is very helpful, because the expectation is that the output
would go to the debugfs file.

>>@@ -3033,9 +3069,14 @@ static void pnv_pci_ioda_create_dbgfs(void)
>>
>> 		sprintf(name, "PCI%04x", hose->global_number);
>> 		phb->dbgfs = debugfs_create_dir(name, powerpc_debugfs_root);
>>-		if (!phb->dbgfs)
>>+		if (!phb->dbgfs) {
>> 			pr_warning("%s: Error on creating debugfs on PHB#%x\n",
>> 				__func__, hose->global_number);
>>+			continue;
>>+		}
>>+
>>+		debugfs_create_file("dump_regs", 0200, phb->dbgfs, hose,
>>+				    &pnv_pci_diag_data_fops);
>
> I still think "diag-data" is more indicative. It's also consistent with
> the handler's name (pnv_pci_diag_data_set())?

"dump_regs" is better because it's clearly a verb phrase. Which makes it clear
that manipulating the file causes something to happen.

"diag-data" could be a verb phrase, if you read "diag" as "diagnose", in which
case it would mean "diagnose the data". But that's not what the file does, it
doesn't diagnose anything, it just dumps some registers.

Most likely people will read "diag" as "diagnostic", in which case the name
means "diagnostic data" - which is a noun phrase. That indicates the file *is*
something, ie. an object, so when I read it I would expect to see the object,
and when I write to the file I would expect that to mutate the object.

cheers