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
