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

From: Bjorn Helgaas
Date: Fri May 21 2021 - 15:58:04 EST


On Tue, May 18, 2021 at 11:16:17PM +0530, Shradha Todi wrote:
> RAS is a vendor specific extended PCIe capability which which helps to read
> hardware internal state.
>
> This framework support provides debugfs entries to use these features for
> DesignWare controller. Following primary features of DesignWare controllers
> are provided to userspace via debugfs:
> - Debug registers
> - Error injection
> - Statistical counters
>
> Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/Kconfig | 9 +
> drivers/pci/controller/dwc/Makefile | 1 +
> .../controller/dwc/pcie-designware-debugfs.c | 544 ++++++++++++++++++
> .../controller/dwc/pcie-designware-debugfs.h | 133 +++++
> drivers/pci/controller/dwc/pcie-designware.h | 1 +
> 5 files changed, 688 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
> create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 423d35872ce4..2d0a18cb9418 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -6,6 +6,15 @@ menu "DesignWare PCI Core Support"
> config PCIE_DW
> bool
>
> +config PCIE_DW_DEBUGFS
> + bool "DWC PCIe debugfs entries"
> + default n
> + help
> + Enables debugfs entries for the DWC PCIe Controller.
> + These entries make use of the RAS DES features in the DW
> + controller to help in debug, error injection and statistical
> + counters

Maybe omit "DES"? The rest of the sentence spells it out, but the
connection isn't completely obvious, and I don't think the first "DES"
adds anything.

> +
> config PCIE_DW_HOST
> bool
> depends on PCI_MSI_IRQ_DOMAIN
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..6ad12638f793 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_PCIE_DW) += pcie-designware.o
> +obj-$(CONFIG_PCIE_DW_DEBUGFS) += pcie-designware-debugfs.o
> obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
> obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> new file mode 100644
> index 000000000000..84bf196df240
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@xxxxxxxxxxx>
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "pcie-designware.h"
> +#include "pcie-designware-debugfs.h"
> +
> +#define DEBUGFS_BUF_MAX_SIZE 100
> +
> +static char debugfs_buf[DEBUGFS_BUF_MAX_SIZE];

Should not be static, file scope. More comments below.

> +static struct dentry *dir;
> +static struct dentry *sub_dir;

These should not be static, file scope either. I think "dir" should
be returned by create_debugfs_files(), and the caller should hang onto
it until it needs it for remove_debugfs_files().

"sub_dir" looks like it's only needed inside create_debugfs_files(),
so it should be a local variable there.

> +static struct event_counters ras_events[] = {
> + {"ebuf_overflow", ebuf_overflow},
> + {"ebuf_underrun", ebuf_underrun},
> + {"decode_error", decode_error},
> + {"sync_header_error", sync_header_error},
> + {"receiver_error", receiver_error},
> + {"framing_error", framing_error},
> + {"lcrc_error", lcrc_error},
> + {"ecrc_error", ecrc_error},
> + {"unsupp_req_error", unsupp_req_error},
> + {"cmpltr_abort_error", cmpltr_abort_error},
> + {"cmpltn_timeout_error", cmpltn_timeout_error},
> + {"tx_l0s_entry", tx_l0s_entry},
> + {"rx_l0s_entry", rx_l0s_entry},
> + {"l1_entry", l1_entry},
> + {"l1_1_entry", l1_1_entry},
> + {"l1_2_entry", l1_2_entry},
> + {"l2_entry", l2_entry},
> + {"speed_change", speed_change},
> + {"width_chage", width_chage},
> + {0, 0},
> +};
> +
> +static struct error_injections error_list[] = {
> + {"tx_lcrc", tx_lcrc},
> + {"tx_ecrc", tx_ecrc},
> + {"rx_lcrc", rx_lcrc},
> + {"rx_ecrc", rx_ecrc},
> + {0, 0},
> +};
> +
> +static int open(struct inode *inode, struct file *file)
> +{
> + file->private_data = inode->i_private;
> +
> + return 0;
> +}
> +
> +/*
> + * set_event_number: Function to set event number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all event counters. Using the debugfs filname, the
> + * group number and event number for the counter is extracted and
> + * then programmed into the control register.

You can omit the "Function to" text here (more occurrences below).

You can also omit the "This function is " text. It's obvious that
this comment applies to this function.

> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: void
> + */
> +static void set_event_number(struct file *file)
> +{
> + int i;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> + u32 max_size = sizeof(ras_events) / sizeof(struct event_counters);

Reorder the local decls here and below like this:

struct dw_pcie *pci = file->private_data;
int i;
u32 val;

so things are in the order they're used. It's fairly conventional to
look up things based on incoming parameters first, e.g., the
"struct dw_pcie *pci = file->private_data;" (no cast needed there).

> + for (i = 0; i < max_size; i++) {

Drop "max_size" and use this instead:

for (i = 0; i < ARRAY_SIZE(ras_events); i++) {

> + if (strcmp(ras_events[i].name,
> + file->f_path.dentry->d_parent->d_iname) == 0) {

There are *very* few uses of d_iname in the tree, which makes me think
you're doing something wrong here. I don't have a suggestion yet, but
maybe take a look at other debugfs users to see if there's a common
pattern you can copy?

> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG);
> + val &= ~(EVENT_COUNTER_ENABLE);

Unnecessary parentheses.

> + val &= ~(0xFFF << 16);
> + val |= (ras_events[i].event_num << 16);
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> + break;
> + }
> + }
> +}

