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

[Guangshuo Li]

Hi Marek,

Thanks for the review.

On Thu, 17 Sept 2026 at 06:56, Marek Vasut <marek.vasut@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> >   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.

Thanks,
Guangshuo