Re: [PATCH v5 3/5] PCI: endpoint: pci-epf-test: Add doorbell test support

[Niklas Cassel <cassel@kernel.org>]

On Fri, Nov 08, 2024 at 02:43:30PM -0500, Frank Li wrote:

(snip)

> +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;
> +	struct msi_msg *msg;
> +	u64 doorbell_addr;
> +	u32 align;
> +	int ret;
> +
> +	align = epf_test->epc_features->align;
> +	align = align ? align : 128;
> +
> +	if (epf_test->epc_features->bar[bar].type == BAR_FIXED)
> +		align = max(epf_test->epc_features->bar[bar].fixed_size, align);
> +
> +	if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> +		reg->status |= STATUS_DOORBELL_ENABLE_FAIL;
> +		return;
> +	}
> +
> +	msg = &epf->db_msg[0].msg;
> +	doorbell_addr = msg->address_hi;
> +	doorbell_addr <<= 32;
> +	doorbell_addr |= msg->address_lo;
> +
> +	db_bar.phys_addr = round_down(doorbell_addr, align);
> +	db_bar.barno = bar;
> +	db_bar.size = epf->bar[bar].size;
> +	db_bar.flags = epf->bar[bar].flags;
> +	db_bar.addr = NULL;

Some of the code above ...

> +
> +	ret = pci_epc_set_bar(epc, epf->func_no, epf->vfunc_no, &db_bar);
> +	if (!ret)
> +		reg->status |= STATUS_DOORBELL_ENABLE_SUCCESS;
> +}
> +



> @@ -909,12 +998,46 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> +	ret = pci_epf_alloc_doorbell(epf, 1);
> +	if (!ret) {
> +		struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +		struct msi_msg *msg = &epf->db_msg[0].msg;
> +		u32 align = epc_features->align;
> +		u64 doorbell_addr;
> +		enum pci_barno bar;
> +
> +		bar = pci_epc_get_next_free_bar(epc_features, test_reg_bar + 1);
> +
> +		ret = request_irq(epf->db_msg[0].virq, pci_epf_test_doorbell_handler, 0,
> +				  "pci-test-doorbell", epf_test);
> +		if (ret) {
> +			dev_err(&epf->dev,
> +				"Failed to request irq %d, doorbell feature is not supported\n",
> +				epf->db_msg[0].virq);
> +			return 0;
> +		}
> +
> +		align = align ? align : 128;
> +
> +		if (epf_test->epc_features->bar[bar].type == BAR_FIXED)
> +			align = max(epf_test->epc_features->bar[bar].fixed_size, align);
> +
> +		doorbell_addr = msg->address_hi;
> +		doorbell_addr <<= 32;
> +		doorbell_addr |= msg->address_lo;
> +
> +		reg->doorbell_addr = doorbell_addr & (align - 1);
> +		reg->doorbell_data = msg->data;
> +		reg->doorbell_bar = bar;

... seems to be duplicated in this function.

Perhaps create a helper function so that you don't need to duplicate it.

Also one function is doing:
reg->doorbell_addr = doorbell_addr & (align - 1);

to align, the other one is doing:
round_down(doorbell_addr, align);

Which seems to be a bit inconsistent.
Anyway, if you move this to a helper, they will both use the same
way to align the address.
(May I suggest that you use ALIGN_DOWN() in the helper.)


Kind regards,
Niklas