Message-ID: <aCu0Wem4KkaybW4f@lizhi-Precision-Tower-5810>
Date: Mon, 19 May 2025 18:44:41 -0400
From: Frank Li <Frank.li@....com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Jon Mason <jdmason@...zu.us>, Dave Jiang <dave.jiang@...el.com>,
	Allen Hubbe <allenbh@...il.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, ntb@...ts.linux.dev,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] NTB: epf: Allow arbitrary BAR mapping

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@...libre.com>
> ---
>  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.

>  	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?

>
>  	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

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
>

Powered by blists - more mailing lists