linux-kernel - Re: [PATCH 3/4] PCI: cadence-ep: Introduce cdns_pcie_ep

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250319103217.aaoxpzk2baqna5vc@thinkpad>
Date: Wed, 19 Mar 2025 16:02:17 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: lpieralisi@...nel.org, kw@...ux.com, robh@...nel.org,
	bhelgaas@...gle.com, vigneshr@...com, kishon@...nel.org,
	cassel@...nel.org, wojciech.jasko-EXT@...tinental-corporation.com,
	thomas.richard@...tlin.com, bwawrzyn@...co.com,
	linux-pci@...r.kernel.org, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	srk@...com
Subject: Re: [PATCH 3/4] PCI: cadence-ep: Introduce cdns_pcie_ep_disable
 helper for cleanup

On Tue, Mar 18, 2025 at 01:42:39PM +0530, Siddharth Vadapalli wrote:
> On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote:
> 
> Hello Mani,
> 
> > On Fri, Mar 07, 2025 at 04:01:27PM +0530, Siddharth Vadapalli wrote:
> > > Introduce the helper function cdns_pcie_ep_disable() which will undo the
> > > configuration performed by cdns_pcie_ep_setup(). Also, export it for use
> > > by the existing callers of cdns_pcie_ep_setup(), thereby allowing them
> > > to cleanup on their exit path.
> > > 
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> > > ---
> > >  drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
> > >  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > index eeb2afdd223e..85bec57fa5d9 100644
> > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > @@ -646,6 +646,16 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = {
> > >  	.get_features	= cdns_pcie_ep_get_features,
> > >  };
> > >  
> > > +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
> > > +{
> > > +	struct device *dev = ep->pcie.dev;
> > > +	struct pci_epc *epc = to_pci_epc(dev);
> > > +
> > 
> > pci_epc_deinit_notify()
> 
> I had initially planned to add this, but I noticed that it is not
> invoked by dw_pcie_ep_deinit() within
> drivers/pci/controller/dwc/pcie-designware-ep.c
> Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I
> decided to drop it. Current callers of pci_epc_deinit_notify() are:
> drivers/pci/controller/dwc/pcie-qcom-ep.c
> drivers/pci/controller/dwc/pcie-tegra194.c
> while there are many more users of dw_pcie_ep_deinit() that don't invoke
> pci_epc_deinit_notify().
> 
> While I don't intend to justify dropping pci_epc_deinit_notify() in the
> cleanup path, I wanted to check if this should be added to
> dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
> is different from cdns_pcie_ep_disable()? Please let me know.
> 

Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is
supposed to be called while performing the resource cleanup with active refclk.

Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is
called when there is no refclk, it will crash the endpoint SoC. But since
cadence endpoint platforms seem to generate their own refclk, you can call
deinit_notify() during deinit phase.

That said, I noticed some issues in the DWC cleanup path while checking the code
now. Will submit fixes for them.

- Mani

-- 
மணிவண்ணன் சதாசிவம்