[PATCH v16 0/7] pci-epf-vntb clean up

[PATCH v16 0/7] pci-epf-vntb clean up

[Frank Li]

continue patch series https://www.spinics.net/lists/linux-pci/msg130372.html
https://www.spinics.net/lists/linux-pci/msg130924.html

Lorenzo suggest create new series. Version number continue from old one
Change from v15 to v16
  consistent subject
  Add () after pci_epc_mem_free_addr

Change from v14-v15
  Fixed according to Manivannan Sadhasivam's comments, except endian
problem. Endian problem is out of this patch series scope.
  kernel test robot report sparse build warning problem already fixed
at patch 6 and 7.

Change from v13-v14
  split spare warning fix to two patch
  remove local variable reorder

Frank Li (7):
  PCI: endpoint: pci-epf-vntb: Clean up kernel_doc warning
  PCI: endpoint: pci-epf-vntb: Fix indentation of the struct
    epf_ntb_ctrl
  PCI: endpoint: pci-epf-vntb: fix call pci_epc_mem_free_addr at err
    path
  PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy
  PCI: endpoint: pci-epf-vntb: replace hardcode 4 with sizeof(u32)
  PCI: endpoint: pci-epf-vntb: fix sparse build warning at epf_db
  PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 149 ++++++++++--------
 1 file changed, 85 insertions(+), 64 deletions(-)

-- 
2.34.1

[PATCH v16 1/7] PCI: endpoint: pci-epf-vntb: clean up kernel_doc warning

[Frank Li]

From: Frank Li <frank.li@nxp.com>

Cleanup warning found by scripts/kernel-doc
Consolidate term
    host, host1 to HOST
    vhost, vHost, Vhost, VHOST2 to VHOST

Acked-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Frank Li <frank.li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 83 ++++++++++++-------
 1 file changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 0ea85e1d292e..c0115bcb3b5e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -11,7 +11,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
-/**
+/*
  * +------------+         +---------------------------------------+
  * |            |         |                                       |
  * +------------+         |                        +--------------+
@@ -156,12 +156,14 @@ static struct pci_epf_header epf_ntb_header = {
 };
 
 /**
- * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host
+ * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
  * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @link_up: true or false indicating Link is UP or Down
  *
  * Once NTB function in HOST invoke ntb_link_enable(),
- * this NTB function driver will trigger a link event to vhost.
+ * this NTB function driver will trigger a link event to VHOST.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 {
@@ -175,9 +177,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
 }
 
 /**
- * epf_ntb_configure_mw() - Configure the Outbound Address Space for vhost
- *   to access the memory window of host
- * @ntb: NTB device that facilitates communication between host and vhost
+ * epf_ntb_configure_mw() - Configure the Outbound Address Space for VHOST
+ *   to access the memory window of HOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  *                          EP Outbound Window
@@ -194,7 +196,9 @@ static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
  * |        |              |           |
  * |        |              |           |
  * +--------+              +-----------+
- *  VHost                   PCI EP
+ *  VHOST                   PCI EP
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 {
@@ -219,7 +223,7 @@ static int epf_ntb_configure_mw(struct epf_ntb *ntb, u32 mw)
 
 /**
  * epf_ntb_teardown_mw() - Teardown the configured OB ATU
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  * @mw: Index of the memory window (either 0, 1, 2 or 3)
  *
  * Teardown the configured OB ATU configured in epf_ntb_configure_mw() using
@@ -234,12 +238,12 @@ static void epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw)
 }
 
 /**
- * epf_ntb_cmd_handler() - Handle commands provided by the NTB Host
+ * epf_ntb_cmd_handler() - Handle commands provided by the NTB HOST
  * @work: work_struct for the epf_ntb_epc
  *
  * Workqueue function that gets invoked for the two epf_ntb_epc
  * periodically (once every 5ms) to see if it has received any commands
- * from NTB host. The host can send commands to configure doorbell or
+ * from NTB HOST. The HOST can send commands to configure doorbell or
  * configure memory window or to update link status.
  */
 static void epf_ntb_cmd_handler(struct work_struct *work)