Add blank line here.

> +/*
> + * get_error_inj_number: Function to get error number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all error injection debugfs entries. Using the debugfs
> + * filname, the error group and type of error to be injected is extracted.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: u32
> + * [31:8]: Type of error to be injected
> + * [7:0]: Group of error it belongs to
> + */
> +

Remove spurious blank line above.

> +static u32 get_error_inj_number(struct file *file)
> +{
> + int i;
> + u32 max_size = sizeof(error_list) / sizeof(struct error_injections);

Drop "max_size" and use ARRAY_SIZE() again.

> + for (i = 0; i < max_size; i++) {
> + if (strcmp(error_list[i].name,
> + file->f_path.dentry->d_iname) == 0) {
> + return error_list[i].type_of_err;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * ras_event_counter_en_read: Function to get if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable
> + * It returns whether the counter is enabled or not
> + */
> +static ssize_t ras_event_counter_en_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;

Unnecessary cast, reorder decls (more occurrences below).

> + set_event_number(file);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG);

This *looks* like it might require mutual exclusion around the
"set_event_number(); dw_pcie_readl_dbi();" pair. Does
set_event_number() write a register that determines which counter
dw_pcie_readl_dbi() will read? If so, that should be made atomic
somehow.

> + val = (val >> EVENT_COUNTER_STATUS_SHIFT) & EVENT_COUNTER_STATUS_MASK;
> + if (val)
> + sprintf(debugfs_buf, "Enabled\n");
> + else
> + sprintf(debugfs_buf, "Disabled\n");
> +
> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));

I don't feel confident about this because:

- "debugfs_buf" is static and shared among all these users without
any obvious mutual exclusion, and

- sprintf() is a notorious problem for buffer overflows. This isn't
a problem here, but it does require extra effort to validate. I
think it would be better to use scnprintf(), which also has the
benefit of returning the value you need to pass to
simple_read_from_buffer(), so you don't need the strlen().
drivers/ntb/test/ntb_tool.c has lots of examples you could copy.

I don't *think* you have any buffer overflow issues in this file, but
you also don't seem to ever print more than a dozen or so characters.
Using buffers on the stack would be simple and make it obvious that
there are no mutual exclusion issues.

> +
> + return ret;

return simple_read_from_buffer(...);

and drop "ret" (also more occurrences below).

> +}
> +
> +/*
> + * ras_event_counter_lane_sel_read: Function to get lane number selected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * It returns for which lane the counter configurations are done
> + */
> +static ssize_t ras_event_counter_lane_sel_read(struct file *file,
> + char __user *buf, size_t count,
> + loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + set_event_number(file);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG);
> + val = (val >> LANE_SELECT_SHIFT) & LANE_SELECT_MASK;
> + sprintf(debugfs_buf, "0x%x\n", val);

I always use "%#x" because it's a tiny bit shorter and avoids the
possibility of typos like "0X%x" or "0x%X", which would produce
"0Xabcd" or "0xABCD". Weird personal idiosyncracy :)

> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));
> +
> + return ret;
> +}
> +
> +/*
> + * ras_event_counter_value_read: Function to get counter value
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_value
> + * It returns the number of time the selected event has happened if enabled
> + */
> +

Remove spurious blank line above.

