From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f170.google.com (mail-pf1-f170.google.com [209.85.210.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 80B3123A9 for ; Tue, 20 Aug 2024 05:52:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724133138; cv=none; b=Ec1rrOlZIf3f79PbyRlFR736KFYowXwUBIB1KD9KCabHWrLGElMYfTX2RIioN6RQ6USYJsWpW7wlXAm/UknMQoyc37P1wTNw2mBjpevY+pIk5F2VaqrR/P24pG31NpTYBxlhVS0FU9QmkXcXN2HJ5Z6hiJGD4IPjrzclikxdd3Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724133138; c=relaxed/simple; bh=DLK5ItQpzxghqBx4flWmDJes7nJFtU1awddB31t1t/g=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Z3LqHIbAd+RFf3GpIUIb3ZgdcysmsI39rMlKXlH8PpiF+veJyIMzfu6eRbrkLjUT2EOwqfSwmaRqqSkqdBtAB9B4YRvXfx93giroDx45SLJofsh+VUNFzybRYf433qqJyjjp1+0nm5nsrM0hMBLpqF4khEShOhi0CgIXCoaBkPM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=TilFGu0W; arc=none smtp.client-ip=209.85.210.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="TilFGu0W" Received: by mail-pf1-f170.google.com with SMTP id d2e1a72fcca58-70d199fb3dfso4213332b3a.3 for ; Mon, 19 Aug 2024 22:52:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1724133136; x=1724737936; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=boJ5GhilqsNI0J+v1vrM4G45uO2RgryBQBYvfmNmTfE=; b=TilFGu0WCCzRLw4uqCpcjYTiQQnlv3rY+YsTXrIYWSk8sthig2wzwIX1Xs3rtOrXOr 3msEFRKBCDYyALNbG4pMkKHSh83c1lE/dCah2avh22j7/pdFMt4tKVVLOY+q6Z6lZdm6 fllTCVK4d1KwYB7iMYMuLh6AhytjXXwRN1ES//TTPf36IOwQVhPvPOB25MaW0W16Ks0g dnt2wQ5n6P2yH2cueA4nixhvmnxgyfaGFNr0ONO1vFa2GhJ0FYtdMH9lcmXTz8mujMc1 lDqIxZ6jiTKg6Zj1HP859reM2TFNtQqOsGfc6z0PveqPW4yO4qVMwUYspBPskYOSYBjx z5Pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724133136; x=1724737936; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=boJ5GhilqsNI0J+v1vrM4G45uO2RgryBQBYvfmNmTfE=; b=MohvpkYvGYyVTAmZvjPeGSA2YDQe1kU/pExuSDizBEZVBewQ+k8GgYMbjEW8HnqLY5 yFzbhQfB5t4GiNRAxG2cC612uj3Dq5bA5YNknabypuLPftMxhyzl9hSp4KuGx00zJXjH uq9Z4n9tiFGlzA8+jIate1j7qm+3QtAjR67Pb6WR6mP1QSwQ/666QBqbcj7bUeamcavf xHCAXgz5Y7a4V8J29cM5KpX7kg9/uMbM7SgLy4xoXFB8bLwKS7XoGzjsljA0Tf6qC4Ze ePDELETP1S5i7WThB0KJhGgS63CkzHHu9M6I0SaY7ata8/qTqleE47H65p79zAAHa1P/ mJpg== X-Forwarded-Encrypted: i=1; AJvYcCUuNqHoyP2P3Fv1zPSnQeKyiGGcQiHxlOqzhgY032zBBdH+xvG0fdjL0g86t+RHQ5b+e2sv6eiMINbceiY=@vger.kernel.org X-Gm-Message-State: AOJu0YzcLmLnURcJxjyyuN1/fGXCXgLWPHS7FJmEAsE+4/Uxfu+/ACDp /mxh92V42FstLUk4Jr86vCDmLWgmkrCAmRqb9x6H7ZaHHTfkzDzHpM4L33BCQA== X-Google-Smtp-Source: AGHT+IExg7cB/Xy0vOL//UAkuMnPPxBv49gmpCMa8JbsqU5untaUwshPv3OFJbbDot/5OFyk9/K5YA== X-Received: by 2002:a05:6a20:2d22:b0:1bd:1e06:9db4 with SMTP id adf61e73a8af0-1c904f88a2emr17776636637.19.1724133135685; Mon, 19 Aug 2024 22:52:15 -0700 (PDT) Received: from thinkpad ([120.60.128.138]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-201f02faa03sm71137595ad.19.2024.08.19.22.52.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 19 Aug 2024 22:52:15 -0700 (PDT) Date: Tue, 20 Aug 2024 11:22:08 +0530 From: Manivannan Sadhasivam To: Rick Wertenbroek Cc: rick.wertenbroek@heig-vd.ch, dlemoal@kernel.org, alberto.dassatti@heig-vd.ch, Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Kishon Vijay Abraham I , Bjorn Helgaas , Niklas Cassel , Frank Li , Lars-Peter Clausen , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] PCI: endpoint: pci-epf-test: Move DMA check into read/write/copy functions Message-ID: <20240820055208.g6iorjl4uhl2jq45@thinkpad> References: <20240819120112.23563-1-rick.wertenbroek@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240819120112.23563-1-rick.wertenbroek@gmail.com> On Mon, Aug 19, 2024 at 02:01:10PM +0200, Rick Wertenbroek wrote: > The pci-epf-test PCI endpoint function /drivers/pci/endpoint/function/pci-epf_test.c > is meant to be used in a PCI endpoint device inside a host computer with > the host side driver: /drivers/misc/pci_endpoint_test.c. > s/inside/connected to > The host side driver can request read/write/copy transactions from the > endpoint function and expects an IRQ from the endpoint function once > the read/write/copy transaction is finished. These can be issued with or > without DMA enabled. If the host side driver requests a read/write/copy > transaction with DMA enabled and the endpoint function does not support > DMA, the endpoint would only print an error message and wait for further > commands without sending an IRQ because pci_epf_test_raise_irq() is > skipped in pci_epf_test_cmd_handler(). This results in the host side > driver hanging indefinitely waiting for the IRQ. > TBH, it doesn't make sense to control the endpoint DMA from host. Host should just issue the transfer command, and let the endpoint use DMA or memcpy based on its capability. > Move the DMA check into the pci_epf_test_read()/write()/copy() functions > so that they report a transfer (IO) error and that pci_epf_test_raise_irq() > is called when a transfer with DMA is requested, even if unsupported. > > The host side driver will no longer hang but report an error on transfer > (printing "NOT OKAY") thanks to the checksum because no data was moved. > > Signed-off-by: Rick Wertenbroek > --- > drivers/pci/endpoint/functions/pci-epf-test.c | 29 +++++++++++++++---- > 1 file changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > index 7c2ed6eae53a..ec0f79383521 100644 > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > @@ -314,6 +314,17 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test, > (u64)ts.tv_sec, (u32)ts.tv_nsec, rate); > } > > +static int pci_epf_test_check_dma(struct pci_epf_test *epf_test, > + struct pci_epf_test_reg *reg) > +{ > + if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) && > + !epf_test->dma_supported) { > + dev_err(&epf_test->epf->dev, "DMA transfer not supported\n"); > + return -EIO; > + } > + return 0; > +} > + > static void pci_epf_test_copy(struct pci_epf_test *epf_test, > struct pci_epf_test_reg *reg) > { > @@ -327,6 +338,10 @@ static void pci_epf_test_copy(struct pci_epf_test *epf_test, > struct device *dev = &epf->dev; > struct pci_epc *epc = epf->epc; > > + ret = pci_epf_test_check_dma(epf_test, reg); > + if (ret) > + goto err; > + > src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size); > if (!src_addr) { > dev_err(dev, "Failed to allocate source address\n"); > @@ -423,6 +438,10 @@ static void pci_epf_test_read(struct pci_epf_test *epf_test, > struct pci_epc *epc = epf->epc; > struct device *dma_dev = epf->epc->dev.parent; > > + ret = pci_epf_test_check_dma(epf_test, reg); > + if (ret) > + goto err; > + > src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size); > if (!src_addr) { > dev_err(dev, "Failed to allocate address\n"); > @@ -507,6 +526,10 @@ static void pci_epf_test_write(struct pci_epf_test *epf_test, > struct pci_epc *epc = epf->epc; > struct device *dma_dev = epf->epc->dev.parent; > > + ret = pci_epf_test_check_dma(epf_test, reg); > + if (ret) > + goto err; > + > dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size); > if (!dst_addr) { > dev_err(dev, "Failed to allocate address\n"); > @@ -647,12 +670,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work) > WRITE_ONCE(reg->command, 0); > WRITE_ONCE(reg->status, 0); > > - if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) && > - !epf_test->dma_supported) { > - dev_err(dev, "Cannot transfer data using DMA\n"); > - goto reset_handler; Why can't you just set the status and trigger the IRQ here itself? This avoids duplicating the check inside each command handler. - Mani -- மணிவண்ணன் சதாசிவம்