Re: [PATCH] pci: tegra194: Fix debugfs cleanup for !CONFIG_PCIEASPM

[Christophe JAILLET]

Le 05/04/2025 à 18:47, Hans Zhang a écrit :
> On 2025/4/6 00:35, Hans Zhang wrote:
> > On 2025/4/6 00:17, Christophe JAILLET wrote:
> > > Le 05/04/2025 à 17:49, Hans Zhang a écrit :
> > > > On 2025/4/5 23:28, Bjorn Helgaas wrote:
> > > > > Follow subject line capitalization convention.
> > > > >
> > > > > On Sat, Apr 05, 2025 at 10:54:59PM +0800, Hans Zhang wrote:
> > > > > > When CONFIG_PCIEASPM is disabled, debugfs entries are not created, 
> > > > > > but
> > > > > > tegra_pcie_dw_remove() and tegra_pcie_dw_shutdown() 
> > > > > > unconditionally call
> > > > > > debugfs_remove_recursive(), leading to potential NULL pointer 
> > > > > > operations.
> > > > > >
> > > > > > Introduce deinit_debugfs() to wrap debugfs_remove_recursive(), 
> > > > > > which is
> > > > > > stubbed for !CONFIG_PCIEASPM. Use this function during removal/ 
> > > > > > shutdown to
> > > > > > ensure debugfs cleanup only occurs when entries were initialized.
> > > > > >
> > > > > > This prevents kernel warnings and instability when ASPM support is
> > > > > > disabled.
> > > > >
> > > > > This looks like there should be a Fixes: tag to connect this to the
> > > > > commit that introduced the problem.
> > > >
> > > > Hi Bjorn,
> > > >
> > > > Thanks your for reply. Will add.
> > > >
> > > > Fixes: bb617cbd8151 (PCI: tegra194: Clean up the exit path for 
> > > > Endpoint mode)
> > > >
> > > > > If this is something that broke with the v6.15 merge window, we should
> > > > > include this in v6.15 via pci/for-linus.  If this broke earlier, we
> > > > > would have to decide whether pci/for-linus is still appropriate or a
> > > > > stable tag.
> > > >
> > > > The original code that introduced the unconditional 
> > > > `debugfs_remove_recursive()` calls was actually merged in an earlier 
> > > > cycle.
> > > >
> > > > > We did merge some debugfs things for v6.15, but I don't see anything
> > > > > specific to pcie-tegra194.c, so I'm confused about why this fix would
> > > > > be in pcie-tegra194.c instead of some more generic place.
> > > >
> > > > The Tegra194 driver conditionally initializes pcie->debugfs based on 
> > > > CONFIG_PCIEASPM. When ASPM is disabled, pcie->debugfs remains 
> > > > uninitialized, but tegra_pcie_dw_remove() and 
> > > > tegra_pcie_dw_shutdown() unconditionally call 
> > > > debugfs_remove_recursive(), leading to a NULL 
> > >
> > > debugfs IS initialized, because it is in a structure allocated with 
> > > devm_kzalloc().
> > >
> > > And debugfs functions handle such cases.
> >
> > Oh, my mind went wrong and I didn't pay attention to devm, and I'm 

Here, what is relevant in devm_kzalloc() is not devm but the kzalloc 
part. the "z" is for zeroing the allocated memory.

> > really sorry about that.
> >
> > Another problem I noticed here is that currently, no matter what, 
> > pcie->debugfs = debugfs_create_dir(name, NULL) is executed; if #if 
> > defined(CONFIG_PCIEASPM) is valid, then pcie->debugfs = 
> > debugfs_create_dir(name, NULL); Is it superfluous?
>
> Sorry, let me reply again:
> Another problem I noticed here is that currently, no matter what, pcie- 
>  >debugfs = debugfs_create_dir(name, NULL) is executed; if #if 
> defined(CONFIG_PCIEASPM) is invalid, then pcie->debugfs = 
> debugfs_create_dir(name, NULL); Is it superfluous?

AFAICT, it looks useless in this case.

I guess that moving debugfs_create_dir() and the
name = devm_kasprintf()...
above it, into init_debugfs() would do the trick.

CJ

> Best regards,
> Hans