| 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
| ||
|
Message-ID: <357zznvglssmaemq4j3v3s4atrkljq3o6ivx35h3ztw64iml3d@hauozlcoaaog>
Date: Wed, 2 Jul 2025 18:36:52 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Frank Li <Frank.Li@....com>
Cc: Kishon Vijay Abraham I <kishon@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
Anup Patel <apatel@...tanamicro.com>, Marc Zyngier <maz@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>, Shuah Khan <shuah@...nel.org>,
Richard Zhu <hongxing.zhu@....com>, Lucas Stach <l.stach@...gutronix.de>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Rob Herring <robh@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>,
Niklas Cassel <cassel@...nel.org>, dlemoal@...nel.org, jdmason@...zu.us,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
linux-kselftest@...r.kernel.org, imx@...ts.linux.dev, devicetree@...r.kernel.org
Subject: Re: [PATCH v19 05/10] PCI: endpoint: pci-epf-test: Add doorbell test
support
On Mon, Jun 09, 2025 at 12:34:17PM GMT, Frank Li wrote:
[...]
> +static irqreturn_t pci_epf_test_doorbell_handler(int irq, void *data)
> +{
> + struct pci_epf_test *epf_test = data;
> + enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> + struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> + u32 status = le32_to_cpu(reg->status);
> +
> + status |= STATUS_DOORBELL_SUCCESS;
> + reg->status = cpu_to_le32(status);
> + pci_epf_test_raise_irq(epf_test, reg);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void pci_epf_test_doorbell_cleanup(struct pci_epf_test *epf_test)
> +{
> + struct pci_epf_test_reg *reg = epf_test->reg[epf_test->test_reg_bar];
> + struct pci_epf *epf = epf_test->epf;
> +
> + if (le32_to_cpu(reg->doorbell_bar) > 0) {
Is this check necessary?
> + free_irq(epf->db_msg[0].virq, epf_test);
> + reg->doorbell_bar = cpu_to_le32(NO_BAR);
> + }
> +
> + if (epf->db_msg)
Same here.
> + pci_epf_free_doorbell(epf);
> +}
> +
> +static void pci_epf_test_enable_doorbell(struct pci_epf_test *epf_test,
> + struct pci_epf_test_reg *reg)
> +{
> + u32 status = le32_to_cpu(reg->status);
> + struct pci_epf *epf = epf_test->epf;
> + struct pci_epc *epc = epf->epc;
> + struct msi_msg *msg;
> + enum pci_barno bar;
> + size_t offset;
> + int ret;
> +
> + ret = pci_epf_alloc_doorbell(epf, 1);
> + if (ret) {
> + status |= STATUS_DOORBELL_ENABLE_FAIL;
> + goto set_status;
I think you can just set the failure status directly in err path:
if (ret)
goto set_err_status;
> + }
> +
> + msg = &epf->db_msg[0].msg;
> + bar = pci_epc_get_next_free_bar(epf_test->epc_features, epf_test->test_reg_bar + 1);
> + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
Can 'bar' really be 'epf_test->test_reg_bar' here? You just need to check for
NO_BAR, that's it.
Also 'epf->db_msg' can't be NULL here. You were already dereferencing above, so
this check is pointless.
> + status |= STATUS_DOORBELL_ENABLE_FAIL;
> + goto set_status;
> + }
> +
> + ret = request_irq(epf->db_msg[0].virq, pci_epf_test_doorbell_handler, 0,
> + "pci-test-doorbell", epf_test);
'pci-ep-test-doorbell'
> + if (ret) {
> + dev_err(&epf->dev,
> + "Failed to request irq %d, doorbell feature is not supported\n",
'Failed to request doorbell IRQ: %d\n'
> + epf->db_msg[0].virq);
> + status |= STATUS_DOORBELL_ENABLE_FAIL;
> + pci_epf_test_doorbell_cleanup(epf_test);
this can be moved to a err label:
goto cleanup_doorbell;
> + goto set_status;
> + }
> +
> + reg->doorbell_data = cpu_to_le32(msg->data);
> + reg->doorbell_bar = cpu_to_le32(bar);
> +
> + msg = &epf->db_msg[0].msg;
> + ret = pci_epf_align_inbound_addr(epf, bar, ((u64)msg->address_hi << 32) | msg->address_lo,
> + &epf_test->db_bar.phys_addr, &offset);
> +
> + if (ret) {
> + status |= STATUS_DOORBELL_ENABLE_FAIL;
> + pci_epf_test_doorbell_cleanup(epf_test);
> + goto set_status;
> + }
> +
> + reg->doorbell_offset = cpu_to_le32(offset);
> +
> + epf_test->db_bar.barno = bar;
> + epf_test->db_bar.size = epf->bar[bar].size;
> + epf_test->db_bar.flags = epf->bar[bar].flags;
> +
> + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &epf_test->db_bar);
> + if (ret) {
> + status |= STATUS_DOORBELL_ENABLE_FAIL;
> + pci_epf_test_doorbell_cleanup(epf_test);
> + } else {
> + status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> + }
> +
Set the success status directly here:
status |= STATUS_DOORBELL_ENABLE_SUCCESS;
reg->status = cpu_to_le32(status);
return;
cleanup_doorbell:
pci_epf_test_doorbell_cleanup(epf_test);
set_err_status:
status |= STATUS_DOORBELL_ENABLE_FAIL;
reg->status = cpu_to_le32(status);
> +set_status:
> + reg->status = cpu_to_le32(status);
> +}
> +
> +static void pci_epf_test_disable_doorbell(struct pci_epf_test *epf_test,
> + struct pci_epf_test_reg *reg)
> +{
> + enum pci_barno bar = le32_to_cpu(reg->doorbell_bar);
> + u32 status = le32_to_cpu(reg->status);
> + struct pci_epf *epf = epf_test->epf;
> + struct pci_epc *epc = epf->epc;
> + int ret;
> +
> + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
Same comment about as above for these checks.
> + status |= STATUS_DOORBELL_DISABLE_FAIL;
> + goto set_status;
> + }
> +
> + ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &epf->bar[bar]);
> + if (ret)
> + status |= STATUS_DOORBELL_DISABLE_FAIL;
> + else
> + status |= STATUS_DOORBELL_DISABLE_SUCCESS;
> +
> + pci_epf_test_doorbell_cleanup(epf_test);
> +
> +set_status:
> + reg->status = cpu_to_le32(status);
> +}
> +
> static void pci_epf_test_cmd_handler(struct work_struct *work)
> {
> u32 command;
> @@ -714,6 +847,14 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
> pci_epf_test_copy(epf_test, reg);
> pci_epf_test_raise_irq(epf_test, reg);
> break;
> + case COMMAND_ENABLE_DOORBELL:
> + pci_epf_test_enable_doorbell(epf_test, reg);
> + pci_epf_test_raise_irq(epf_test, reg);
> + break;
> + case COMMAND_DISABLE_DOORBELL:
> + pci_epf_test_disable_doorbell(epf_test, reg);
> + pci_epf_test_raise_irq(epf_test, reg);
> + break;
> default:
> dev_err(dev, "Invalid command 0x%x\n", command);
> break;
> @@ -987,6 +1128,7 @@ static void pci_epf_test_unbind(struct pci_epf *epf)
> pci_epf_test_clean_dma_chan(epf_test);
> pci_epf_test_clear_bar(epf);
> }
> + pci_epf_test_doorbell_cleanup(epf_test);
Why is this necessary?
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists