[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa44eac9-8986-46d2-899d-df8811131925@163.com>
Date: Sun, 6 Apr 2025 00:35:20 +0800
From: Hans Zhang <18255117159@....com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: bhelgaas@...gle.com, jonathanh@...dia.com, kw@...ux.com,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linux-tegra@...r.kernel.org, lpieralisi@...nel.org,
manivannan.sadhasivam@...aro.org, robh@...nel.org, thierry.reding@...il.com
Subject: Re: [PATCH] pci: tegra194: Fix debugfs cleanup for !CONFIG_PCIEASPM
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?
Best regards,
Hans
Powered by blists - more mailing lists