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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 25 Jun 2024 15:08:12 +0530
Shradha Todi <shradha.t@samsung.com> 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@samsung.com>

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()


> +	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;
>  };