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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zky0XkvmOoFCxnDx@x1-carbon.wireless.wdc>
Date: Tue, 21 May 2024 16:49:02 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
	robh@...nel.org, thierry.reding@...il.com, jonathanh@...dia.com,
	vidyas@...dia.com, linux-pci@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel test robot <lkp@...el.com>,
	Damien Le Moal <dlemoal@...nel.org>
Subject: Re: [PATCH] PCI: tegra194: Add check for host and endpoint modes

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@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202405130815.BwBrIepL-lkp@intel.com
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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