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;@@ -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