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

[Niklas Cassel <cassel@kernel.org>]

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 :)


Kind regards,
Niklas