[PATCH] PCI: tegra194: Add check for host and endpoint modes

[PATCH] PCI: tegra194: Add check for host and endpoint modes

[Manivannan Sadhasivam]

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);
+
 	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,
-- 
2.25.1

Re: [PATCH] PCI: tegra194: Add check for host and endpoint modes

[Niklas Cassel]

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

Re: [PATCH] PCI: tegra194: Add check for host and endpoint modes

[Niklas Cassel]

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