> +static ssize_t ras_event_counter_value_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + set_event_number(file);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_DATA_REG);
> + sprintf(debugfs_buf, "0x%x\n", val);
> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));
> +
> + return ret;
> +}
> +
> +/*
> + * ras_event_counter_en_write: Function to set if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/
> + * ras_des_counter/<name>/counter_enable
> + * Here n can be 1 to enable and 0 to disable
> + */
> +static ssize_t ras_event_counter_en_write(struct file *file,
> + const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + u32 enable;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + ret = kstrtou32_from_user(buf, count, 0, &enable);
> + if (ret)
> + return ret;
> +
> + set_event_number(file);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG);
> + if (enable)
> + val |= PER_EVENT_ON;
> + else
> + val |= PER_EVENT_OFF;
> +
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> + return count;
> +}
> +
> +/*
> + * ras_event_counter_lane_sel_write: Function to set lane number
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * Here n is the lane that we want to select for counter configuration
> + */
> +static ssize_t ras_event_counter_lane_sel_write(struct file *file,
> + const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + u32 lane;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + ret = kstrtou32_from_user(buf, count, 0, &lane);
> + if (ret)
> + return ret;
> +
> + set_event_number(file);
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG);
> + val &= ~(LANE_SELECT_MASK << LANE_SELECT_SHIFT);
> + val |= (lane << LANE_SELECT_SHIFT);
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> + return count;
> +}
> +
> +/*
> + * ras_error_inj_read: Function to read number of errors left to be injected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * This returns the number of errors left to be injected which will
> + * keep reducing as we make pcie transactions to inject error.

Use "PCIe" in text.

> + */
> +static ssize_t ras_error_inj_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val, err_num, inj_num;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + err_num = get_error_inj_number(file);
> + inj_num = (err_num & 0xFF);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> + (0x4 * inj_num));
> + sprintf(debugfs_buf, "0x%x\n", (val & EINJ_COUNT_MASK));
> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));
> +
> + return ret;
> +}
> +
> +/*
> + * ras_error_inj_write: Function to set number of errors to be injected
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * Here n is the number of errors we want to inject
> + */
> +static ssize_t ras_error_inj_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val, err_num, inj_num;
> + u32 counter;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + if (count > DEBUGFS_BUF_MAX_SIZE)
> + return -EINVAL;

What is the point of this check? "debugfs_buf" isn't involved here.
Same question for the similar test below.

> + ret = kstrtou32_from_user(buf, count, 0, &counter);
> + if (ret)
> + return ret;
> +
> + err_num = get_error_inj_number(file);
> + inj_num = (err_num & 0xFF);
> + err_num = (err_num >> 8);
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> + (0x4 * inj_num));
> + val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
> + val |= (err_num << EINJ_TYPE_SHIFT);
> + val &= ~(EINJ_COUNT_MASK);
> + val |= counter;
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> + (0x4 * inj_num), val);
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> + ERR_INJ_ENABLE_REG, (0x1 << inj_num));
> +
> + return count;
> +}
> +
> +static ssize_t lane_detection_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + SD_STATUS_L1LANE_REG);
> + val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;
> + sprintf(debugfs_buf, "0x%x\n", val);
> +
> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));
> +
> + return ret;
> +}
> +
> +static ssize_t rx_valid_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + SD_STATUS_L1LANE_REG);
> + val = (val >> PIPE_RXVALID_SHIFT) & PIPE_RXVALID_MASK;
> + sprintf(debugfs_buf, "0x%x\n", val);
> +
> + ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> + strlen(debugfs_buf));
> +
> + return ret;
> +}
> +
> +static ssize_t lane_selection_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + u32 ret;
> + u32 val;
> + u32 lane;
> + struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> + if (count > DEBUGFS_BUF_MAX_SIZE)
> + return -EINVAL;
> +
> + ret = kstrtou32_from_user(buf, count, 0, &lane);
> + if (ret)
> + return ret;
> +
> + val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> + SD_STATUS_L1LANE_REG);
> + val &= ~(LANE_SELECT_MASK);

Unnecessary parentheses. If needed, they should be in the
LANE_SELECT_MASK definition.

