On Tue, May 21, 2024 at 04:49:02PM +0200, Niklas Cassel wrote: > On Mon, May 13, 2024 at 05:49:00PM +0200, Manivannan Sadhasivam wrote: > > Tegra194 driver supports both the host and endpoint mode, but there are no > > checks to validate whether the corresponding mode is enabled in kernel > > config or not. So if the driver tries to function without enabling the > > required mode (CONFIG_PCIE_TEGRA194_HOST/CONFIG_PCIE_TEGRA194_EP), then it > > will result in driver malfunction. > > > > So let's add the checks in probe() before doing the mode specific config > > and fail probe() if the corresponding mode is not enabled. > > > > But this also requires adding one redundant check in > > pex_ep_event_pex_rst_assert() for pci_epc_deinit_notify(). Because the > > function is called outside of probe() and the compiler fails to spot the > > dependency in probe() and still complains about the undefined reference to > > pci_epc_deinit_notify(). > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@xxxxxxxxx > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > --- > > drivers/pci/controller/dwc/pcie-tegra194.c | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > index d2223821e122..e02a9bca70ef 100644 > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > @@ -1715,7 +1715,16 @@ static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie) > > if (ret) > > dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret); > > > > - pci_epc_deinit_notify(pcie->pci.ep.epc); > > + /* > > + * We do not really need the below guard as the driver won't probe > > + * successfully if it tries to probe in EP mode and > > + * CONFIG_PCIE_TEGRA194_EP is not enabled. But since this function is > > + * being called outside of probe(), compiler fails to spot the > > + * dependency in probe() and hence this redundant check. > > + */ > > + if (IS_ENABLED(CONFIG_PCIE_TEGRA194_EP)) > > + pci_epc_deinit_notify(pcie->pci.ep.epc); > > + > > This big comment is a bit ugly. It would be nice if it could be avoided. > > (pci-epc.h does not provide any dummy implementations for any of the > functions, so I suggest that we leave it like that.) > > However, if we look at dw_pcie_ep_init_notify(), it is called from > pex_ep_event_pex_rst_deassert(), and we do not get a build warning for that. > > The reason is that dw_pcie_ep_init_notify() has a dummy implementation > in pcie-designware.h. > > May I suggest that we add a dw_pcie_ep_deinit_notify() wrapper around > pci_epc_deinit_notify(), while also providing a dummy implementation > in pcie-designware.h ? > > That way, the code in pcie-tegra194.c (and pcie-qcom-ep.c) would be > more symmetrical, calling dw_pcie_ep_init_notify() for init notification, > and dw_pcie_ep_deinit_notify() for deinit notification. > > (Instead of dw_pcie_ep_init_notify() + pci_epc_deinit_notify()) Ping. The branch: pci/endpoint Which has commit: commit f94f2844f28c968364af8543414fbea9c8b3005d Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> Date: Tue Apr 30 11:43:48 2024 +0530 PCI: endpoint: Introduce 'epc_deinit' event and notify the EPF drivers still fails to link using certain arm64-randconfigs. See: https://lore.kernel.org/linux-pci/202405270544.yKgcokbA-lkp@xxxxxxxxx/T/#u The patch in $subject was meant to fix that, but it hasn't been merged yet. Considering that the pci/endpoint branch is currently broken, I think it should be high priority to get a patch in to fix this. (Otherwise the patches in pci/endpoint might get 'deferred' yet another release cycle.) Any thoughts on my review comments on this patch? Kind regards, Niklas
