| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240701120939.00002d76@Huawei.com>
Date: Mon, 1 Jul 2024 12:09:39 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Shradha Todi <shradha.t@...sung.com>
CC: <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
<manivannan.sadhasivam@...aro.org>, <lpieralisi@...nel.org>, <kw@...ux.com>,
<robh@...nel.org>, <bhelgaas@...gle.com>, <jingoohan1@...il.com>,
<fancer.lancer@...il.com>, <yoshihiro.shimoda.uh@...esas.com>,
<conor.dooley@...rochip.com>, <pankaj.dubey@...sung.com>,
<gost.dev@...sung.com>
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@...sung.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@...sung.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;
> };
Powered by blists - more mailing lists