> + val |= lane;
> + dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> + SD_STATUS_L1LANE_REG, val);
> +
> + return count;
> +}
> +
> +static const struct file_operations lane_detection_fops = {
> + .open = open,
> + .read = lane_detection_read,
> + .write = lane_selection_write
> +};
> +
> +static const struct file_operations rx_valid_fops = {
> + .open = open,
> + .read = rx_valid_read,
> + .write = lane_selection_write
> +};
> +
> +static const struct file_operations cnt_en_ops = {
> + .read = ras_event_counter_en_read,
> + .write = ras_event_counter_en_write,
> + .open = simple_open,
> +};
> +
> +static const struct file_operations lane_sel_ops = {
> + .read = ras_event_counter_lane_sel_read,
> + .write = ras_event_counter_lane_sel_write,
> + .open = simple_open,
> +};
> +
> +static const struct file_operations cnt_val_ops = {
> + .read = ras_event_counter_value_read,
> + .open = simple_open,
> +};
> +
> +static const struct file_operations inj_ops = {
> + .read = ras_error_inj_read,
> + .write = ras_error_inj_write,
> + .open = simple_open,
> +};
> +
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> + int ret = 0;
> + char dirname[32];
> + struct device *dev;
> +

Remove spurious blank line above.

> + struct dentry *ras_des_debug_regs;
> + struct dentry *ras_des_error_inj;
> + struct dentry *ras_des_event_counter;
> + struct dentry *lane_detection;
> + struct dentry *rx_valid;
> +
> + if (!pci) {
> + pr_err("pcie struct is NULL\n");
> + return -ENODEV;
> + }
> +
> + dev = pci->dev;
> + sprintf(dirname, "pcie_dwc_%s", dev_name(dev));

scnprintf() to prevent buffer overflow.

