[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c0c1476c-75a2-4d45-83a2-4751a7487892@wanadoo.fr>
Date: Sat, 5 Apr 2025 19:04:36 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: 18255117159@....com
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
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
>
>
>
Powered by blists - more mailing lists