Re: [PATCH] PCI: qcom-ep: Move controller cleanups to qcom_pcie_perst_deassert()

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Aug 16, 2024 at 10:30:29AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 15, 2024 at 05:47:17PM -0500, Bjorn Helgaas wrote:
> > [+cc Vidya, Jon since tegra194 does similar things]
> > 
> > On Mon, Jul 29, 2024 at 05:52:45PM +0530, Manivannan Sadhasivam wrote:
> > > Currently, the endpoint cleanup function dw_pcie_ep_cleanup() and EPF
> > > deinit notify function pci_epc_deinit_notify() are called during the
> > > execution of qcom_pcie_perst_assert() i.e., when the host has asserted
> > > PERST#. But quickly after this step, refclk will also be disabled by the
> > > host.
> > > 
> > > All of the Qcom endpoint SoCs supported as of now depend on the refclk from
> > > the host for keeping the controller operational. Due to this limitation,
> > > any access to the hardware registers in the absence of refclk will result
> > > in a whole endpoint crash. Unfortunately, most of the controller cleanups
> > > require accessing the hardware registers (like eDMA cleanup performed in
> > > dw_pcie_ep_cleanup(), powering down MHI EPF etc...). So these cleanup
> > > functions are currently causing the crash in the endpoint SoC once host
> > > asserts PERST#.
> > > 
> > > One way to address this issue is by generating the refclk in the endpoint
> > > itself and not depending on the host. But that is not always possible as
> > > some of the endpoint designs do require the endpoint to consume refclk from
> > > the host (as I was told by the Qcom engineers).
> > > 
> > > So let's fix this crash by moving the controller cleanups to the start of
> > > the qcom_pcie_perst_deassert() function. qcom_pcie_perst_deassert() is
> > > called whenever the host has deasserted PERST# and it is guaranteed that
> > > the refclk would be active at this point. So at the start of this function,
> > > the controller cleanup can be performed. Once finished, rest of the code
> > > execution for PERST# deassert can continue as usual.
> > 
> > What makes this v6.11 material?  Does it fix a problem we added in
> > v6.11-rc1?
> 
> No, this is not a 6.11 material, but the rest of the patches I
> shared offline.

For reference, the patches you shared offline are:

  PCI: qcom: Use OPP only if the platform supports it
  PCI: qcom-ep: Do not enable resources during probe()
  PCI: qcom-ep: Disable MHI RAM data parity error interrupt for SA8775P SoC
  PCI: qcom-ep: Move controller cleanups to qcom_pcie_perst_deassert()

> > Is there a Fixes: commit?
> 
> Hmm, the controller addition commit could be the valid fixes tag.
> 
> > This patch essentially does this:
> > 
> >   qcom_pcie_perst_assert
> > -   pci_epc_deinit_notify
> > -   dw_pcie_ep_cleanup
> >     qcom_pcie_disable_resources
> > 
> >   qcom_pcie_perst_deassert
> > +   if (pcie_ep->cleanup_pending)
> > +     pci_epc_deinit_notify(pci->ep.epc);
> > +     dw_pcie_ep_cleanup(&pci->ep);
> >     dw_pcie_ep_init_registers
> >     pci_epc_init_notify
> > 
> > Maybe it makes sense to call both pci_epc_deinit_notify() and
> > pci_epc_init_notify() from the PERST# deassert function, but it makes
> > me question whether we really need both.
> 
> There is really no need to call pci_epc_deinit_notify() during the first
> deassert (i.e., during the ep boot) because there are no cleanups to be done.
> It is only needed during a successive PERST# assert + deassert.
> 
> > pcie-tegra194.c has a similar structure:
> > 
> >   pex_ep_event_pex_rst_assert
> >     pci_epc_deinit_notify
> >     dw_pcie_ep_cleanup
> > 
> >   pex_ep_event_pex_rst_deassert
> >     dw_pcie_ep_init_registers
> >     pci_epc_init_notify
> > 
> > Is there a reason to make them different, or could/should a similar
> > change be made to tegra?
> 
> Design wise both drivers are similar, so it could apply. I didn't
> spin a patch because if testing of tegra driver gets delayed (I've
> seen this before), then I do not want to stall merging the whole
> series. 

It can and should be separate patches, one per driver.  But I don't
want to end up with the drivers being needlessly different.

> For Qcom it is important to get this merged asap to avoid
> the crash.

If this is not v6.11 material, there's time to work this out.

> > > +	if (pcie_ep->cleanup_pending) {
> > 
> > Do we really need this flag?  I assume the cleanup functions could
> > tell whether any previous setup was done?
> 
> Not so. Some cleanup functions may trigger a warning if attempted to do it
> before 'setup'. I think dw_edma_remove() that is part of dw_pcie_ep_cleanup()
> does that IIRC.

It looks safe to me:

  dw_pcie_ep_cleanup
    dw_pcie_edma_remove
      dw_edma_remove(chip = &pci->edma)       # struct dw_pcie *pci
        dev = chip->dev
        dw = chip->dw
        if (!dw)
          return -ENODEV

but if not, it could probably be made safe by adding a NULL pointer
check and/or a "chip->dw = NULL" at the right spot.

We hardly have any cleanup functions affected by "cleanup_pending", so
I think we can decide that they should be safe before 'setup' and just
make it so.

Bjorn