[PATCH v19 05/10] PCI: endpoint: pci-epf-test: Add doorbell test support

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்