On Mon 19 May 2025 at 18:44, Frank Li <Frank.li@xxxxxxx> wrote:
> On Mon, May 05, 2025 at 07:41:50PM +0200, Jerome Brunet wrote:
>> The NTB epf host driver assumes the BAR number associated with a memory
>> window is just incremented from the BAR number associated with MW1. This
>> seems to have been enough so far but this is not really how the endpoint
>> side work and the two could easily become mis-aligned.
>>
>> ntb_epf_mw_to_bar() even assumes that the BAR number is the memory window
>> index + 2, which means the function only returns a proper result if BAR_2
>> is associated with MW1.
>>
>> Instead, fully describe and allow arbitrary NTB BAR mapping.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>> ---
>> drivers/ntb/hw/epf/ntb_hw_epf.c | 108 ++++++++++++++++++++--------------------
>> 1 file changed, 55 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c b/drivers/ntb/hw/epf/ntb_hw_epf.c
>> index 00f0e78f685bf7917b02dd8a52b5b35f68d5bb64..9539cdcd0f8fa4b5c5e66477672f8f97d5ec4e52 100644
>> --- a/drivers/ntb/hw/epf/ntb_hw_epf.c
>> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
>> @@ -49,6 +49,7 @@
>> #define NTB_EPF_COMMAND_TIMEOUT 1000 /* 1 Sec */
>>
>> enum pci_barno {
>> + NO_BAR = -1,
>
> Not related with this patch, but there are too many place to define
> enum pci_barno. it need be consolidate.
I agree it needs to consilidated at some point but that's another topic
and there are tiny differences between the 3 definitions so it won't be
as trivial as one might initially think
>
>> BAR_0,
>> BAR_1,
>> BAR_2,
>> @@ -57,16 +58,26 @@ enum pci_barno {
>> BAR_5,
>> };
>>
>> +enum epf_ntb_bar {
>> + BAR_CONFIG,
>> + BAR_PEER_SPAD,
>> + BAR_DB,
>> + BAR_MW1,
>> + BAR_MW2,
>> + BAR_MW3,
>> + BAR_MW4,
>> + NTB_BAR_NUM,
>> +};
>> +
>> +#define NTB_EPF_MAX_MW_COUNT (NTB_BAR_NUM - BAR_MW1)
>> +
>> struct ntb_epf_dev {
>> struct ntb_dev ntb;
>> struct device *dev;
>> /* Mutex to protect providing commands to NTB EPF */
>> struct mutex cmd_lock;
>>
>> - enum pci_barno ctrl_reg_bar;
>> - enum pci_barno peer_spad_reg_bar;
>> - enum pci_barno db_reg_bar;
>> - enum pci_barno mw_bar;
>> + const enum pci_barno *barno;
>
> barno_map?
ok
>
>>
>> unsigned int mw_count;
>> unsigned int spad_count;
>> @@ -85,17 +96,6 @@ struct ntb_epf_dev {
>>
>> #define ntb_ndev(__ntb) container_of(__ntb, struct ntb_epf_dev, ntb)
>>
>> -struct ntb_epf_data {
>> - /* BAR that contains both control region and self spad region */
>> - enum pci_barno ctrl_reg_bar;
>> - /* BAR that contains peer spad region */
>> - enum pci_barno peer_spad_reg_bar;
>> - /* BAR that contains Doorbell region and Memory window '1' */
>> - enum pci_barno db_reg_bar;
>> - /* BAR that contains memory windows*/
>> - enum pci_barno mw_bar;
>> -};
>> -
>> static int ntb_epf_send_command(struct ntb_epf_dev *ndev, u32 command,
>> u32 argument)
>> {
>> @@ -144,7 +144,7 @@ static int ntb_epf_mw_to_bar(struct ntb_epf_dev *ndev, int idx)
>> return -EINVAL;
>> }
>>
>> - return idx + 2;
>> + return ndev->barno[BAR_MW1 + idx];
>> }
>>
>> static int ntb_epf_mw_count(struct ntb_dev *ntb, int pidx)
>> @@ -413,7 +413,9 @@ static int ntb_epf_mw_set_trans(struct ntb_dev *ntb, int pidx, int idx,
>> return -EINVAL;
>> }
>>
>> - bar = idx + ndev->mw_bar;
>> + bar = ntb_epf_mw_to_bar(ndev, idx);
>> + if (bar < 0)
>> + return bar;
>>
>> mw_size = pci_resource_len(ntb->pdev, bar);
>>
>> @@ -455,7 +457,9 @@ static int ntb_epf_peer_mw_get_addr(struct ntb_dev *ntb, int idx,
>> if (idx == 0)
>> offset = readl(ndev->ctrl_reg + NTB_EPF_MW1_OFFSET);
>>
>> - bar = idx + ndev->mw_bar;
>> + bar = ntb_epf_mw_to_bar(ndev, idx);
>> + if (bar < 0)
>> + return bar;
>>
>> if (base)
>> *base = pci_resource_start(ndev->ntb.pdev, bar) + offset;
>> @@ -557,8 +561,13 @@ static int ntb_epf_init_dev(struct ntb_epf_dev *ndev)
>> }
>>
>> ndev->db_valid_mask = BIT_ULL(ndev->db_count) - 1;
>> - ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>> ndev->spad_count = readl(ndev->ctrl_reg + NTB_EPF_SPAD_COUNT);
>> + ndev->mw_count = readl(ndev->ctrl_reg + NTB_EPF_MW_COUNT);
>> +
>> + if (ndev->mw_count > NTB_EPF_MAX_MW_COUNT) {
>> + dev_err(dev, "Unsupported MW count: %u\n", ndev->mw_count);
>> + return -EINVAL;
>> + }
>>
>> return 0;
>> }
>> @@ -596,14 +605,14 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>> dev_warn(&pdev->dev, "Cannot DMA highmem\n");
>> }
>>
>> - ndev->ctrl_reg = pci_iomap(pdev, ndev->ctrl_reg_bar, 0);
>> + ndev->ctrl_reg = pci_iomap(pdev, ndev->barno[BAR_CONFIG], 0);
>> if (!ndev->ctrl_reg) {
>> ret = -EIO;
>> goto err_pci_regions;
>> }
>>
>> - if (ndev->peer_spad_reg_bar) {
>> - ndev->peer_spad_reg = pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
>> + if (ndev->barno[BAR_PEER_SPAD] != ndev->barno[BAR_CONFIG]) {
>> + ndev->peer_spad_reg = pci_iomap(pdev, ndev->barno[BAR_PEER_SPAD], 0);
>> if (!ndev->peer_spad_reg) {
>> ret = -EIO;
>> goto err_pci_regions;
>> @@ -614,7 +623,7 @@ static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
>> ndev->peer_spad_reg = ndev->ctrl_reg + spad_off + spad_sz;
>> }
>>
>> - ndev->db_reg = pci_iomap(pdev, ndev->db_reg_bar, 0);
>> + ndev->db_reg = pci_iomap(pdev, ndev->barno[BAR_DB], 0);
>> if (!ndev->db_reg) {
>> ret = -EIO;
>> goto err_pci_regions;
>> @@ -656,15 +665,20 @@ static void ntb_epf_cleanup_isr(struct ntb_epf_dev *ndev)
>> pci_free_irq_vectors(pdev);
>> }
>>
>> +static const enum pci_barno ntb_epf_default_barno[NTB_BAR_NUM] = {
>> + [BAR_CONFIG] = BAR_0,
>> + [BAR_PEER_SPAD] = BAR_1,
>> + [BAR_DB] = BAR_2,
>> + [BAR_MW1] = BAR_2,
>> + [BAR_MW2] = BAR_3,
>> + [BAR_MW3] = BAR_4,
>> + [BAR_MW4] = BAR_5
>> +};
>> +
>> static int ntb_epf_pci_probe(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>> {
>> - enum pci_barno peer_spad_reg_bar = BAR_1;
>> - enum pci_barno ctrl_reg_bar = BAR_0;
>> - enum pci_barno db_reg_bar = BAR_2;
>> - enum pci_barno mw_bar = BAR_2;
>> struct device *dev = &pdev->dev;
>> - struct ntb_epf_data *data;
>> struct ntb_epf_dev *ndev;
>> int ret;
>>
>> @@ -675,18 +689,10 @@ static int ntb_epf_pci_probe(struct pci_dev *pdev,
>> if (!ndev)
>> return -ENOMEM;
>>
>> - data = (struct ntb_epf_data *)id->driver_data;
>> - if (data) {
>> - peer_spad_reg_bar = data->peer_spad_reg_bar;
>> - ctrl_reg_bar = data->ctrl_reg_bar;
>> - db_reg_bar = data->db_reg_bar;
>> - mw_bar = data->mw_bar;
>> - }
>> + ndev->barno = (const enum pci_barno *)id->driver_data;
>> + if (!ndev->barno)
>> + ndev->barno = ntb_epf_default_barno;
>
> I think needn't check it because all .driver_data already set in ntb_epf_pci_tbl
>
A check was there before, I'm not changing what was done in that regard.
I'll another patch to implement your suggestion seperately.
> Frank
>>
>> - ndev->peer_spad_reg_bar = peer_spad_reg_bar;
>> - ndev->ctrl_reg_bar = ctrl_reg_bar;
>> - ndev->db_reg_bar = db_reg_bar;
>> - ndev->mw_bar = mw_bar;
>> ndev->dev = dev;
>>
>> ntb_epf_init_struct(ndev, pdev);
>> @@ -730,30 +736,26 @@ static void ntb_epf_pci_remove(struct pci_dev *pdev)
>> ntb_epf_deinit_pci(ndev);
>> }
>>
>> -static const struct ntb_epf_data j721e_data = {
>> - .ctrl_reg_bar = BAR_0,
>> - .peer_spad_reg_bar = BAR_1,
>> - .db_reg_bar = BAR_2,
>> - .mw_bar = BAR_2,
>> -};
>> -
>> -static const struct ntb_epf_data mx8_data = {
>> - .ctrl_reg_bar = BAR_0,
>> - .peer_spad_reg_bar = BAR_0,
>> - .db_reg_bar = BAR_2,
>> - .mw_bar = BAR_4,
>> +static const enum pci_barno mx8_barno[NTB_BAR_NUM] = {
>> + [BAR_CONFIG] = BAR_0,
>> + [BAR_PEER_SPAD] = BAR_0,
>> + [BAR_DB] = BAR_2,
>> + [BAR_MW1] = BAR_4,
>> + [BAR_MW2] = BAR_5,
>> + [BAR_MW3] = NO_BAR,
>> + [BAR_MW4] = NO_BAR,
>> };
>>
>> static const struct pci_device_id ntb_epf_pci_tbl[] = {
>> {
>> PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
>> .class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
>> - .driver_data = (kernel_ulong_t)&j721e_data,
>> + .driver_data = (kernel_ulong_t)ntb_epf_default_barno,
>> },
>> {
>> PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, 0x0809),
>> .class = PCI_CLASS_MEMORY_RAM << 8, .class_mask = 0xffff00,
>> - .driver_data = (kernel_ulong_t)&mx8_data,
>> + .driver_data = (kernel_ulong_t)mx8_barno,
>> },
>> { },
>> };
>>
>> --
>> 2.47.2
>>
--
Jerome