> + pci->ras_cap_offset = dw_pcie_find_vsec_capability(pci,
> + DW_PCIE_RAS_CAP_ID);
> + if (!pci->ras_cap_offset) {
> + pr_err("No RAS capability available\n");

Use dev_err() here and below to connect the message with the device.

> + return -ENODEV;
> + }
> +
> + /* Create main directory for each platform driver */
> + dir = debugfs_create_dir(dirname, NULL);
> + if (dir == NULL) {

"if (!dir)" is typical style for NULL pointer checking.

> + pr_err("error creating directory: %s\n", dirname);
> + return -ENODEV;
> + }
> +
> + /* Create sub dirs for Debug, Error injection, Statistics */
> + ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
> + if (ras_des_debug_regs == NULL) {
> + pr_err("error creating directory: %s\n", dirname);
> + ret = -ENODEV;
> + goto remove_debug_file;
> + }
> +
> + ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
> + if (ras_des_error_inj == NULL) {
> + pr_err("error creating directory: %s\n", dirname);
> + ret = -ENODEV;
> + goto remove_debug_file;
> + }
> +
> + ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
> + if (ras_des_event_counter == NULL) {
> + pr_err("error creating directory: %s\n", dirname);
> + ret = -ENODEV;
> + goto remove_debug_file;
> + }
> +
> + /* Create debugfs files for Debug subdirectory */
> + lane_detection = debugfs_create_file("lane_detection", 0644,
> + ras_des_debug_regs, pci,
> + &lane_detection_fops);
> +
> + rx_valid = debugfs_create_file("rx_valid", 0644,
> + ras_des_debug_regs, pci,
> + &lane_detection_fops);
> +
> + /* Create debugfs files for Error injection sub dir */
> + CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_ecrc);
> + CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_ecrc);
> + CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_lcrc);
> + CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_lcrc);
> +
> + /* Create debugfs files for Statistical counter sub dir */
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_overflow);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_underrun);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(decode_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(receiver_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(framing_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(lcrc_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(ecrc_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(unsupp_req_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltr_abort_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltn_timeout_error);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(tx_l0s_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(rx_l0s_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_1_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_2_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(l2_entry);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(speed_change);
> + CREATE_RAS_EVENT_COUNTER_DEBUGFS(width_chage);
> +
> + return ret;
> +
> +remove_debug_file:
> + remove_debugfs_files();
> + return ret;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> + debugfs_remove_recursive(dir);
> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h b/drivers/pci/controller/dwc/pcie-designware-debugfs.h

This new file is only included from one place. Why not just put the
contents there?

> new file mode 100644
> index 000000000000..3dc3cf696a04
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@xxxxxxxxxxx>
> + */
> +
> +#ifndef _PCIE_DESIGNWARE_DEBUGFS_H
> +#define _PCIE_DESIGNWARE_DEBUGFS_H
> +
> +#include "pcie-designware.h"
> +
> +#define RAS_DES_EVENT_COUNTER_CTRL_REG 0x8
> +#define RAS_DES_EVENT_COUNTER_DATA_REG 0xC
> +#define SD_STATUS_L1LANE_REG 0xB0
> +#define ERR_INJ_ENABLE_REG 0x30
> +#define ERR_INJ0_OFF 0x34
> +
> +#define LANE_DETECT_SHIFT 17
> +#define LANE_DETECT_MASK 0x1
> +#define PIPE_RXVALID_SHIFT 18
> +#define PIPE_RXVALID_MASK 0x1
> +
> +#define LANE_SELECT_SHIFT 8
> +#define LANE_SELECT_MASK 0xF
> +#define EVENT_COUNTER_STATUS_SHIFT 7
> +#define EVENT_COUNTER_STATUS_MASK 0x1
> +#define EVENT_COUNTER_ENABLE (0x7 << 2)
> +#define PER_EVENT_OFF (0x1 << 2)
> +#define PER_EVENT_ON (0x3 << 2)
> +
> +#define EINJ_COUNT_MASK 0xFF
> +#define EINJ_TYPE_MASK 0xFFFFFF
> +#define EINJ_TYPE_SHIFT 8
> +
> +enum event_numbers {
> + ebuf_overflow = 0x0,
> + ebuf_underrun = 0x1,
> + decode_error = 0x2,
> + sync_header_error = 0x5,
> + receiver_error = 0x106,
> + framing_error = 0x109,
> + lcrc_error = 0x201,
> + ecrc_error = 0x302,
> + unsupp_req_error = 0x303,
> + cmpltr_abort_error = 0x304,
> + cmpltn_timeout_error = 0x305,
> + tx_l0s_entry = 0x502,
> + rx_l0s_entry = 0x503,
> + l1_entry = 0x505,
> + l1_1_entry = 0x507,
> + l1_2_entry = 0x508,
> + l2_entry = 0x50B,
> + speed_change = 0x50C,
> + width_chage = 0x50D,
> +};
> +
> +/*
> + * struct event_counters: Struct to store event number
> + *
> + * @name: name of the event counter
> + * eg: ecrc_err count, l1 entry count
> + * @event_num: Event number and group number
> + * [16:8]: Group number
> + * [7:0]: Event number
> + */
> +struct event_counters {
> + const char *name;
> + u32 event_num;
> +};
> +
> +enum error_inj_code {
> + tx_lcrc = 0x000,
> + tx_ecrc = 0x300,
> + rx_lcrc = 0x800,
> + rx_ecrc = 0xB00,
> +};
> +
> +/*
> + * struct error_injectionss: Struct to store error numbers

s/error_injectionss/error_injections/

> + *
> + * @name: name of the error to be injected
> + * eg: ecrc_err, lcrc_err
> + * @event_num: Error number and group number
> + * [31:8]: Error type. This should be same as bits [31:8]
> + * in the EINJn_REG where n is group number
> + * [7:0]: Error injection group
> + * 0 - CRC
> + * 1 - seq num
> + * 2 - DLLP error
> + * 3 - symbol error
> + * 4 - FC error
> + */
> +struct error_injections {
> + const char *name;
> + u32 type_of_err;
> +};
> +
> +#define CREATE_RAS_EVENT_COUNTER_DEBUGFS(name) \
> +do { \
> + sub_dir = debugfs_create_dir(#name, ras_des_event_counter); \
> + debugfs_create_file("counter_enable", 0644, sub_dir, pci, \
> + &cnt_en_ops); \
> + debugfs_create_file("lane_select", 0644, sub_dir, pci, \
> + &lane_sel_ops); \
> + debugfs_create_file("counter_value", 0444, sub_dir, pci, \
> + &cnt_val_ops); \
> +} while (0)
> +
> +#define CREATE_RAS_ERROR_INJECTION_DEBUGFS(name) \
> + debugfs_create_file(#name, 0644, ras_des_error_inj, pci, \
> + &inj_ops);
> +
> +#ifdef CONFIG_PCIE_DW_DEBUGFS
> +int create_debugfs_files(struct dw_pcie *pci);
> +void remove_debugfs_files(void);
> +#else
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> + /* No OP */
> + return 0;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> + /* No OP */
> +}
> +#endif
> +
> +#endif
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 307525aaa063..031fdafe0a3e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -278,6 +278,7 @@ struct dw_pcie {
> u8 n_fts[2];
> bool iatu_unroll_enabled: 1;
> bool io_cfg_atu_shared: 1;
> + u32 ras_cap_offset;

u16 should be sufficient, since that's what
dw_pcie_find_vsec_capability() returns.

I would just call it "ras" with a comment here about it being a "VSEC
RAS capability". But that's just a personal preference.

> };
>
> #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> --
> 2.17.1
>