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