[PATCH v7 3/5] Add debugfs based silicon debug support in DWC

[Shradha Todi]

> -----Original Message-----
> From: Krzysztof Wilczyński <kw at linux.com>
> Sent: 04 March 2025 21:17
> To: Geert Uytterhoeven <geert at linux-m68k.org>
> Cc: Fan Ni <nifan.cxl at gmail.com>; Shradha Todi <shradha.t at samsung.com>; linux-kernel at vger.kernel.org; linux-
> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-perf-users at vger.kernel.org; manivannan.sadhasivam at linaro.org;
> lpieralisi at kernel.org; robh at kernel.org; bhelgaas at google.com; jingoohan1 at gmail.com; Jonathan.Cameron at huawei.com;
> a.manzanares at samsung.com; pankaj.dubey at samsung.com; cassel at kernel.org; 18255117159 at 163.com;
> xueshuai at linux.alibaba.com; renyu.zj at linux.alibaba.com; will at kernel.org; mark.rutland at arm.com; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh at renesas.com>; Linux-Renesas <linux-renesas-soc at vger.kernel.org>
> Subject: Re: [PATCH v7 3/5] Add debugfs based silicon debug support in DWC
> 
> Hello,
> 
> > This patch is now commit 1ff54f4cbaed9ec6 ("PCI: dwc: Add debugfs
> > based Silicon Debug support for DWC") in pci/next (next-20250304).
> >
> > On Mon, 3 Mar 2025 at 20:47, Krzysztof Wilczyński <kw at linux.com> wrote:
> > > [...]
> > > > > +int dwc_pcie_debugfs_init(struct dw_pcie *pci) {
> > > > > +   char dirname[DWC_DEBUGFS_BUF_MAX];
> > > > > +   struct device *dev = pci->dev;
> > > > > +   struct debugfs_info *debugfs;
> > > > > +   struct dentry *dir;
> > > > > +   int ret;
> > > > > +
> > > > > +   /* Create main directory for each platform driver */
> > > > > +   snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
> > > > > +   dir = debugfs_create_dir(dirname, NULL);
> > > > > +   debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
> > > > > +   if (!debugfs)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   debugfs->debug_dir = dir;
> > > > > +   pci->debugfs = debugfs;
> > > > > +   ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> > > > > +   if (ret)
> > > > > +           dev_dbg(dev, "RASDES debugfs init failed\n");
> > > >
> > > > What will happen if ret != 0? still return 0?
> >
> > And that is exactly what happens on Gray Hawk Single with R-Car
> > V4M: dw_pcie_find_rasdes_capability() returns NULL, causing
> > dwc_pcie_rasdes_debugfs_init() to return -ENODEV.
> 
> Thank you for testing and for catching this issue.  Much appreciated.
> 
> > > Given that callers of dwc_pcie_debugfs_init() check for errors,
> >
> > Debugfs issues should never be propagated upstream!
> 
> Makes complete sense.  Sorry for breaking things here!
> 
> > > this probably should correctly bubble up any failure coming from
> > > dwc_pcie_rasdes_debugfs_init().
> > >
> > > I made updates to the code directly on the current branch, have a look:
> >
> > So while applying, you changed this like:
> >
> >             ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> >     -       if (ret)
> >     -               dev_dbg(dev, "RASDES debugfs init failed\n");
> >     +       if (ret) {
> >     +               dev_err(dev, "failed to initialize RAS DES debugfs\n");
> >     +               return ret;
> >     +       }
> >
> >             return 0;
> >
> > Hence this is now a fatal error, causing the probe to fail.
> 
> I removed the changed, and also move the log level to be a warning, per:
> 

Hey Krzysztof,
I think we shouldn't move the log level to be a WARN. I believe many controllers might not support
RAS DES feature in their design and giving a warn dump would draw unnecessary attention.
My opinion is to silently let it fail unless the user is actually interested in getting the RAS DES feature up.
We can wait for Mani's response though. But good catch to also add the error type, that's definitely a more
informative error log.

> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/dwc&id=c6759a967e69aba16aef0d92f43e527b
> 112e98a5
> 
> Would this be acceptable here?
> 
> Mani, would this be acceptable to you, too?  Given that you posted the following recently:
> 
>   https://lore.kernel.org/linux-pci/20250303200055.GA1881771@rocinante/T/#mab9cbd5834390d259afea056eee9a73d8c3b435f
> 
> That said, perhaps moving the log level to a debug would be better served here.
> 
> 	Krzysztof