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

[Hans Zhang <18255117159@163.com>]

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 
> 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?

Best regards,
Hans