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

From: Jonathan Cameron
Date: Mon Jul 01 2024 - 07:10:03 EST


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


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