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

On Wed, Mar 05, 2025 at 12:46:38AM +0900, Krzysztof Wilczyński wrote:

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:

  https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/dwc&id=c6759a967e69aba16aef0d92f43e527b112e98a5

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.

Even though debugfs_init() failure is not supposed to fail the probe(),
dwc_pcie_rasdes_debugfs_init() has a devm_kzalloc() and propagating that
failure would be canolically correct IMO.

So I would still opt to have my version + your previous one.

- Mani

-- 
மணிவண்ணன் சதாசிவம்