| Message ID | 20240513154900.127612-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: tegra194: Add check for host and endpoint modes | expand |
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@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@intel.com > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > 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()) Kind regards, Niklas
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@intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@intel.com > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > 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@linaro.org> 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@intel.com/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
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); + dw_pcie_ep_cleanup(&pcie->pci.ep); reset_control_assert(pcie->core_rst); @@ -2245,6 +2254,11 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) switch (pcie->of_data->mode) { case DW_PCIE_RC_TYPE: + if (!IS_ENABLED(CONFIG_PCIE_TEGRA194_HOST)) { + ret = -ENODEV; + goto fail; + } + ret = devm_request_irq(dev, pp->irq, tegra_pcie_rp_irq_handler, IRQF_SHARED, "tegra-pcie-intr", pcie); if (ret) { @@ -2261,6 +2275,11 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) break; case DW_PCIE_EP_TYPE: + if (!IS_ENABLED(CONFIG_PCIE_TEGRA194_EP)) { + ret = -ENODEV; + goto fail; + } + ret = devm_request_threaded_irq(dev, pp->irq, tegra_pcie_ep_hard_irq, tegra_pcie_ep_irq_thread,
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@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@intel.com Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-tegra194.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)