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

[Marek Vasut]

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@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.
Thank you