RE: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Shradha Todi]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Sent: 01 July 2024 16:40
> To: Shradha Todi <shradha.t@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> robh@xxxxxxxxxx; bhelgaas@xxxxxxxxxx; jingoohan1@xxxxxxxxx;
> fancer.lancer@xxxxxxxxx; yoshihiro.shimoda.uh@xxxxxxxxxxx;
> conor.dooley@xxxxxxxxxxxxx; pankaj.dubey@xxxxxxxxxxx;
> gost.dev@xxxxxxxxxxx
> Subject: Re: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC
> 
> On Tue, 25 Jun 2024 15:08:12 +0530
> Shradha Todi <shradha.t@xxxxxxxxxxx> wrote:
> 
> > Add support to use the RASDES feature of DesignWare PCIe controller
> > using debugfs entries.
> >
> > RASDES is a vendor specific extended PCIe capability which reads the
> > current hardware internal state of PCIe device. Following primary
> > features are provided to userspace via debugfs:
> > - Debug registers
> > - Error injection
> > - Statistical counters
> >
> > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> 
> A few minor things inline.
> 
> > +
> > +struct rasdes_info {
> > +	/* to store rasdes capability offset */
> > +	u32 ras_cap;
> > +	struct mutex dbg_mutex;
> 
> Add a comment on what data this mutex protects.
> 
> > +	struct dentry *rasdes;
> > +};
> 
> > +struct err_inj {
> Very generic name is likely to bite in future if similar gets defined in a
header. I'd at
> least prefix with dw_
> 
> > +	const char *name;
> > +	/* values can be from group 0 - 6 */
> > +	u32 err_inj_group;
> > +	/* within each group there can be types */
> > +	u32 err_inj_type;
> > +	/* More details about the error */
> > +	u32 err_inj_12_31;
> > +};
> 
> 
> > +
> > +int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci) {
> > +	struct device *dev = pci->dev;
> > +	int ras_cap;
> > +	struct rasdes_info *dump_info;
> > +	char dirname[DWC_DEBUGFS_MAX];
> > +	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
> > +	struct dentry *rasdes_event_counter, *rasdes_events;
> > +	int i;
> 
> Perhaps combine with int ras_cap above.
> 
> From a quick look, I think this can get called from resume paths as well as
initial
> setup which doesn't look like a safe thing to do.
> imx6_pcie_resume_irq()
> dw_pcie_setup_rc()
> dw_pcie_setup()
> dwc_pcie_rasdes_debugfs_init()
> 
> 

Thanks for review. I'm going to post the next version soon with all your
comments addressed.
Yes, I guess this is a fair point that calling it during resume path + setup is
not a safe thing to do.
I was thinking we leave it up to  platform drivers to call it and drop patch 3
altogether as I realised
that there is also no ep_deinit to call the
"dwc_pcie_rasdes_debugfs_deinit(pci)" from.
Would love to know your opinion on this before I proceed to post the changes.

> > +	struct rasdes_priv *priv_tmp;
> > +
> > +	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);
> > +	if (!ras_cap) {
> > +		dev_err(dev, "No RASDES capability available\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dump_info = devm_kzalloc(dev, sizeof(*dump_info), GFP_KERNEL);
> > +	if (!dump_info)
> > +		return -ENOMEM;
> > +
> > +	/* Create main directory for each platform driver */
> > +	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
> 
> dev_name could in theory be huge.  I'd use snprintf() and check the result as
more
> obviously correct.
> 
> > +	dir = debugfs_create_dir(dirname, NULL);
> 
> Check for errors in all these.
> 
> > +
> > +	/* Create subdirectories for Debug, Error injection, Statistics */
> > +	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
> > +	rasdes_err_inj = debugfs_create_dir("rasdes_err_inj", dir);
> > +	rasdes_event_counter = debugfs_create_dir("rasdes_event_counter",
> > +dir);
> > +
> > +	mutex_init(&dump_info->dbg_mutex);
> > +	dump_info->ras_cap = ras_cap;
> > +	dump_info->rasdes = dir;
> > +	pci->dump_info = dump_info;
> > +
> > +	/* Create debugfs files for Debug subdirectory */
> > +	dwc_debugfs_create(lane_detect);
> > +	dwc_debugfs_create(rx_valid);
> > +
> > +	/* Create debugfs files for Error injection subdirectory */
> > +	for (i = 0; i < ARRAY_SIZE(err_inj_list); i++) {
> > +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> > +		if (!priv_tmp)
> > +			goto err;
> > +
> > +		priv_tmp->idx = i;
> > +		priv_tmp->pci = pci;
> > +		debugfs_create_file(err_inj_list[i].name, 0200,
> > +				    rasdes_err_inj, priv_tmp, &err_inj_ops);
> > +	}
> > +
> > +	/* Create debugfs files for Statistical counter subdirectory */
> > +	for (i = 0; i < ARRAY_SIZE(event_counters); i++) {
> > +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> > +		if (!priv_tmp)
> > +			goto err;
> > +
> > +		priv_tmp->idx = i;
> > +		priv_tmp->pci = pci;
> > +		rasdes_events = debugfs_create_dir(event_counters[i].name,
> > +						   rasdes_event_counter);
> > +		if (event_counters[i].group_no == 0) {
> > +			debugfs_create_file("lane_select", 0644, rasdes_events,
> > +					    priv_tmp, &cnt_lane_ops);
> > +		}
> > +		debugfs_create_file("counter_value", 0444, rasdes_events,
> priv_tmp,
> > +				    &cnt_val_ops);
> > +		debugfs_create_file("counter_enable", 0644, rasdes_events,
> priv_tmp,
> > +				    &cnt_en_ops);
> > +	}
> > +
> > +	return 0;
> > +err:
> > +	dwc_pcie_rasdes_debugfs_deinit(pci);
> > +	return -ENOMEM;
> > +}
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> > b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> > new file mode 100644
> > index 000000000000..e69de29bb2d1
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 77686957a30d..9fa9f33e4ddb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -223,6 +223,8 @@
> >
> >  #define PCIE_RAS_DES_EVENT_COUNTER_DATA		0xc
> >
> > +#define DW_PCIE_RAS_DES_CAP			0x2
> 
> I'd be tempted to try and name that in a fashion that makes it clear it is a
vsec
> capability ID.  Currently it sounds like a top level capability ID.
> 
> > +
> >  /*
> >   * The default address offset between dbi_base and atu_base. Root
controller
> >   * drivers are not required to initialize atu_base if the offset
> > matches this @@ -410,6 +412,7 @@ struct dw_pcie {
> >  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
> >  	struct gpio_desc		*pe_rst;
> >  	bool			suspended;
> > +	void			*dump_info;
> >  };
>