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
--
மணிவண்ணன் சதாசிவம்
