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

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 07/21/2016 11:36 PM, Gavin Shan wrote:
> On Fri, Jul 22, 2016 at 03:23:36PM +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>
> 
>> ---
>> arch/powerpc/platforms/powernv/pci-ioda.c | 35 +++++++++++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index 891fc4a..ada2f3c 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -3018,6 +3018,38 @@ static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe)
>> 	}
>> }
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +static ssize_t pnv_pci_debug_write(struct file *filp,
>> +				   const char __user *user_buf,
>> +				   size_t count, loff_t *ppos)
>> +{
>> +	struct pci_controller *hose = filp->private_data;
>> +	struct pnv_phb *phb;
>> +	int ret = 0;
> 
> Needn't initialize @ret in advance. The code might be simpler, but it's
> only a personal preference:

I believe its actually preferred that it not be initialized in advance
so that the tooling can warn you about conditional code paths where you
may have forgotten to set a value. Or as Gavin suggests to explicitly
use error values in the return statements.

-Tyrel

> 
> 	struct pci_controller *hose = filp->private_data;
> 	struct pnv_phb *phb = hose ? hose->private_data : NULL;
> 
> 	if (!phb)
> 		return -ENODEV;
> 
>> +
>> +	if (!hose)
>> +		return -EFAULT;
>> +
>> +	phb = hose->private_data;
>> +	if (!phb)
>> +		return -EFAULT;
>> +
>> +	ret = opal_pci_get_phb_diag_data2(phb->opal_id, phb->diag.blob,
>> +					  PNV_PCI_DIAG_BUF_SIZE);
>> +
>> +	if (!ret)
>> +		pnv_pci_dump_phb_diag_data(phb->hose, phb->diag.blob);
>> +
>> +	return ret < 0 ? ret : count;
> 
> return ret == OPAL_SUCCESS ? count : -EIO;
> 
>> +}
>> +
>> +static const struct file_operations pnv_pci_debug_ops = {
>> +	.open	= simple_open,
>> +	.llseek	= no_llseek,
>> +	.write	= pnv_pci_debug_write,
> 
> It might be reasonable to dump the diag-data on read if it is trying
> to do it on write.
> 
>> +};
>> +#endif /* CONFIG_DEBUG_FS */
>> +
>> static void pnv_pci_ioda_create_dbgfs(void)
>> {
>> #ifdef CONFIG_DEBUG_FS
>> @@ -3036,6 +3068,9 @@ static void pnv_pci_ioda_create_dbgfs(void)
>> 		if (!phb->dbgfs)
>> 			pr_warning("%s: Error on creating debugfs on PHB#%x\n",
>> 				__func__, hose->global_number);
>> +
>> +		debugfs_create_file("regdump", 0200, phb->dbgfs, hose,
>> +				    &pnv_pci_debug_ops);
> 
> "diag-data" might be indicating or a better one you can name :)
> 
> Thanks,
> Gavin
> 
>> 	}
>> #endif /* CONFIG_DEBUG_FS */
>> }
>> -- 
>> 2.9.0
>>
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>