Received-SPF: pass (google.com: domain of linux-kernel+bounces-111756-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 22 Mar 2024 17:10:06 +0100
From: Niklas Cassel <cassel@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, mhi@lists.linux.dev,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to
 EPC init callback
Message-ID: <Zf2tXgKo-gc3qy1D@ryzen>
References: <20240314-pci-epf-rework-v1-0-6134e6c1d491@linaro.org>
 <20240314-pci-epf-rework-v1-5-6134e6c1d491@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240314-pci-epf-rework-v1-5-6134e6c1d491@linaro.org>

On Thu, Mar 14, 2024 at 08:53:44PM +0530, Manivannan Sadhasivam wrote:
> To maintain uniformity across EPF drivers, let's move the DMA
> initialization to EPC init callback. This will also allow us to deinit DMA
> during PERST# assert in the further commits.
> 
> For EPC drivers without PERST#, DMA deinit will only happen during driver
> unbind.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>


For the record, I was debugging a problem related to EPF DMA recently
and was dumping the DMA mask for the struct device of the epf driver.
I was a bit confused to see it as 32-bits, even though the EPC driver
has it set to 64-bits.

The current code works, because e.g., pci_epf_test_write(), etc,
does:
struct device *dma_dev = epf->epc->dev.parent;
dma_map_single(dma_dev, ...);

but it also means that all EPF drivers will do this uglyness.


However, if a EPF driver does e.g.
dma_alloc_coherent(), and sends in the struct *device for the EPF,
which is the most logical thing to do IMO, it will use the wrong DMA
mask.

Perhaps EPF or EPC code should make sure that the struct *device
for the EPF will get the same DMA mask as epf->epc->dev.parent,
so that EPF driver developer can use the struct *epf when calling
e.g. dma_alloc_coherent().

(This is however not strictly related to this patch, but I thought
that I should mention it.)


Kind regards,
Niklas

>  drivers/pci/endpoint/functions/pci-epf-mhi.c  | 16 ++++++++--------
>  drivers/pci/endpoint/functions/pci-epf-test.c | 12 ++++++------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index da894a9a447e..4e4300efd9d7 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -737,6 +737,14 @@ static int pci_epf_mhi_epc_init(struct pci_epf *epf)
>  	if (!epf_mhi->epc_features)
>  		return -ENODATA;
>  
> +	if (info->flags & MHI_EPF_USE_DMA) {
> +		ret = pci_epf_mhi_dma_init(epf_mhi);
> +		if (ret) {
> +			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -749,14 +757,6 @@ static int pci_epf_mhi_link_up(struct pci_epf *epf)
>  	struct device *dev = &epf->dev;
>  	int ret;
>  
> -	if (info->flags & MHI_EPF_USE_DMA) {
> -		ret = pci_epf_mhi_dma_init(epf_mhi);
> -		if (ret) {
> -			dev_err(dev, "Failed to initialize DMA: %d\n", ret);
> -			return ret;
> -		}
> -	}
> -
>  	mhi_cntrl->mmio = epf_mhi->mmio;
>  	mhi_cntrl->irq = epf_mhi->irq;
>  	mhi_cntrl->mru = info->mru;
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 2fac36553633..8f1e0cb08814 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -753,6 +753,12 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  	bool msi_capable = true;
>  	int ret;
>  
> +	epf_test->dma_supported = true;
> +
> +	ret = pci_epf_test_init_dma_chan(epf_test);
> +	if (ret)
> +		epf_test->dma_supported = false;
> +
>  	epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
>  	if (epc_features) {
>  		msix_capable = epc_features->msix_capable;
> @@ -942,12 +948,6 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> -	epf_test->dma_supported = true;
> -
> -	ret = pci_epf_test_init_dma_chan(epf_test);
> -	if (ret)
> -		epf_test->dma_supported = false;
> -
>  	return 0;
>  }
>  
> 
> -- 
> 2.25.1
>