From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) (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 233B21BF7E8 for ; Tue, 26 Nov 2024 12:41:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732624881; cv=none; b=riSYMbZSAeQ3FmJFv7EbzcXamu6ph79kAYpImU3e4RdHr7xbOzgdGpWuMIJ72N3quFixhU/QqaZIRt4UPpj+T5FEnvgclBEZ0QQG6Yp2JtV31Jy7FU9AnmDJOkIiv4KOLUDe++Ly0v9b8ZZc5xwGRuFojbbJXv04eKhi6hgNcFs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732624881; c=relaxed/simple; bh=5QGqLgj7ult2KpL1mhiCaa8OlPSUKLYv0B9W4FZ6P0Y=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tFzxtKBXgp03WooM1k3yIMW5EKcSNR3lEpCgBNDNMZQTfgxLKIADpWh8BikBNH4UpUOd67QstIuQoWY4/DD9+GXZRhlz84XrvRZC6tLgP3PQHanP3v2lOcUtJb/f121XaBdp5+kRVNGrYvfhLfxGd6nKSYG86lqqsjH8XnC7GwE= 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=HylJWy6B; arc=none smtp.client-ip=209.85.214.174 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="HylJWy6B" Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-21290973bcbso53107365ad.3 for ; Tue, 26 Nov 2024 04:41:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1732624878; x=1733229678; darn=lists.linux.dev; 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=MxF+aV1Hv4QY3oDIZgKkkm4GIfDo73DrZkwY5UF6N5I=; b=HylJWy6BPSO8PaI0K7tCpzTX7QcV5WPiHSKkkFD+eR7Q9BmJnufN6yRHu6wIoivzpB GhGUSMtH2H2JM8HAFpMSNgmpNdk0WneSIg1HSMs8EVoh7EDMA49mPFZCZ8QYIb5bWTe7 pd4GLm9sCX8BjXA84D4/B0R41z4j8ig9ZHzVU9dqI2nmXD7rcnzMyr9f7JaniRXiFqVt WoOmc5TfiD+nPUWoVs8eDBuDCCj9T+D93Cv6n7MuX/gp+UJj5TJu872IONmkyhq8bi65 VwrbOVE0snQQc0jH55FXfSbxhtIDjF82bjT14tGNlxCD1fl43FT6Ps0rfaMroXmb6vTU ybvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732624878; x=1733229678; 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=MxF+aV1Hv4QY3oDIZgKkkm4GIfDo73DrZkwY5UF6N5I=; b=UAxl9xewwg73n1MMv9N5qwY50HJyTimrh96FsmSnVuT7FmK/ESLKjU33zHuZqOzaOB qhiCpusqJYwUgd/zXjaT3qopfoy7ZYVvDKLBfRqvelEwXfA8lioGVF/eFFFT+WJptz7X KcC8+KjeoKusLpNMSLqKWLvI7oA4i+E7lmMqE3iblJCr8NGK566i86pZyajxw069PvY/ sg86E9MjSkkhZLeIf6pC6lNyezFuCT5gtbLLNMrG4TZ8koZKNyo6yNdWlx152G30bkvV fjkmhd3nlLQYZVqSrQnpG1RojpD8reBBm8dNxp5L0rxLAswaEnpdTUO16YlduP1VPVJi wmTg== X-Forwarded-Encrypted: i=1; AJvYcCX9Y5UM3nA28pcK7x5qCAzjIEGP6pXJemxN8npINH7IIrEabqDH1+1A78gSRXMG/TX8fOs=@lists.linux.dev X-Gm-Message-State: AOJu0YzuedCE8VU4Qy+WeZmfrnVmrIaPJ2X3KKakkLBz6T4T3Z9DwEfK hJsvQQ+hz411VSfrJSwj0KJM0QI3bve+h9+bCpiW7RyKWbnZD/hPVEKbeoZY5PbZXHBLCWQbn8Q = X-Gm-Gg: ASbGncure2gq+HW6alXZfuWXrgtKr2357G0k8hgIGYEK77d3VBRnDczOLsnxZznz/uP QbnClgpy3nPANzBAj9BsId+C63DJRroEZ2eeDUFT3B5SaLy26TE0W3NU3lZzzuHjJAkIMdH5697 fYEa9sZZEKkT0UBJ3a97fi54cYE52an+q1ECxX7UgM/x6buQgA01uhhXaLZJylD5N5Xje+V8HMt ulEEsooRkI+7f9zgWPPpRr/qaMExAHe6EmbviafNDqX4mGgfB/TxNnCVQ0q X-Google-Smtp-Source: AGHT+IHNkV0w1MtKgqBXdudpD0c62ethWhe9CLuXm06H3Scg2apzmGTe2RkEFVHEhLbz5G18I7I10Q== X-Received: by 2002:a17:902:dac5:b0:212:500:74e with SMTP id d9443c01a7336-2129f51d2c6mr233343125ad.11.1732624878328; Tue, 26 Nov 2024 04:41:18 -0800 (PST) Received: from thinkpad ([120.60.136.64]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2129dc22237sm83758345ad.243.2024.11.26.04.41.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Nov 2024 04:41:17 -0800 (PST) Date: Tue, 26 Nov 2024 18:11:12 +0530 From: Manivannan Sadhasivam To: Niklas Cassel Cc: Frank Li , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Kishon Vijay Abraham I , Bjorn Helgaas , Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, imx@lists.linux.dev, dlemoal@kernel.org, maz@kernel.org, tglx@linutronix.de, jdmason@kudzu.us Subject: Re: [PATCH v8 4/6] PCI: endpoint: pci-epf-test: Add doorbell test support Message-ID: <20241126124112.5o4c3lzem72lkvdw@thinkpad> References: <20241116-ep-msi-v8-0-6f1f68ffd1bb@nxp.com> <20241116-ep-msi-v8-4-6f1f68ffd1bb@nxp.com> <20241124075645.szue5nzm4gcjspxf@thinkpad> <20241126042523.6qlmhkjfl5kwouth@thinkpad> Precedence: bulk X-Mailing-List: imx@lists.linux.dev 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: On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote: > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote: > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote: > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote: > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote: > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data, > > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a > > > > > > > > I don't see 'doorbell_done' defined anywhere. > > > > > > > > > doorbell address space. > > > > > > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell > > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's > > > > > address space. > > > > > > > > > > Set doorbell_done in the doorbell callback to indicate completion. > > > > > > > > > > > > > Same here. > > > > > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL > > > > > > > > 'avoid breaking compatibility between host and endpoint,...' > > > > > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL > > > > > to map one bar's inbound address to MSI space. the command > > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping. > > > > > > > > > > Host side new driver Host side old driver > > > > > > > > > > EP: new driver S F > > > > > EP: old driver F F > > > > > > > > So the last case of old EP and host drivers will fail? > > > > > > doorbell test will fail if old EP. > > > > > > > How come there would be doorbell test if it is an old host driver? > > I also don't understand this. > > The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE > can only be sent if there is a new host driver. > > Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously > return "Invalid command" if the EP driver is old. > > If EP driver is new, DOORBELL_ENABLE will only return success if the SoC > has support for GIC ITS and has configured DTS with msi-parent > (i.e. if the pci_epf_alloc_doorbell() call was successful). > > > > > > > > > > > > > > > > > > S: If EP side support MSI, 'pcitest -B' return success. > > > > > If EP side doesn't support MSI, the same to 'F'. > > > > > > > > > > F: 'pcitest -B' return failure, other case as usual. > > > > > > > > > > Tested-by: Niklas Cassel > > > > > Signed-off-by: Frank Li > > > > > --- > > > > > Change from v7 to v8 > > > > > - rename to pci_epf_align_inbound_addr_lo_hi() > > > > > > > > > > Change from v6 to v7 > > > > > - use help function pci_epf_align_addr_lo_hi() > > > > > > > > > > Change from v5 to v6 > > > > > - rename doorbell_addr to doorbell_offset > > > > > > > > > > Chagne from v4 to v5 > > > > > - Add doorbell free at unbind function. > > > > > - Move msi irq handler to here to more complex user case, such as differece > > > > > doorbell can use difference handler function. > > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off > > > > > tag or co-developer tag, please let me know. > > > > > > > > > > change from v3 to v4 > > > > > - remove revid requirement > > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL. > > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at > > > > > COMMAND_ENABLE_DOORBELL. > > > > > --- > > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++ > > > > > 1 file changed, 117 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > index ef6677f34116e..410b2f4bb7ce7 100644 > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c > > > > > @@ -11,12 +11,14 @@ > > > > > #include > > > > > #include > > > > > #include > > > > > +#include > > > > > #include > > > > > #include > > > > > #include > > > > > > > > > > #include > > > > > #include > > > > > +#include > > > > > #include > > > > > > > > > > #define IRQ_TYPE_INTX 0 > > > > > @@ -29,6 +31,8 @@ > > > > > #define COMMAND_READ BIT(3) > > > > > #define COMMAND_WRITE BIT(4) > > > > > #define COMMAND_COPY BIT(5) > > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6) > > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7) > > > > > > > > > > #define STATUS_READ_SUCCESS BIT(0) > > > > > #define STATUS_READ_FAIL BIT(1) > > > > > @@ -39,6 +43,11 @@ > > > > > #define STATUS_IRQ_RAISED BIT(6) > > > > > #define STATUS_SRC_ADDR_INVALID BIT(7) > > > > > #define STATUS_DST_ADDR_INVALID BIT(8) > > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9) > > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10) > > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11) > > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12) > > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13) > > > > > > > > > > #define FLAG_USE_DMA BIT(0) > > > > > > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg { > > > > > u32 irq_type; > > > > > u32 irq_number; > > > > > u32 flags; > > > > > + u32 doorbell_bar; > > > > > + u32 doorbell_offset; > > > > > + u32 doorbell_data; > > > > > } __packed; > > > > > > > > > > static struct pci_epf_header test_header = { > > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, > > > > > } > > > > > } > > > > > > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg) > > > > > +{ > > > > > + enum pci_barno bar = reg->doorbell_bar; > > > > > + struct pci_epf *epf = epf_test->epf; > > > > > + struct pci_epc *epc = epf->epc; > > > > > + struct pci_epf_bar db_bar; > > > > > > > > db_bar = {}; > > > > > > > > > + struct msi_msg *msg; > > > > > + size_t offset; > > > > > + int ret; > > > > > + > > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) { > > > > > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've > > > > allocated proper BAR already. > > > > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature. > > > > What is 'optional feature' here? allocating doorbell? > > > > > It return failure when it actually use it. > > > > > > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()? > > This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at > .bind() time. > > DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver > could theoretically send these even if pci_epf_alloc_doorbell() failed. > > > pci_epf_test_cmd_handler() additions looks like this: > > + case COMMAND_ENABLE_DOORBELL: > + pci_epf_enable_doorbell(epf_test, reg); > + pci_epf_test_raise_irq(epf_test, reg); > + break; > + case COMMAND_DISABLE_DOORBELL: > + pci_epf_disable_doorbell(epf_test, reg); > + pci_epf_test_raise_irq(epf_test, reg); > + break; > > so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell() > unconditionally, without any check to see if the doorbell was allocated. > > We could move the was doorbell allocated check (if (!epf->db_msg)) to > pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler() > more messy, so personally I think it is fine to keep the doorbell allocated > check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell(). > > > I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call > to pci_epf_enable_doorbell(): > https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/ > > His reply is here:: > https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/ > > "it may be too frequent to allocate and free msi resources when call > pci_epf_enable_doorbell()/pci_epf_disable_doorbell()." > > I don't think that is a good argument, as presumably (in the normal case) an > EPF driver will enable doorbell in the beginning, and then keep it enabled. > > However, one point could be that pci-epf-test currently does all allocations > (the allocations for the backing memory) in .bind(), so in one way it makes > sense to also allocate the doorbell in .bind(). > > To play devil's advocate, I guess you could argue that doorbell feature is > optional, while allocating backing memory for BARs is not, so it makes sense > that they are not allocated at the same time. > I like the idea of calling pci_epf_alloc_doorbell() in pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense to call these APIs too frequently. - Mani -- மணிவண்ணன் சதாசிவம்