Message-ID: <20250220094825.2idjwpuo2yo5n6tc@thinkpad>
Date: Thu, 20 Feb 2025 15:18:25 +0530
From: 'Manivannan Sadhasivam' <manivannan.sadhasivam@...aro.org>
To: Shradha Todi <shradha.t@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
	bhelgaas@...gle.com, jingoohan1@...il.com,
	Jonathan.Cameron@...wei.com, fan.ni@...sung.com,
	nifan.cxl@...il.com, a.manzanares@...sung.com,
	pankaj.dubey@...sung.com, cassel@...nel.org, 18255117159@....com,
	quic_nitegupt@...cinc.com, quic_krichai@...cinc.com,
	gost.dev@...sung.com
Subject: Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC

On Thu, Feb 20, 2025 at 02:48:12PM +0530, Shradha Todi wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > Sent: 18 February 2025 20:33
> > To: Shradha Todi <shradha.t@...sung.com>
> > Cc: linux-kernel@...r.kernel.org; linux-pci@...r.kernel.org; lpieralisi@...nel.org; kw@...ux.com; robh@...nel.org;
> > bhelgaas@...gle.com; jingoohan1@...il.com; Jonathan.Cameron@...wei.com; fan.ni@...sung.com; nifan.cxl@...il.com;
> > a.manzanares@...sung.com; pankaj.dubey@...sung.com; cassel@...nel.org; 18255117159@....com;
> > quic_nitegupt@...cinc.com; quic_krichai@...cinc.com; gost.dev@...sung.com
> > Subject: Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC
> > 
> > On Fri, Feb 14, 2025 at 04:20:05PM +0530, Shradha Todi wrote:
> > > Add support to provide silicon debug interface to userspace. This set
> > > of debug registers are part of the RASDES feature present in
> > > DesignWare PCIe controllers.
> > >
> > > Signed-off-by: Shradha Todi <shradha.t@...sung.com>
> > > ---
> > >  Documentation/ABI/testing/debugfs-dwc-pcie    |  13 ++
> > >  drivers/pci/controller/dwc/Kconfig            |  10 +
> > >  drivers/pci/controller/dwc/Makefile           |   1 +
> > >  .../controller/dwc/pcie-designware-debugfs.c  | 207 ++++++++++++++++++
> > >  .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
> > >  .../pci/controller/dwc/pcie-designware-host.c |   6 +
> > >  drivers/pci/controller/dwc/pcie-designware.h  |  20 ++
> > >  7 files changed, 262 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
> > >  create mode 100644
> > > drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > >
> > > diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie
> > > b/Documentation/ABI/testing/debugfs-dwc-pcie
> > > new file mode 100644
> > > index 000000000000..e8ed34e988ef
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> > > @@ -0,0 +1,13 @@
> > > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/lane_detect
> > > +Date:		Feburary 2025
> > 
> > Please align these fields
> 
> The fields are already aligned in my patch (when I check in git), not sure why the mail misaligns it. Can you suggest how
> should I fix this?
> 

You should consistently use tabs or space after colon throughout the file. Don't
mix them.

> > 
> > > +Contact:	Shradha Todi <shradha.t@...sung.com>
> > > +Description:	(RW) Write the lane number to be checked for detection.	Read
> > > +		will return whether PHY indicates receiver detection on the
> > > +		selected lane. The default selected lane is Lane0.
> > > +
> > > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/rx_valid
> > > +Date:		Feburary 2025
> > > +Contact:	Shradha Todi <shradha.t@...sung.com>
> > > +Description:	(RW) Write the lane number to be checked as valid or invalid. Read
> > > +		will return the status of PIPE RXVALID signal of the selected lane.
> > > +		The default selected lane is Lane0.

[...]