@@ -321,8 +325,8 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 
 /**
  * epf_ntb_config_sspad_bar_clear() - Clear Config + Self scratchpad BAR
- * @ntb_epc: EPC associated with one of the HOST which holds peer's outbound
- *	     address.
+ * @ntb: EPC associated with one of the HOST which holds peer's outbound
+ *	 address.
  *
  * Clear BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
  * self scratchpad region (removes inbound ATU configuration). While BAR0 is
@@ -331,8 +335,10 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
  * used for self scratchpad from epf_ntb_bar[BAR_CONFIG].
  *
  * Please note the self scratchpad region and config region is combined to
- * a single region and mapped using the same BAR. Also note HOST2's peer
- * scratchpad is HOST1's self scratchpad.
+ * a single region and mapped using the same BAR. Also note VHOST's peer
+ * scratchpad is HOST's self scratchpad.
+ *
+ * Returns: void
  */
 static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 {
@@ -347,13 +353,15 @@ static void epf_ntb_config_sspad_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_config_sspad_bar_set() - Set Config + Self scratchpad BAR
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
- * Map BAR0 of EP CONTROLLER 1 which contains the HOST1's config and
+ * Map BAR0 of EP CONTROLLER which contains the VHOST's config and
  * self scratchpad region.
  *
  * Please note the self scratchpad region and config region is combined to
  * a single region and mapped using the same BAR.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 {
@@ -380,7 +388,7 @@ static int epf_ntb_config_sspad_bar_set(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_free() - Free the physical memory associated with
  *   config + scratchpad region
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 {
@@ -393,11 +401,13 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
 /**
  * epf_ntb_config_spad_bar_alloc() - Allocate memory for config + scratchpad
  *   region
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Allocate the Local Memory mentioned in the above diagram. The size of
  * CONFIG REGION is sizeof(struct epf_ntb_ctrl) and size of SCRATCHPAD REGION
  * is obtained from "spad-count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 {
@@ -465,11 +475,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 }
 
 /**
- * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capaiblity
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * epf_ntb_configure_interrupt() - Configure MSI/MSI-X capability
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Configure MSI/MSI-X capability for each interface with number of
  * interrupts equal to "db_count" configfs entry.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 {
@@ -511,7 +523,9 @@ static int epf_ntb_configure_interrupt(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_db_bar_init() - Configure Doorbell window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 {
@@ -566,7 +580,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws);
 /**
  * epf_ntb_db_bar_clear() - Clear doorbell BAR and free memory
  *   allocated in peer's outbound address space
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 {
@@ -582,8 +596,9 @@ static void epf_ntb_db_bar_clear(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_init() - Configure Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 {
@@ -639,7 +654,7 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_mw_bar_clear() - Clear Memory window BARs
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  */
 static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
 {
@@ -662,7 +677,7 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
 
 /**
  * epf_ntb_epc_destroy() - Cleanup NTB EPC interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper for epf_ntb_epc_destroy_interface() to cleanup all the NTB interfaces
  */
@@ -675,7 +690,9 @@ static void epf_ntb_epc_destroy(struct epf_ntb *ntb)
 /**
  * epf_ntb_init_epc_bar() - Identify BARs to be used for each of the NTB
  * constructs (scratchpad region, doorbell, memorywindow)
- * @ntb: NTB device that facilitates communication between HOST and vHOST
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 {
@@ -716,11 +733,13 @@ static int epf_ntb_init_epc_bar(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_init() - Initialize NTB interface
- * @ntb: NTB device that facilitates communication between HOST and vHOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to initialize a particular EPC interface and start the workqueue
- * to check for commands from host. This function will write to the
+ * to check for commands from HOST. This function will write to the
  * EP controller HW for configuring it.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_epc_init(struct epf_ntb *ntb)
 {
@@ -787,7 +806,7 @@ static int epf_ntb_epc_init(struct epf_ntb *ntb)
 
 /**
  * epf_ntb_epc_cleanup() - Cleanup all NTB interfaces
- * @ntb: NTB device that facilitates communication between HOST1 and HOST2
+ * @ntb: NTB device that facilitates communication between HOST and VHOST
  *
  * Wrapper to cleanup all NTB interfaces.
  */
@@ -951,6 +970,8 @@ static const struct config_item_type ntb_group_type = {
  *
  * Add configfs directory specific to NTB. This directory will hold
  * NTB specific properties like db_count, spad_count, num_mws etc.,
+ *
+ * Returns: Pointer to config_group
  */
 static struct config_group *epf_ntb_add_cfs(struct pci_epf *epf,
 					    struct config_group *group)
@@ -1292,6 +1313,8 @@ static struct pci_driver vntb_pci_driver = {
  * Invoked when a primary interface or secondary interface is bound to EPC
  * device. This function will succeed only when EPC is bound to both the
  * interfaces.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_bind(struct pci_epf *epf)
 {
@@ -1377,6 +1400,8 @@ static struct pci_epf_ops epf_ntb_ops = {
  *
  * Probe NTB function driver when endpoint function bus detects a NTB
  * endpoint function.
+ *
+ * Returns: Zero for success, or an error code in case of failure
  */
 static int epf_ntb_probe(struct pci_epf *epf)
 {
-- 
2.34.1

[PATCH v16 2/7] PCI: endpoint: pci-epf-vntb: fix indentation of the struct epf_ntb_ctrl

[Frank Li]

From: Frank Li <frank.li@nxp.com>

Align the indentation of struct epf_ntb_ctrl with other structs in the driver

Signed-off-by: Frank Li <frank.li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index c0115bcb3b5e..1863006cc36c 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -99,20 +99,20 @@ enum epf_ntb_bar {
  *       NTB Driver               NTB Driver
  */
 struct epf_ntb_ctrl {
-	u32     command;
-	u32     argument;
-	u16     command_status;
-	u16     link_status;
-	u32     topology;
-	u64     addr;
-	u64     size;
-	u32     num_mws;
-	u32	reserved;
-	u32     spad_offset;
-	u32     spad_count;
-	u32	db_entry_size;
-	u32     db_data[MAX_DB_COUNT];
-	u32     db_offset[MAX_DB_COUNT];
+	u32 command;
+	u32 argument;
+	u16 command_status;
+	u16 link_status;
+	u32 topology;
+	u64 addr;
+	u64 size;
+	u32 num_mws;
+	u32 reserved;
+	u32 spad_offset;
+	u32 spad_count;
+	u32 db_entry_size;
+	u32 db_data[MAX_DB_COUNT];
+	u32 db_offset[MAX_DB_COUNT];
 } __packed;
 
 struct epf_ntb {
-- 
2.34.1

[PATCH v16 3/7] PCI: endpoint: pci-epf-vntb: fix call pci_epc_mem_free_addr() at err path

[Frank Li]

From: Frank Li <frank.li@nxp.com>

Replace pci_epc_mem_free_addr() with pci_epf_free_space() at error handle
path to match pci_epf_alloc_space().

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")

Signed-off-by: Frank Li <frank.li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 1863006cc36c..191924a83454 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -571,7 +571,7 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	return ret;
 
 err_alloc_peer_mem:
-	pci_epc_mem_free_addr(ntb->epf->epc, epf_bar->phys_addr, mw_addr, epf_bar->size);
+	pci_epf_free_space(ntb->epf, mw_addr, barno, 0);
 	return -1;
 }
 
-- 
2.34.1

[PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy

[Frank Li]

From: Frank Li <frank.li@nxp.com>

epf_db_phy is not used, so remove it

Signed-off-by: Frank Li <frank.li@nxp.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 191924a83454..ee66101cb5c4 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -136,7 +136,6 @@ struct epf_ntb {
 
 	struct epf_ntb_ctrl *reg;
 
-	phys_addr_t epf_db_phy;
 	void __iomem *epf_db;
 
 	phys_addr_t vpci_mw_phy[MAX_MW];
-- 
2.34.1

Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy

[Lorenzo Pieralisi]

On Wed, Nov 02, 2022 at 10:10:11AM -0400, Frank Li wrote:
> From: Frank Li <frank.li@nxp.com>
> 
> epf_db_phy is not used, so remove it

Sentences end with a period (.). I can fix these things but
we can't spend our lives telling you how to write a commit log,
check how they are written in the PCI subsystem and follow the
pattern.

https://lore.kernel.org/all/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com

> 
> Signed-off-by: Frank Li <frank.li@nxp.com>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index 191924a83454..ee66101cb5c4 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> @@ -136,7 +136,6 @@ struct epf_ntb {
>  
>  	struct epf_ntb_ctrl *reg;
>  
> -	phys_addr_t epf_db_phy;
>  	void __iomem *epf_db;
>  
>  	phys_addr_t vpci_mw_phy[MAX_MW];
> -- 
> 2.34.1
> 

RE: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy

[Frank Li]

> -----Original Message-----
> From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Sent: Thursday, November 10, 2022 9:29 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> dave.jiang@intel.com; helgaas@kernel.org; imx@lists.linux.dev;
> jdmason@kudzu.us; kw@linux.com; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; ntb@lists.linux.dev
> Subject: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove
> unused field epf_db_phy
> 
> Caution: EXT Email
> 
> On Wed, Nov 02, 2022 at 10:10:11AM -0400, Frank Li wrote:
> > From: Frank Li <frank.li@nxp.com>
> >
> > epf_db_phy is not used, so remove it
> 
> Sentences end with a period (.). I can fix these things but
> we can't spend our lives telling you how to write a commit log,
> check how they are written in the PCI subsystem and follow the
> pattern.

[Frank Li] Do you need me send new version to fix "."? Or you will plan
queue these patches?

My means: 
Mani's below feedback will make both live easy.
	 > 
               > None use epf_db_phy and remove it.

                  "epf_db_phy is not used, so remove it"

                >
                > 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Fall%2F20171026223701.GA25649%40bhelgaas-
> glaptop.roam.corp.google.com&amp;data=05%7C01%7CFrank.Li%40nxp.co
> m%7Ca0924bed538a494cbfd508dac3304e8e%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C638036909484154968%7CUnknown%7CTWFpbGZsb
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> %3D%7C3000%7C%7C%7C&amp;sdata=B3G7sfaSVdLDC8BG95WzpBPFO5lPJ
> QpThKDcEexOHfU%3D&amp;reserved=0

[Frank Li] Thank you for your documents.

> 
> >
> > Signed-off-by: Frank Li <frank.li@nxp.com>
> > Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> > ---
> >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > index 191924a83454..ee66101cb5c4 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > @@ -136,7 +136,6 @@ struct epf_ntb {
> >
> >       struct epf_ntb_ctrl *reg;
> >
> > -     phys_addr_t epf_db_phy;
> >       void __iomem *epf_db;
> >
> >       phys_addr_t vpci_mw_phy[MAX_MW];
> > --
> > 2.34.1
> >

Re: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy

[Lorenzo Pieralisi]

On Fri, Nov 11, 2022 at 02:39:12AM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Sent: Thursday, November 10, 2022 9:29 AM
> > To: Frank Li <frank.li@nxp.com>
> > Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> > dave.jiang@intel.com; helgaas@kernel.org; imx@lists.linux.dev;
> > jdmason@kudzu.us; kw@linux.com; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org; ntb@lists.linux.dev
> > Subject: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove
> > unused field epf_db_phy
> > 
> > Caution: EXT Email
> > 
> > On Wed, Nov 02, 2022 at 10:10:11AM -0400, Frank Li wrote:
> > > From: Frank Li <frank.li@nxp.com>
> > >
> > > epf_db_phy is not used, so remove it
> > 
> > Sentences end with a period (.). I can fix these things but
> > we can't spend our lives telling you how to write a commit log,
> > check how they are written in the PCI subsystem and follow the
> > pattern.
> 
> [Frank Li] Do you need me send new version to fix "."? Or you will plan

You don't have to write your name in brackets all the time in replies,
it is clear from the indentation what I am replying to and to whom.

> queue these patches?

I will queue them but next time I won't fix the commit log myself.

> My means: 
> Mani's below feedback will make both live easy.

What feedback ? I am sorry I don't understand.

Thank you,
Lorenzo

> 	 > 
>                > None use epf_db_phy and remove it.
> 
>                   "epf_db_phy is not used, so remove it"
> 
>                 >
>                 > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> > ernel.org%2Fall%2F20171026223701.GA25649%40bhelgaas-
> > glaptop.roam.corp.google.com&amp;data=05%7C01%7CFrank.Li%40nxp.co
> > m%7Ca0924bed538a494cbfd508dac3304e8e%7C686ea1d3bc2b4c6fa92cd99c
> > 5c301635%7C0%7C0%7C638036909484154968%7CUnknown%7CTWFpbGZsb
> > 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > %3D%7C3000%7C%7C%7C&amp;sdata=B3G7sfaSVdLDC8BG95WzpBPFO5lPJ
> > QpThKDcEexOHfU%3D&amp;reserved=0
> 
> [Frank Li] Thank you for your documents.
> 
> > 
> > >
> > > Signed-off-by: Frank Li <frank.li@nxp.com>
> > > Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> > > ---
> > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > index 191924a83454..ee66101cb5c4 100644
> > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > @@ -136,7 +136,6 @@ struct epf_ntb {
> > >
> > >       struct epf_ntb_ctrl *reg;
> > >
> > > -     phys_addr_t epf_db_phy;
> > >       void __iomem *epf_db;
> > >
> > >       phys_addr_t vpci_mw_phy[MAX_MW];
> > > --
> > > 2.34.1
> > >

RE: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy

[Frank Li]

> -----Original Message-----
> From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Sent: Friday, November 11, 2022 4:56 AM
> To: Frank Li <frank.li@nxp.com>
> Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> dave.jiang@intel.com; helgaas@kernel.org; imx@lists.linux.dev;
> jdmason@kudzu.us; kw@linux.com; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; ntb@lists.linux.dev
> Subject: Re: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove
> unused field epf_db_phy
> 
> Caution: EXT Email
> 
> On Fri, Nov 11, 2022 at 02:39:12AM +0000, Frank Li wrote:
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > > Sent: Thursday, November 10, 2022 9:29 AM
> > > To: Frank Li <frank.li@nxp.com>
> > > Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> > > dave.jiang@intel.com; helgaas@kernel.org; imx@lists.linux.dev;
> > > jdmason@kudzu.us; kw@linux.com; linux-kernel@vger.kernel.org; linux-
> > > pci@vger.kernel.org; ntb@lists.linux.dev
> > > Subject: [EXT] Re: [PATCH v16 4/7] PCI: endpoint: pci-epf-vntb: remove
> > > unused field epf_db_phy
> > >
> > > Caution: EXT Email
> > >
> > > On Wed, Nov 02, 2022 at 10:10:11AM -0400, Frank Li wrote:
> > > > From: Frank Li <frank.li@nxp.com>
> > > >
> > > > epf_db_phy is not used, so remove it
> > >
> > > Sentences end with a period (.). I can fix these things but
> > > we can't spend our lives telling you how to write a commit log,
> > > check how they are written in the PCI subsystem and follow the
> > > pattern.
> >
> > [Frank Li] Do you need me send new version to fix "."? Or you will plan
> 
> You don't have to write your name in brackets all the time in replies,
> it is clear from the indentation what I am replying to and to whom.
> 
> > queue these patches?
> 
> I will queue them but next time I won't fix the commit log myself.

Do you have chance to queue it? I still have one patch (enable MSI)
depend on these patch series. 

> 
> > My means:
> > Mani's below feedback will make both live easy.
> 
> What feedback ? I am sorry I don't understand.
> 
> Thank you,
> Lorenzo
> 
> >        >
> >                > None use epf_db_phy and remove it.
> >
> >                   "epf_db_phy is not used, so remove it"
> >
> >                 >
> >                 >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> %2F&amp;data=05%7C01%7Cfrank.li%40nxp.com%7C200a354ecae54e7b1af
> 408dac3d34892%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 037609466107831%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAi
> LCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&a
> mp;sdata=3ngvYAIP0oTTF1uwAAHFAFmBVi1FvKtgTTw1u3tDXUg%3D&amp;re
> served=0
> > > ernel.org%2Fall%2F20171026223701.GA25649%40bhelgaas-
> > >
> glaptop.roam.corp.google.com&amp;data=05%7C01%7CFrank.Li%40nxp.co
> > >
> m%7Ca0924bed538a494cbfd508dac3304e8e%7C686ea1d3bc2b4c6fa92cd99c
> > >
> 5c301635%7C0%7C0%7C638036909484154968%7CUnknown%7CTWFpbGZsb
> > >
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > > %3D%7C3000%7C%7C%7C&amp;sdata=B3G7sfaSVdLDC8BG95WzpBPFO5l
> PJ
> > > QpThKDcEexOHfU%3D&amp;reserved=0
> >
> > [Frank Li] Thank you for your documents.
> >
> > >
> > > >
> > > > Signed-off-by: Frank Li <frank.li@nxp.com>
> > > > Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> > > > ---
> > > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > index 191924a83454..ee66101cb5c4 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > @@ -136,7 +136,6 @@ struct epf_ntb {
> > > >
> > > >       struct epf_ntb_ctrl *reg;
> > > >
> > > > -     phys_addr_t epf_db_phy;
> > > >       void __iomem *epf_db;
> > > >
> > > >       phys_addr_t vpci_mw_phy[MAX_MW];
> > > > --
> > > > 2.34.1
> > > >

[PATCH v16 5/7] PCI: endpoint: pci-epf-vntb: replace hardcode 4 with sizeof(u32)

[Frank Li]

From: Frank Li <frank.li@nxp.com>

ntb spad entry item size is sizeof(u32).

Signed-off-by: Frank Li <frank.li@nxp.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 +++++++++----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index ee66101cb5c4..54616281da9e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -257,12 +257,12 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 	ntb = container_of(work, struct epf_ntb, cmd_handler.work);
 
 	for (i = 1; i < ntb->db_count; i++) {
-		if (readl(ntb->epf_db + i * 4)) {
-			if (readl(ntb->epf_db + i * 4))
+		if (readl(ntb->epf_db + i * sizeof(u32))) {
+			if (readl(ntb->epf_db + i * sizeof(u32)))
 				ntb->db |= 1 << (i - 1);
 
 			ntb_db_event(&ntb->ntb, i);
-			writel(0, ntb->epf_db + i * 4);
+			writel(0, ntb->epf_db + i * sizeof(u32));
 		}
 	}
 
@@ -433,7 +433,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	spad_count = ntb->spad_count;
 
 	ctrl_size = sizeof(struct epf_ntb_ctrl);
-	spad_size = 2 * spad_count * 4;
+	spad_size = 2 * spad_count * sizeof(u32);
 
 	if (!align) {
 		ctrl_size = roundup_pow_of_two(ctrl_size);
@@ -463,7 +463,7 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
 	ctrl->num_mws = ntb->num_mws;
 	ntb->spad_size = spad_size;
 
-	ctrl->db_entry_size = 4;
+	ctrl->db_entry_size = sizeof(u32);
 
 	for (i = 0; i < ntb->db_count; i++) {
 		ntb->reg->db_data[i] = 1 + i;
@@ -535,7 +535,7 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
 	struct pci_epf_bar *epf_bar;
 	void __iomem *mw_addr;
 	enum pci_barno barno;
-	size_t size = 4 * ntb->db_count;
+	size_t size = sizeof(u32) * ntb->db_count;
 
 	epc_features = pci_epc_get_features(ntb->epf->epc,
 					    ntb->epf->func_no,
@@ -1121,11 +1121,11 @@ static int vntb_epf_link_enable(struct ntb_dev *ntb,
 static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
-	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * 4;
+	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
 	u32 val;
 	void __iomem *base = ntb->reg;
 
-	val = readl(base + off + ct + idx * 4);
+	val = readl(base + off + ct + idx * sizeof(u32));
 	return val;
 }
 
@@ -1133,10 +1133,10 @@ static int vntb_epf_spad_write(struct ntb_dev *ndev, int idx, u32 val)
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
-	int off = ctrl->spad_offset, ct = ctrl->spad_count * 4;
+	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
 	void __iomem *base = ntb->reg;
 
-	writel(val, base + off + ct + idx * 4);
+	writel(val, base + off + ct + idx * sizeof(u32));
 	return 0;
 }
 
@@ -1148,7 +1148,7 @@ static u32 vntb_epf_peer_spad_read(struct ntb_dev *ndev, int pidx, int idx)
 	void __iomem *base = ntb->reg;
 	u32 val;
 
-	val = readl(base + off + idx * 4);
+	val = readl(base + off + idx * sizeof(u32));
 	return val;
 }
 
@@ -1159,7 +1159,7 @@ static int vntb_epf_peer_spad_write(struct ntb_dev *ndev, int pidx, int idx, u32
 	int off = ctrl->spad_offset;
 	void __iomem *base = ntb->reg;
 
-	writel(val, base + off + idx * 4);
+	writel(val, base + off + idx * sizeof(u32));
 	return 0;
 }
 
-- 
2.34.1

[PATCH v16 6/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at epf_db

[Frank Li]

From: Frank Li <frank.li@nxp.com>

Using epf_db[i] instead of readl() because epf_db is in local
system memory and allocated by dma_alloc_coherent().

Signed-off-by: Frank Li <frank.li@nxp.com>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index 54616281da9e..f896846ed970 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -136,7 +136,7 @@ struct epf_ntb {
 
 	struct epf_ntb_ctrl *reg;
 
-	void __iomem *epf_db;
+	u32 *epf_db;
 
 	phys_addr_t vpci_mw_phy[MAX_MW];
 	void __iomem *vpci_mw_addr[MAX_MW];
@@ -257,12 +257,10 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 	ntb = container_of(work, struct epf_ntb, cmd_handler.work);
 
 	for (i = 1; i < ntb->db_count; i++) {
-		if (readl(ntb->epf_db + i * sizeof(u32))) {
-			if (readl(ntb->epf_db + i * sizeof(u32)))
-				ntb->db |= 1 << (i - 1);
-
+		if (ntb->epf_db[i]) {
+			ntb->db |= 1 << (i - 1);
 			ntb_db_event(&ntb->ntb, i);
-			writel(0, ntb->epf_db + i * sizeof(u32));
+			ntb->epf_db[i] = 0;
 		}
 	}
 
-- 
2.34.1

[PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

[Frank Li]

From: Frank Li <frank.li@nxp.com>

  pci-epf-vntb.c:1128:33: sparse:     expected void [noderef] __iomem *base
  pci-epf-vntb.c:1128:33: sparse:     got struct epf_ntb_ctrl *reg

Add __iomem type convert in vntb_epf_peer_spad_read() and
vntb_epf_peer_spad_write().

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Frank Li <frank.li@nxp.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-vntb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index f896846ed970..04698e7995a5 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
 	u32 val;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
 	val = readl(base + off + ct + idx * sizeof(u32));
 	return val;
@@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev *ndev, int idx, u32 val)
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
 	writel(val, base + off + ct + idx * sizeof(u32));
 	return 0;
@@ -1143,7 +1143,7 @@ static u32 vntb_epf_peer_spad_read(struct ntb_dev *ndev, int pidx, int idx)
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 	u32 val;
 
 	val = readl(base + off + idx * sizeof(u32));
@@ -1155,7 +1155,7 @@ static int vntb_epf_peer_spad_write(struct ntb_dev *ndev, int pidx, int idx, u32
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 	struct epf_ntb_ctrl *ctrl = ntb->reg;
 	int off = ctrl->spad_offset;
-	void __iomem *base = ntb->reg;
+	void __iomem *base = (void __iomem *)ntb->reg;
 
 	writel(val, base + off + idx * sizeof(u32));
 	return 0;
-- 
2.34.1

Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

[Bjorn Helgaas]

On Wed, Nov 02, 2022 at 10:10:14AM -0400, Frank Li wrote:
> From: Frank Li <frank.li@nxp.com>
> 
>   pci-epf-vntb.c:1128:33: sparse:     expected void [noderef] __iomem *base
>   pci-epf-vntb.c:1128:33: sparse:     got struct epf_ntb_ctrl *reg
> 
> Add __iomem type convert in vntb_epf_peer_spad_read() and
> vntb_epf_peer_spad_write().

I don't understand all the bits and pieces here, but I'm a little
dubious about adding all these "(void __iomem *)"casts.  There are
very few of them in drivers/pci/, and I doubt this driver is so unique
that it needs them.

> @@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
>  	int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
>  	u32 val;
> -	void __iomem *base = ntb->reg;
> +	void __iomem *base = (void __iomem *)ntb->reg;
>  
>  	val = readl(base + off + ct + idx * sizeof(u32));
>  	return val;
> @@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev *ndev, int idx, u32 val)
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
>  	struct epf_ntb_ctrl *ctrl = ntb->reg;
>  	int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> -	void __iomem *base = ntb->reg;
> +	void __iomem *base = (void __iomem *)ntb->reg;
>  
>  	writel(val, base + off + ct + idx * sizeof(u32));

These things look gratuitously different to begin with:

  int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
  int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);

They're doing the same thing, and they should do it the same way.

Since db_data[] and db_offset[] are never referenced except to be
initialized to zero, I'm guessing the point of vntb_epf_spad_read()
and vntb_epf_spad_write() is to read/write things in those arrays?

You access other things in ntb->reg directly by dereferencing a
pointer, e.g.,

  ntb->reg->link_status |= LINK_STATUS_UP;
  addr = ntb->reg->addr;
  ctrl->command_status = COMMAND_STATUS_OK;

Why don't you just compute the appropriate *index* and access the
array directly instead of using readl() and writel()?

Bjorn

RE: [EXT] Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

[Frank Li]

> 
> On Wed, Nov 02, 2022 at 10:10:14AM -0400, Frank Li wrote:
> > From: Frank Li <frank.li@nxp.com>
> >
> >   pci-epf-vntb.c:1128:33: sparse:     expected void [noderef] __iomem
> *base
> >   pci-epf-vntb.c:1128:33: sparse:     got struct epf_ntb_ctrl *reg
> >
> > Add __iomem type convert in vntb_epf_peer_spad_read() and
> > vntb_epf_peer_spad_write().
> 
> I don't understand all the bits and pieces here, but I'm a little
> dubious about adding all these "(void __iomem *)"casts.  There are
> very few of them in drivers/pci/, and I doubt this driver is so unique
> that it needs them.

sparse compiler report warning without cast.  I write it at commit message.

Best regards
Frank Li 

> 
> > @@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev
> *ndev, int idx)
> >       struct epf_ntb *ntb = ntb_ndev(ndev);
> >       int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> sizeof(u32);
> >       u32 val;
> > -     void __iomem *base = ntb->reg;
> > +     void __iomem *base = (void __iomem *)ntb->reg;
> >
> >       val = readl(base + off + ct + idx * sizeof(u32));
> >       return val;
> > @@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev
> *ndev, int idx, u32 val)
> >       struct epf_ntb *ntb = ntb_ndev(ndev);
> >       struct epf_ntb_ctrl *ctrl = ntb->reg;
> >       int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > -     void __iomem *base = ntb->reg;
> > +     void __iomem *base = (void __iomem *)ntb->reg;
> >
> >       writel(val, base + off + ct + idx * sizeof(u32));
> 
> These things look gratuitously different to begin with:
> 
>   int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
>   int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> 
> They're doing the same thing, and they should do it the same way.
> 
> Since db_data[] and db_offset[] are never referenced except to be
> initialized to zero, I'm guessing the point of vntb_epf_spad_read()
> and vntb_epf_spad_write() is to read/write things in those arrays?
> 
> You access other things in ntb->reg directly by dereferencing a
> pointer, e.g.,
> 
>   ntb->reg->link_status |= LINK_STATUS_UP;
>   addr = ntb->reg->addr;
>   ctrl->command_status = COMMAND_STATUS_OK;
> 
> Why don't you just compute the appropriate *index* and access the
> array directly instead of using readl() and writel()?
> 
> Bjorn

Re: [EXT] Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

[Bjorn Helgaas]

On Wed, Dec 14, 2022 at 12:49:15AM +0000, Frank Li wrote:
> > 
> > On Wed, Nov 02, 2022 at 10:10:14AM -0400, Frank Li wrote:
> > > From: Frank Li <frank.li@nxp.com>
> > >
> > >   pci-epf-vntb.c:1128:33: sparse:     expected void [noderef] __iomem
> > *base
> > >   pci-epf-vntb.c:1128:33: sparse:     got struct epf_ntb_ctrl *reg
> > >
> > > Add __iomem type convert in vntb_epf_peer_spad_read() and
> > > vntb_epf_peer_spad_write().
> > 
> > I don't understand all the bits and pieces here, but I'm a little
> > dubious about adding all these "(void __iomem *)"casts.  There are
> > very few of them in drivers/pci/, and I doubt this driver is so unique
> > that it needs them.
> 
> sparse compiler report warning without cast.  I write it at commit message.

As a matter of fact, I did read your commit message.  My point is that
I don't think littering the code with casts is the best solution.  I
wrote more details below; please read the entire email.

> > > @@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev
> > *ndev, int idx)
> > >       struct epf_ntb *ntb = ntb_ndev(ndev);
> > >       int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> > sizeof(u32);
> > >       u32 val;
> > > -     void __iomem *base = ntb->reg;
> > > +     void __iomem *base = (void __iomem *)ntb->reg;
> > >
> > >       val = readl(base + off + ct + idx * sizeof(u32));
> > >       return val;
> > > @@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev
> > *ndev, int idx, u32 val)
> > >       struct epf_ntb *ntb = ntb_ndev(ndev);
> > >       struct epf_ntb_ctrl *ctrl = ntb->reg;
> > >       int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > > -     void __iomem *base = ntb->reg;
> > > +     void __iomem *base = (void __iomem *)ntb->reg;
> > >
> > >       writel(val, base + off + ct + idx * sizeof(u32));
> > 
> > These things look gratuitously different to begin with:
> > 
> >   int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
> >   int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > 
> > They're doing the same thing, and they should do it the same way.
> > 
> > Since db_data[] and db_offset[] are never referenced except to be
> > initialized to zero, I'm guessing the point of vntb_epf_spad_read()
> > and vntb_epf_spad_write() is to read/write things in those arrays?
> > 
> > You access other things in ntb->reg directly by dereferencing a
> > pointer, e.g.,
> > 
> >   ntb->reg->link_status |= LINK_STATUS_UP;
> >   addr = ntb->reg->addr;
> >   ctrl->command_status = COMMAND_STATUS_OK;
> > 
> > Why don't you just compute the appropriate *index* and access the
> > array directly instead of using readl() and writel()?
> > 
> > Bjorn

RE: [EXT] Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg

[Frank Li]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, December 13, 2022 6:27 PM
> To: Frank Li <frank.li@nxp.com>
> Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> dave.jiang@intel.com; imx@lists.linux.dev; jdmason@kudzu.us;
> kw@linux.com; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> lpieralisi@kernel.org; ntb@lists.linux.dev
> Subject: [EXT] Re: [PATCH v16 7/7] PCI: endpoint: pci-epf-vntb: fix sparse
> build warning at ntb->reg
> 
> Caution: EXT Email
> 
> On Wed, Nov 02, 2022 at 10:10:14AM -0400, Frank Li wrote:
> > From: Frank Li <frank.li@nxp.com>
> >
> >   pci-epf-vntb.c:1128:33: sparse:     expected void [noderef] __iomem
> *base
> >   pci-epf-vntb.c:1128:33: sparse:     got struct epf_ntb_ctrl *reg
> >
> > Add __iomem type convert in vntb_epf_peer_spad_read() and
> > vntb_epf_peer_spad_write().
> 
> I don't understand all the bits and pieces here, but I'm a little
> dubious about adding all these "(void __iomem *)"casts.  There are
> very few of them in drivers/pci/, and I doubt this driver is so unique
> that it needs them.
> 
> > @@ -1121,7 +1121,7 @@ static u32 vntb_epf_spad_read(struct ntb_dev
> *ndev, int idx)
> >       struct epf_ntb *ntb = ntb_ndev(ndev);
> >       int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count *
> sizeof(u32);
> >       u32 val;
> > -     void __iomem *base = ntb->reg;
> > +     void __iomem *base = (void __iomem *)ntb->reg;
> >
> >       val = readl(base + off + ct + idx * sizeof(u32));
> >       return val;
> > @@ -1132,7 +1132,7 @@ static int vntb_epf_spad_write(struct ntb_dev
> *ndev, int idx, u32 val)
> >       struct epf_ntb *ntb = ntb_ndev(ndev);
> >       struct epf_ntb_ctrl *ctrl = ntb->reg;
> >       int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> > -     void __iomem *base = ntb->reg;
> > +     void __iomem *base = (void __iomem *)ntb->reg;
> >
> >       writel(val, base + off + ct + idx * sizeof(u32));
> 
> These things look gratuitously different to begin with:
> 
>   int off = ntb->reg->spad_offset, ct = ntb->reg->spad_count * sizeof(u32);
>   int off = ctrl->spad_offset, ct = ctrl->spad_count * sizeof(u32);
> 
> They're doing the same thing, and they should do it the same way.
> 
> Since db_data[] and db_offset[] are never referenced except to be
> initialized to zero,

Db_data and db_offset map to PCI host bar0,  so PCI host will read it.
It is generally used as MSI physical address and data, which PCI host
do doorbell by write these. I have followed patch, which under review.
Irq MSI platform msi change lots, I need more time to study such change.

Default use software polling.  Even though it is zero, pci host driver still use it
to calculate doorbell register offset.    

> I'm guessing the point of vntb_epf_spad_read()
> and vntb_epf_spad_write() is to read/write things in those arrays?

No, it is separated region. 

> 
> You access other things in ntb->reg directly by dereferencing a
> pointer, e.g.,
> 
>   ntb->reg->link_status |= LINK_STATUS_UP;
>   addr = ntb->reg->addr;
>   ctrl->command_status = COMMAND_STATUS_OK;
> 
> Why don't you just compute the appropriate *index* and access the
> array directly instead of using readl() and writel()?

Good question. 
NTB transfer layer treat it as register, so it need keep write\read order. 
1. write data to buffer,
2. update header point. 

1 and 2 must be keep order.  NTB transfer layer have not added memory
barrier. Need use writel to guarantee order. 

About ntb->reg,  actually I think it should use readl also,  but I port these code
from pci_epf_ntb.c and pci_epf_test.c.

PCI host side use writel, so write is ordered. 

But I think reg->* 's order can't be guaranteed at ARM platform.   Reg->addr may
get order value when check reg->command. 

At least a rmb() need after reg->command.  This code are almost run once at
Begging,  so no problem happen. 

> 
> Bjorn

Re: [PATCH v16 0/7] pci-epf-vntb clean up

[Bjorn Helgaas]

On Wed, Nov 02, 2022 at 10:10:07AM -0400, Frank Li wrote:
> continue patch series https://www.spinics.net/lists/linux-pci/msg130372.html
> https://www.spinics.net/lists/linux-pci/msg130924.html
> 
> Lorenzo suggest create new series. Version number continue from old one
> Change from v15 to v16
>   consistent subject
>   Add () after pci_epc_mem_free_addr

Don't forget these comments on your v15:
https://lore.kernel.org/r/20221101172006.GA1264778@bhelgaas

RE: [EXT] Re: [PATCH v16 0/7] pci-epf-vntb clean up

[Frank Li]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, November 2, 2022 6:03 PM
> To: Frank Li <frank.li@nxp.com>
> Cc: mani@kernel.org; allenbh@gmail.com; bhelgaas@google.com;
> dave.jiang@intel.com; imx@lists.linux.dev; jdmason@kudzu.us;
> kw@linux.com; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> lpieralisi@kernel.org; ntb@lists.linux.dev
> Subject: [EXT] Re: [PATCH v16 0/7] pci-epf-vntb clean up
> 
> Caution: EXT Email
> 
> On Wed, Nov 02, 2022 at 10:10:07AM -0400, Frank Li wrote:
> > continue patch series
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Flinux-
> pci%2Fmsg130372.html&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C01d
> f759917bd40de2a6a08dabd2668df%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C638030269909169306%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0%7C%7C%7C&amp;sdata=ejz4GV8zkQCqKY6fVhpS4Xh2yKmL1661pRdqpnOJ
> xp4%3D&amp;reserved=0
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .spinics.net%2Flists%2Flinux-
> pci%2Fmsg130924.html&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C01d
> f759917bd40de2a6a08dabd2668df%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C638030269909169306%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0%7C%7C%7C&amp;sdata=QhWY8%2F0FvNK1hvMM9JL34j5M9I9gu%2BgBt1
> JQEih0HrE%3D&amp;reserved=0
> >
> > Lorenzo suggest create new series. Version number continue from old one
> > Change from v15 to v16
> >   consistent subject
> >   Add () after pci_epc_mem_free_addr
> 
> Don't forget these comments on your v15:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
> ernel.org%2Fr%2F20221101172006.GA1264778%40bhelgaas&amp;data=05%7
> C01%7CFrank.Li%40nxp.com%7C01df759917bd40de2a6a08dabd2668df%7C68
> 6ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638030269909169306%7CUn
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6
> Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Gmi8t7jCrFXo
> 2LIxd7epeC7aOAlgaw7Xe51zLJq5gzQ%3D&amp;reserved=0

[Frank Li] I fixed all.  Just cover letter reused V15 one and forget update 
Patch lists. The real patches subject should be updated. 

Re: [PATCH v16 0/7] pci-epf-vntb clean up

[Lorenzo Pieralisi]

On Wed, 2 Nov 2022 10:10:07 -0400, Frank Li wrote:
> continue patch series https://www.spinics.net/lists/linux-pci/msg130372.html
> https://www.spinics.net/lists/linux-pci/msg130924.html
> 
> Lorenzo suggest create new series. Version number continue from old one
> Change from v15 to v16
>   consistent subject
>   Add () after pci_epc_mem_free_addr
> 
> [...]

Applied to pci/endpoint, thanks!

[1/7] PCI: endpoint: pci-epf-vntb: clean up kernel_doc warning
      https://git.kernel.org/lpieralisi/pci/c/929880484045
[2/7] PCI: endpoint: pci-epf-vntb: fix indentation of the struct epf_ntb_ctrl
      https://git.kernel.org/lpieralisi/pci/c/1d118fed348f
[3/7] PCI: endpoint: pci-epf-vntb: fix call pci_epc_mem_free_addr() at err path
      https://git.kernel.org/lpieralisi/pci/c/0c031262d2dd
[4/7] PCI: endpoint: pci-epf-vntb: remove unused field epf_db_phy
      https://git.kernel.org/lpieralisi/pci/c/03d426ae5426
[5/7] PCI: endpoint: pci-epf-vntb: replace hardcode 4 with sizeof(u32)
      https://git.kernel.org/lpieralisi/pci/c/2b35c886556a
[6/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at epf_db
      https://git.kernel.org/lpieralisi/pci/c/01dcec6d57ce
[7/7] PCI: endpoint: pci-epf-vntb: fix sparse build warning at ntb->reg
      https://git.kernel.org/lpieralisi/pci/c/5f697b25009c

Thanks,
Lorenzo