Re: [PATCH 1/3] PCI: dwc: ep: Add dw_pcie_ep_deinit_notify()

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, May 28, 2024 at 09:17:40PM +0200, Niklas Cassel wrote:
> On Tue, May 28, 2024 at 10:55:34AM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 03:00:37PM +0200, Niklas Cassel wrote:
> > > Add a DWC specific wrapper function (dw_pcie_ep_deinit_notify()) around
> > > pci_epc_deinit_notify(), similar to how we have a wrapper function
> > > (dw_pcie_ep_init_notify()) around pci_epc_init_notify().
> > > 
> > > This will allow the DWC glue drivers to use the same API layer for init
> > > and deinit notification.
> > > 
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-ep.c | 13 +++++++++++++
> > >  drivers/pci/controller/dwc/pcie-designware.h    |  5 +++++
> > >  2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index 2063cf2049e5..3c9079651dff 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -39,6 +39,19 @@ void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_init_notify);
> > >  
> > > +/**
> > > + * dw_pcie_ep_deinit_notify - Notify EPF drivers about EPC deinitialization
> > > + *			      complete
> > > + * @ep: DWC EP device
> > > + */
> > > +void dw_pcie_ep_deinit_notify(struct dw_pcie_ep *ep)
> > > +{
> > > +	struct pci_epc *epc = ep->epc;
> > > +
> > > +	pci_epc_deinit_notify(epc);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dw_pcie_ep_deinit_notify);
> > 
> > What is the value of this wrapper?  
> > 
> > I see that dw_pcie_ep_deinit_notify() would be parallel to
> > dw_pcie_ep_init_notify() and dw_pcie_ep_linkup(), but none of these
> > has any DWC-specific content other than the fact that
> > pcie-designware.h provides stubs for the non-CONFIG_PCIE_DW_EP case.
> 
> Exactly what you are saying, consistency with the existing design.
> 
> To me, it seems a bit weird to use:
> dw_pcie_ep_init_notify() to notify init completion, and then to use
> pci_epc_deinit_notify() to notify deinit completion.
> 
> deinit notify callback should basically undo what the init notify callback
> did, so it would make sense that the naming of the API calls are similar.
> 
> > What if we added stubs to pci-epc.h pci_epc_init_notify(),
> > pci_epc_deinit_notify(), pci_epc_linkup(), and pci_epc_linkdown() for
> > the non-CONFIG_PCI_ENDPOINT case instead?  Then we might be able to
> > drop all these DWC-specific wrappers.
> 
> The PCI endpoint subsystem currently does not provide any stubs at all,
> so that would be a bigger change compared to this small patch.
> (And considering that the pci/endpoint branch does not build, I opted
> for the smaller patch.)

> Your suggestion would of course work as well, but if we go that route,
> then we should probably add stubs for all functions in both
> include/linux/pci-epc.h and include/linux/pci-epf.h.
> As long as the DWC glue drivers use the same "API layer" for init and
> deinit notification, I'm happy :)

The cadence, rcar, and rockchip drivers use pci_epc_init_notify() with
no wrapper.

A bunch of DWC-based drivers (artpec6, dra7xx, imx6, keembay, ks, ls,
qcom, rcar_gen4, etc) use the dw_pcie_ep_init_notify() wrapper.

ls and qcom even use *both*: pci_epc_linkdown() but
dw_pcie_ep_linkup().

Personally I would drop the dw_*() wrappers.  It's a bigger patch but
not any more complicated, and the result is consistency across both
DWC and the non-DWC drivers.

I don't know if we need to add stubs for *all* the functions.  I'd
probably defer that until we trip over them.

Bjorn