Received-SPF: pass (google.com: domain of linux-kernel+bounces-191845-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 28 May 2024 09:11:59 +0200
From: Niklas Cassel <cassel@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: lpieralisi@kernel.org, kw@linux.com, bhelgaas@google.com,
	robh@kernel.org, thierry.reding@gmail.com, jonathanh@nvidia.com,
	vidyas@nvidia.com, linux-pci@vger.kernel.org,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel test robot <lkp@intel.com>,
	Damien Le Moal <dlemoal@kernel.org>
Subject: Re: [PATCH] PCI: tegra194: Add check for host and endpoint modes
Message-ID: <ZlWDv6k1AwUbAKmr@ryzen.lan>
References: <20240513154900.127612-1-manivannan.sadhasivam@linaro.org>
 <Zky0XkvmOoFCxnDx@x1-carbon.wireless.wdc>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zky0XkvmOoFCxnDx@x1-carbon.wireless.wdc>

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