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

-----Original Message-----
From: Krzysztof Wilczyński <redacted>
Sent: 04 March 2025 21:17
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Fan Ni <redacted>; Shradha Todi <redacted>; linux-kernel@vger.kernel.org; linux-
pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-perf-users@vger.kernel.org; manivannan.sadhasivam@linaro.org;
lpieralisi@kernel.org; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
a.manzanares@samsung.com; pankaj.dubey@samsung.com; cassel@kernel.org; 18255117159@163.com;
xueshuai@linux.alibaba.com; renyu.zj@linux.alibaba.com; will@kernel.org; mark.rutland@arm.com; Yoshihiro Shimoda
[off-list ref]; Linux-Renesas [off-list ref]
Subject: Re: [PATCH v7 3/5] Add debugfs based silicon debug support in DWC

Hello,

quoted

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 [off-list ref] wrote:

quoted

[...]

quoted

quoted

+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.

quoted

quoted

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!

quoted

quoted

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

That said, perhaps moving the log level to a debug would be better served here.

	Krzysztof