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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

Hi Niklas,

On Wed, May 29, 2024 at 09:35:44AM +0200, Niklas Cassel wrote:
> On Tue, May 28, 2024 at 02:55:39PM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 09:17:40PM +0200, Niklas Cassel wrote:
> > > 
> > > > 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.
> 
> Hello Mani,
> 
> considering that:
> 
> 1) Bjorn dropped the commit:
> "PCI: endpoint: Introduce 'epc_deinit' event and notify the EPF drivers"
> which means that you will need resubmit your patch.
> 
> 2) Any changes I would do would conflict with your patch.
> (It probably makes most sense put your patch as the final patch in a series.)
> 
> 3) You are the PCI endpoint maintainer, so you are most suited to decide
> which functions to stub (if any).
> 
> 4) Your patch only affects tegra and qcom, and I don't have the hardware
> for either, so I wouldn't be able to test.
> 
> Thus, I do not intend to respin this series.
> I hope that is okay with you.
> 

That's fine. Thanks a lot for stepping in to fix the build issue. I was on
vacation, so couldn't act on your query/series promptly.

Let us conclude the fix here itself as we have more than 1 threads going on.
I did consider adding the stubs to pci-epc.h, but only the deinit API requires
that. So I thought it will look odd to add stub for only one function, that too
for one of the two variants (init/deinit).

So I went ahead with the ugly (yes) conditional for the deinit_notify API.

Ideally, I would've expected both dwc and EP subsystem to provide stubs for the
APIs used by the common driver (host and EP). But since the controller drivers
were using the conditional check to differentiate between host and EP mode,
compilers were smart enough to spot the dead functions and removes them. So
there were no reports so far.

But in this case, the pci_epc_deinit_notify() is called in a separate helper and
hence the issue.

So to conclude, I think it is best if we can add stub just for
pci_epc_deinit_notify() in pci-epc.h and get rid of the dummy
dw_pcie_ep_init_notify() wrapper to make the init/deinit API usage consistent.

Also I do not want to remove the wrapper for dw_pcie_ep_linkup() since its
conterpart dw_pcie_ep_linkdown() is required.

Let me know what you think! I will submit a new series with the left over
patches.

- Mani

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