Re: [PATCH] PCI: keystone: deinitialize endpoint on remove

[Marek Vasut <marek.vasut@mailbox.org>]

On 9/21/26 4:15 PM, Guangshuo Li wrote:
> Hi Marek,
> 
> Thanks for the review.
> 
> On Thu, 17 Sept 2026 at 06:56, Marek Vasut <marek.vasut@mailbox.org> wrote:
>>
>> On 9/16/26 10:00 AM, Guangshuo Li wrote:
>>> ks_pcie_probe() initializes the DesignWare PCIe endpoint with
>>> dw_pcie_ep_init(). The probe failure path calls dw_pcie_ep_deinit()
>>> when endpoint register initialization fails, but the remove path does
>>> not perform the corresponding endpoint teardown after a successful
>>> probe.
>>>
>>> The successful endpoint initialization also calls pci_epc_init_notify().
>>> Without the matching teardown on removal, the EPC initialization state
>>> and resources allocated by the DesignWare endpoint core are left
>>> active after the driver is removed.
>>>
>>> Notify the endpoint framework about deinitialization and call
>>> dw_pcie_ep_deinit() before disabling runtime PM and the PHYs.
>>>
>>> This issue was found by manual code inspection.
>>>
>>> Fixes: 23284ad677a9 ("PCI: keystone: Add support for PCIe EP in AM654x Platforms")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>>> ---
>>>    drivers/pci/controller/dwc/pci-keystone.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>> index 602516239a57..35787e727c5c 100644
>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>> @@ -1349,9 +1349,17 @@ static void ks_pcie_remove(struct platform_device *pdev)
>>>    {
>>>        struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
>>>        struct device_link **link = ks_pcie->link;
>>> +     struct dw_pcie *pci = ks_pcie->pci;
>>> +     const struct ks_pcie_of_data *data;
>>>        int num_lanes = ks_pcie->num_lanes;
>>>        struct device *dev = &pdev->dev;
>>>
>>> +     data = of_device_get_match_data(dev);
>>
>> Could you maybe cache the mode in struct keystone_pcie {} instead ?
>>
>>> +     if (data->mode == DW_PCIE_EP_TYPE) {
>>> +             pci_epc_deinit_notify(pci->ep.epc);
>>> +             dw_pcie_ep_deinit(&pci->ep);
>>
>> Would it make sense to make dw_pcie_ep_deinit() call
>> pci_epc_deinit_notify() , to avoid duplication in controller drivers ?
>>
>>> +     }
>>> +
>>>        pm_runtime_put(dev);
>>>        pm_runtime_disable(dev);
>>>        ks_pcie_disable_phy(ks_pcie);
>>
>> --
>> Best regards,
>> Marek Vasut
> 
> Agreed. I'll cache the mode in struct keystone_pcie and move
> pci_epc_deinit_notify() into dw_pcie_ep_deinit() so controller drivers
> do not need to duplicate it. I'll send a v2 with those changes.
Thank you