> > > +static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct
> > > +dentry *dir) {
> > > +	struct dentry *rasdes_debug;
> > > +	struct dwc_pcie_rasdes_info *rasdes_info;
> > > +	const struct dwc_pcie_vsec_id *vid;
> > > +	struct device *dev = pci->dev;
> > > +	int ras_cap;
> > > +
> > > +	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
> > > +		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
> > > +							vid->vsec_id);
> > > +		if (ras_cap)
> > > +			break;
> > > +	}
> > > +	if (!ras_cap) {
> > > +		dev_dbg(dev, "no rasdes capability available\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > This will also go inside a new API, dw_pcie_find_rasdes_capability(pci).
> > 
> 
> Okay, are we planning to make a function for each VSEC? Or should we just pass the rasdes_vids to the
> dw_pcie_find_vsec_capability?
> 

dw_pcie_find_vsec_capability() is a static function in my series. We should
add separate function for each VSEC.

> > > +
> > > +	rasdes_info = devm_kzalloc(dev, sizeof(*rasdes_info), GFP_KERNEL);
> > > +	if (!rasdes_info)
> > > +		return -ENOMEM;
> > > +
> > > +	/* Create subdirectories for Debug, Error injection, Statistics */
> > > +	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
> > 
> > _debug prefix is not needed since the directory itself belongs to debugfs.
> > 
> 
> It's not for the debug in debugfs. So the DES features are
> Debug
> Error Injection
> Statistics.
> The debug here is for the "D" in DES.
> 

Sorry, I missed it.

> > > +
> > > +	mutex_init(&rasdes_info->reg_lock);
> > > +	rasdes_info->ras_cap_offset = ras_cap;
> > > +	pci->debugfs->rasdes_info = rasdes_info;
> > > +
> > > +	/* Create debugfs files for Debug subdirectory */
> > > +	dwc_debugfs_create(lane_detect);
> > > +	dwc_debugfs_create(rx_valid);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void dwc_pcie_debugfs_deinit(struct dw_pcie *pci) {
> > > +	dwc_pcie_rasdes_debugfs_deinit(pci);
> > > +	debugfs_remove_recursive(pci->debugfs->debug_dir);
> > > +}
> > > +
> > > +int dwc_pcie_debugfs_init(struct dw_pcie *pci) {
> > > +	char dirname[DWC_DEBUGFS_BUF_MAX];
> > > +	struct device *dev = pci->dev;
> > > +	struct debugfs_info *debugfs;
> > > +	struct dentry *dir;
> > > +	int ret;
> > > +
> > > +	/* Create main directory for each platform driver */
> > > +	snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
> > > +	dir = debugfs_create_dir(dirname, NULL);
> > > +	if (IS_ERR(dir))
> > > +		return PTR_ERR(dir);
> > 
> > debugfs creation is not supposed to fail. So you should remove the error check.
> > 
> 
> There was no error check until v3. Got a comment from Jonathan in v3:
> 	"Check for errors in all these."
> I think he wanted to add in all the debugfs creations but I just added in the topmost directory.
> I checked that error will be returned in case someone turns off debugfs mounting as early param.
> So, if the first directory gets made, there would be no issues in subsequent subdirectories.
> 

Please see the Kdoc comments for this API:

 * NOTE: it's expected that most callers should _ignore_ the errors returned
 * by this function. Other debugfs functions handle the fact that the "dentry"
 * passed to them could be an error and they don't crash in that case.
 * Drivers should generally work fine even if debugfs fails to init anyway.

> > > +
> > > +	debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
> > > +	if (!debugfs)
> > > +		return -ENOMEM;
> > > +
> > > +	debugfs->debug_dir = dir;
> > > +	pci->debugfs = debugfs;
> > > +	ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> > > +	if (ret)
> > > +		dev_dbg(dev, "rasdes debugfs init failed\n");
> > 
> > RASDES
> > 
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index f3ac7d46a855..a87a714bb472 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -642,6 +642,7 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >
> > > +	dwc_pcie_debugfs_deinit(pci);
> > >  	dw_pcie_edma_remove(pci);
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_cleanup);
> > > @@ -813,6 +814,10 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep
> > > *ep)
> > >
> > >  	dw_pcie_ep_init_non_sticky_registers(pci);
> > >
> > > +	ret = dwc_pcie_debugfs_init(pci);
> > > +	if (ret)
> > > +		goto err_remove_edma;
> > > +
> > >  	return 0;
> > >
> > >  err_remove_edma:
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index d2291c3ceb8b..6b03ef7fd014 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -524,6 +524,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	if (ret)
> > >  		goto err_remove_edma;
> > >
> > > +	ret = dwc_pcie_debugfs_init(pci);
> > > +	if (ret)
> > > +		goto err_remove_edma;
> > > +
> > 
> > Why can't you move it to the end of the function?
> 
> So the debugfs entries record certain debug values that might be useful to read
> in case link does not come up. Therefore, I'm adding it before starting link up so that users can 
> read it in case some failure occurs in further steps.
> 

I don't understand this. Even if you move it to the end of the function, users
can still read the attributes and learn what happended with the link. We don't
fail if the link doesn't come up.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists