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

[Hans Zhang <18255117159@163.com>]

On 2025/4/6 01:04, Christophe JAILLET wrote:
> 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.
> 

Hi Christophe,

Thanks your for reply. I understand.

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

That's what I was thinking. I will submit the version again in the future.

Best regards,
Hans