[PATCH v2 00/16] PCI endpoint fixes and improvements

[PATCH v2 00/16] PCI endpoint fixes and improvements

[Damien Le Moal]

This series fixes several issues with the PCI endpoint code and endpoint
test drivers (host side and EP side).

The first 2 patches address an issue with the use of configfs to create
an endpoint driver type attributes group, preventing a potential crash
if the user creates a directory multiple times for the driver type
attributes.

The following patches are fixes and improvements for the endpoint test
drivers, EP side and RP side.

This is all tested using a Pine Rockpro64 board, with the rockchip ep
driver fixed using Rick Wertenbroek <rick.wertenbroek@gmail.com>
patches [1], plus some additional fixes from me.

[1] https://lore.kernel.org/linux-pci/20230214140858.1133292-1-rick.wertenbroek@gmail.com/

Changes from v1:
 - Improved patch 1 commit message
 - Modified patch 2 to not have to add an internal header file
 - Split former patch 3 into patch 3, 4 and 5
 - Removed former patch 4 introducing volatile casts and replaced it
   with patch 9
 - Added patch 6, 7, 8 and 10
 - Added Reviewed-by tags in patches not modified

Damien Le Moal (16):
  PCI: endpoint: Automatically create a function specific attributes group
  PCI: endpoint: Move pci_epf_type_add_cfs() code
  PCI: epf-test: Fix DMA transfer completion initialization
  PCI: epf-test: Fix DMA transfer completion detection
  PCI: epf-test: Use dmaengine_submit() to initiate DMA transfer
  PCI: epf-test: Simplify read/write/copy test functions
  PCI: epf-test: Simply pci_epf_test_raise_irq()
  PCI: epf-test: Simplify IRQ test commands execution
  PCI: epf-test: Improve handling of command and status registers
  PCI: epf-test: Cleanup pci_epf_test_cmd_handler()
  PCI: epf-test: Simplify dma support checks
  PCI: epf-test: Simplify transfers result print
  misc: pci_endpoint_test: Free IRQs before removing the device
  misc: pci_endpoint_test: Re-init completion for every test
  misc: pci_endpoint_test: Do not write status in IRQ handler
  misc: pci_endpoint_test: Simplify pci_endpoint_test_msi_irq()

 drivers/misc/pci_endpoint_test.c              |  25 +-
 drivers/pci/endpoint/functions/pci-epf-test.c | 240 ++++++++----------
 drivers/pci/endpoint/pci-ep-cfs.c             |  53 ++--
 drivers/pci/endpoint/pci-epf-core.c           |  32 ---
 include/linux/pci-epf.h                       |   2 -
 5 files changed, 155 insertions(+), 197 deletions(-)

-- 
2.39.2

[PATCH v2 01/16] PCI: endpoint: Automatically create a function specific attributes group

[Damien Le Moal]

A PCI endpoint function driver can define function specific attributes
under its function configfs directory using the add_cfs() endpoint
driver operation. This is done by tighing up the mkdir operation for
the function configfs directory to a call to the add_cfs() operation.
However, there are no checks preventing the user from repeatedly
creating function specific attribute directories with different names,
resulting in the same endpoing specific attributes group being added
multiple times, which also result in an invalid refernce counting for
the attribute groups. E.g., using the pci-epf-ntb function driver as an
example, the user creates the function as follows:

 modprobe pci-epf-ntb
func0/
|-- baseclass_code
|-- cache_line_size
|-- ...
`-- vendorid

func0/
|-- attrs
|   |-- db_count
|   |-- mw1
|   |-- mw2
|   |-- mw3
|   |-- mw4
|   |-- num_mws
|   `-- spad_count
|-- baseclass_code
|-- cache_line_size
|-- ...
`-- vendorid

At this point, the function can be started by linking the EP controller.
However, if the user mistakenly creates again a directory:

func0/
|-- attrs
|   |-- db_count
|   |-- mw1
|   |-- mw2
|   |-- mw3
|   |-- mw4
|   |-- num_mws
|   `-- spad_count
|-- attrs2
|   |-- db_count
|   |-- mw1
|   |-- mw2
|   |-- mw3
|   |-- mw4
|   |-- num_mws
|   `-- spad_count
|-- baseclass_code
|-- cache_line_size
|-- ...
`-- vendorid

The function specific attributes are duplicated and cause a crash when
the function is tore down:

[ 9740.729598] ------------[ cut here ]------------
[ 9740.730071] refcount_t: addition on 0; use-after-free.
[ 9740.730564] WARNING: CPU: 2 PID: 834 at lib/refcount.c:25 refcount_warn_saturate+0xc8/0x144
[ 9740.735593] CPU: 2 PID: 834 Comm: rmdir Not tainted 6.3.0-rc1 #1
[ 9740.736133] Hardware name: Pine64 RockPro64 v2.1 (DT)
[ 9740.736586] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 9740.737210] pc : refcount_warn_saturate+0xc8/0x144
[ 9740.737648] lr : refcount_warn_saturate+0xc8/0x144
[ 9740.738085] sp : ffff800009cebc90
[ 9740.738385] x29: ffff800009cebc90 x28: ffff0000019ed700 x27: ffff0000040c3900
[ 9740.739032] x26: 0000000000000000 x25: ffff800009325320 x24: ffff0000012da000
[ 9740.739678] x23: ffff000003bd9a80 x22: ffff000005ee9580 x21: ffff000003bd9ad8
[ 9740.740324] x20: ffff0000f36cd2c8 x19: ffff0000012da2b8 x18: 0000000000000006
[ 9740.740969] x17: 0000000000000000 x16: 0000000000000000 x15: 0765076507720766
[ 9740.741615] x14: 072d077207650774 x13: ffff800009281000 x12: 000000000000056d
[ 9740.742261] x11: 00000000000001cf x10: ffff8000092d9000 x9 : ffff800009281000
[ 9740.742906] x8 : 00000000ffffefff x7 : ffff8000092d9000 x6 : 80000000fffff000
[ 9740.743552] x5 : ffff0000f7771b88 x4 : 0000000000000000 x3 : 0000000000000027
[ 9740.744197] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000019ed700
[ 9740.744842] Call trace:
[ 9740.745068]  refcount_warn_saturate+0xc8/0x144
[ 9740.745475]  config_item_get+0x7c/0x80
[ 9740.745822]  configfs_rmdir+0x17c/0x30c
[ 9740.746174]  vfs_rmdir+0x8c/0x204
[ 9740.746482]  do_rmdir+0x158/0x184
[ 9740.746787]  __arm64_sys_unlinkat+0x64/0x80
[ 9740.747171]  invoke_syscall+0x48/0x114
[ 9740.747519]  el0_svc_common.constprop.0+0x44/0xec
[ 9740.747948]  do_el0_svc+0x38/0x98
[ 9740.748255]  el0_svc+0x2c/0x84
[ 9740.748541]  el0t_64_sync_handler+0xf4/0x120
[ 9740.748932]  el0t_64_sync+0x190/0x194
[ 9740.749269] ---[ end trace 0000000000000000 ]---
[ 9740.749754] ------------[ cut here ]------------

Fix this by modifying pci_epf_cfs_work() to execute the new function
pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs()
to obtain the function specific attribute group and the group name
(directory name) from the endpoint function driver. If the function
driver defines an attribute group, pci_ep_cfs_add_type_group() then
proceeds to register this group using configfs_register_group(), thus
automatically exposing the function type pecific onfigfs attributes to
the user. E.g.:

func0/
|-- baseclass_code
|-- cache_line_size
|-- ...
|-- pci_epf_ntb.0
|   |-- db_count
|   |-- mw1
|   |-- mw2
|   |-- mw3
|   |-- mw4
|   |-- num_mws
|   `-- spad_count
|-- primary
|-- ...
`-- vendorid

With this change, there is no need for the user to create/delete
directories in the endpoint function configfs directory. The
pci_epf_type_group_ops group operations are thus removed.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/pci-ep-cfs.c | 41 ++++++++++++++-----------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index 4b8ac0ac84d5..b16fc6093c20 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -23,6 +23,7 @@ struct pci_epf_group {
 	struct config_group group;
 	struct config_group primary_epc_group;
 	struct config_group secondary_epc_group;
+	struct config_group *type_group;
 	struct delayed_work cfs_work;
 	struct pci_epf *epf;
 	int index;
@@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = {
 	.release		= pci_epf_release,
 };
 
-static struct config_group *pci_epf_type_make(struct config_group *group,
-					      const char *name)
-{
-	struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item);
-	struct config_group *epf_type_group;
-
-	epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group);
-	return epf_type_group;
-}
-
-static void pci_epf_type_drop(struct config_group *group,
-			      struct config_item *item)
-{
-	config_item_put(item);
-}
-
-static struct configfs_group_operations pci_epf_type_group_ops = {
-	.make_group     = &pci_epf_type_make,
-	.drop_item      = &pci_epf_type_drop,
-};
-
 static const struct config_item_type pci_epf_type = {
-	.ct_group_ops	= &pci_epf_type_group_ops,
 	.ct_item_ops	= &pci_epf_ops,
 	.ct_attrs	= pci_epf_attrs,
 	.ct_owner	= THIS_MODULE,
 };
 
+static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group)
+{
+	struct config_group *group;
+
+	group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group);
+	if (!group)
+		return;
+
+	if (IS_ERR(group)) {
+		pr_err("failed to create epf type specific attributes\n");
+		return;
+	}
+
+	configfs_register_group(&epf_group->group, group);
+}
+
 static void pci_epf_cfs_work(struct work_struct *work)
 {
 	struct pci_epf_group *epf_group;
@@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work)
 		pr_err("failed to create 'secondary' EPC interface\n");
 		return;
 	}
+
+	pci_ep_cfs_add_type_group(epf_group);
 }
 
 static struct config_group *pci_epf_make(struct config_group *group,
-- 
2.39.2

[PATCH v2 02/16] PCI: endpoint: Move pci_epf_type_add_cfs() code

[Damien Le Moal]

pci_epf_type_add_cfs() is called only from pci_ep_cfs_add_type_group()
in drivers/pci/endpoint/pci-ep-cfs.c, so there is no need to export this
function and we can move its code from pci-epf-core.c to pci-ep-cfs.c
as a static function.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/pci-ep-cfs.c   | 20 ++++++++++++++++++
 drivers/pci/endpoint/pci-epf-core.c | 32 -----------------------------
 include/linux/pci-epf.h             |  2 --
 3 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index b16fc6093c20..3a05e9b5a4e9 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -509,6 +509,26 @@ static const struct config_item_type pci_epf_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
+						 struct config_group *group)
+{
+	struct config_group *epf_type_group;
+
+	if (!epf->driver) {
+		dev_err(&epf->dev, "epf device not bound to driver\n");
+		return NULL;
+	}
+
+	if (!epf->driver->ops->add_cfs)
+		return NULL;
+
+	mutex_lock(&epf->lock);
+	epf_type_group = epf->driver->ops->add_cfs(epf, group);
+	mutex_unlock(&epf->lock);
+
+	return epf_type_group;
+}
+
 static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group)
 {
 	struct config_group *group;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 2036e38be093..355a6f56fcea 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -20,38 +20,6 @@ static DEFINE_MUTEX(pci_epf_mutex);
 static struct bus_type pci_epf_bus_type;
 static const struct device_type pci_epf_type;
 
-/**
- * pci_epf_type_add_cfs() - Help function drivers to expose function specific
- *                          attributes in configfs
- * @epf: the EPF device that has to be configured using configfs
- * @group: the parent configfs group (corresponding to entries in
- *         pci_epf_device_id)
- *
- * Invoke to expose function specific attributes in configfs. If the function
- * driver does not have anything to expose (attributes configured by user),
- * return NULL.
- */
-struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
-					  struct config_group *group)
-{
-	struct config_group *epf_type_group;
-
-	if (!epf->driver) {
-		dev_err(&epf->dev, "epf device not bound to driver\n");
-		return NULL;
-	}
-
-	if (!epf->driver->ops->add_cfs)
-		return NULL;
-
-	mutex_lock(&epf->lock);
-	epf_type_group = epf->driver->ops->add_cfs(epf, group);
-	mutex_unlock(&epf->lock);
-
-	return epf_type_group;
-}
-EXPORT_SYMBOL_GPL(pci_epf_type_add_cfs);
-
 /**
  * pci_epf_unbind() - Notify the function driver that the binding between the
  *		      EPF device and EPC device has been lost
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index a215dc8ce693..b8441db2fa52 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -214,8 +214,6 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
 			enum pci_epc_interface_type type);
 int pci_epf_bind(struct pci_epf *epf);
 void pci_epf_unbind(struct pci_epf *epf);
-struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
-					  struct config_group *group);
 int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
 void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
 #endif /* __LINUX_PCI_EPF_H */
-- 
2.39.2

[PATCH v2 03/16] PCI: epf-test: Fix DMA transfer completion initialization

[Damien Le Moal]

Re-initialize the transfer_complete DMA transfer completion before
calling tx_submit(), to avoid seeing the DMA transfer complete before
the completion is initialized, thus potentially losing the completion
notification.

Fixes: 8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 0f9d2ec822ac..d65419735d2e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -151,10 +151,10 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 		return -EIO;
 	}
 
+	reinit_completion(&epf_test->transfer_complete);
 	tx->callback = pci_epf_test_dma_callback;
 	tx->callback_param = epf_test;
 	cookie = tx->tx_submit(tx);
-	reinit_completion(&epf_test->transfer_complete);
 
 	ret = dma_submit_error(cookie);
 	if (ret) {
-- 
2.39.2

[PATCH v2 04/16] PCI: epf-test: Fix DMA transfer completion detection

[Damien Le Moal]

pci_epf_test_data_transfer() and pci_epf_test_dma_callback() are not
handling DMA transfer completion correctly, leading to completion
notifications to the RP side that are too early. This problem can be
detected when the RP side is running an IOMMU with messages such as:

pci-endpoint-test 0000:0b:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT
domain=0x001c address=0xfff00000 flags=0x0000]

When running the pcitest.sh tests: the address used for a previous
test transfer generates the above error while the next test transfer is
running.

Fix this by testing the dma transfer status in
pci_epf_test_dma_callback() and notifying the completion only when the
transfer status is DMA_COMPLETE or DMA_ERROR. Furthermore, in
pci_epf_test_data_transfer(), be paranoid and check again the transfer
status and always call dmaengine_terminate_sync() before returning.

Fixes: 8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 40 ++++++++++++++-----
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index d65419735d2e..557fbb91c729 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -54,6 +54,9 @@ struct pci_epf_test {
 	struct delayed_work	cmd_handler;
 	struct dma_chan		*dma_chan_tx;
 	struct dma_chan		*dma_chan_rx;
+	struct dma_chan		*transfer_chan;
+	dma_cookie_t		transfer_cookie;
+	enum dma_status		transfer_status;
 	struct completion	transfer_complete;
 	bool			dma_supported;
 	bool			dma_private;
@@ -85,8 +88,14 @@ static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
 static void pci_epf_test_dma_callback(void *param)
 {
 	struct pci_epf_test *epf_test = param;
-
-	complete(&epf_test->transfer_complete);
+	struct dma_tx_state state;
+
+	epf_test->transfer_status =
+		dmaengine_tx_status(epf_test->transfer_chan,
+				    epf_test->transfer_cookie, &state);
+	if (epf_test->transfer_status == DMA_COMPLETE ||
+	    epf_test->transfer_status == DMA_ERROR)
+		complete(&epf_test->transfer_complete);
 }
 
 /**
@@ -120,7 +129,6 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 	struct dma_async_tx_descriptor *tx;
 	struct dma_slave_config sconf = {};
 	struct device *dev = &epf->dev;
-	dma_cookie_t cookie;
 	int ret;
 
 	if (IS_ERR_OR_NULL(chan)) {
@@ -152,25 +160,35 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 	}
 
 	reinit_completion(&epf_test->transfer_complete);
+	epf_test->transfer_chan = chan;
 	tx->callback = pci_epf_test_dma_callback;
 	tx->callback_param = epf_test;
-	cookie = tx->tx_submit(tx);
+	epf_test->transfer_cookie = tx->tx_submit(tx);
 
-	ret = dma_submit_error(cookie);
+	ret = dma_submit_error(epf_test->transfer_cookie);
 	if (ret) {
-		dev_err(dev, "Failed to do DMA tx_submit %d\n", cookie);
-		return -EIO;
+		dev_err(dev, "Failed to do DMA tx_submit %d\n", ret);
+		goto terminate;
 	}
 
 	dma_async_issue_pending(chan);
 	ret = wait_for_completion_interruptible(&epf_test->transfer_complete);
 	if (ret < 0) {
-		dmaengine_terminate_sync(chan);
-		dev_err(dev, "DMA wait_for_completion_timeout\n");
-		return -ETIMEDOUT;
+		dev_err(dev, "DMA wait_for_completion interrupted\n");
+		goto terminate;
 	}
 
-	return 0;
+	if (epf_test->transfer_status == DMA_ERROR) {
+		dev_err(dev, "DMA transfer failed\n");
+		ret = -EIO;
+	}
+
+	WARN_ON(epf_test->transfer_status != DMA_COMPLETE);
+
+terminate:
+	dmaengine_terminate_sync(chan);
+
+	return ret;
 }
 
 struct epf_dma_filter {
-- 
2.39.2

[PATCH v2 05/16] PCI: epf-test: Use dmaengine_submit() to initiate DMA transfer

[Damien Le Moal]

Instead of an open coded call to the tx_submit() operation of struct
dma_async_tx_descriptor, use the helper function dmaengine_submit().
No functional change is introduced with this.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 557fbb91c729..3dce9827bd2a 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -163,7 +163,7 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 	epf_test->transfer_chan = chan;
 	tx->callback = pci_epf_test_dma_callback;
 	tx->callback_param = epf_test;
-	epf_test->transfer_cookie = tx->tx_submit(tx);
+	epf_test->transfer_cookie = dmaengine_submit(tx);
 
 	ret = dma_submit_error(epf_test->transfer_cookie);
 	if (ret) {
-- 
2.39.2

[PATCH v2 06/16] PCI: epf-test: Simplify read/write/copy test functions

[Damien Le Moal]

The function pci_epf_test_cmd_handler() casts the register bar to a
struct pci_epf_test_reg to determine the command sent by the host and
execute the test function accordingly. So there is no need for doing
this cast again in each of the read, write and copy test functions. We
can simply pass the reg pointer as an argument to the functions
pci_epf_test_write(), pci_epf_test_read() and pci_epf_test_copy().

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++++++-----------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 3dce9827bd2a..1fc245d79a8e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -327,7 +327,8 @@ static void pci_epf_test_print_rate(const char *ops, u64 size,
 		(u64)ts.tv_sec, (u32)ts.tv_nsec, rate / 1024);
 }
 
-static int pci_epf_test_copy(struct pci_epf_test *epf_test)
+static int pci_epf_test_copy(struct pci_epf_test *epf_test,
+			     struct pci_epf_test_reg *reg)
 {
 	int ret;
 	bool use_dma;
@@ -339,8 +340,6 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 	struct pci_epf *epf = epf_test->epf;
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
-	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
-	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
 	if (!src_addr) {
@@ -426,7 +425,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 	return ret;
 }
 
-static int pci_epf_test_read(struct pci_epf_test *epf_test)
+static int pci_epf_test_read(struct pci_epf_test *epf_test,
+			     struct pci_epf_test_reg *reg)
 {
 	int ret;
 	void __iomem *src_addr;
@@ -440,8 +440,6 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
 	struct device *dma_dev = epf->epc->dev.parent;
-	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
-	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
 	if (!src_addr) {
@@ -516,7 +514,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 	return ret;
 }
 
-static int pci_epf_test_write(struct pci_epf_test *epf_test)
+static int pci_epf_test_write(struct pci_epf_test *epf_test,
+			      struct pci_epf_test_reg *reg)
 {
 	int ret;
 	void __iomem *dst_addr;
@@ -529,8 +528,6 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
 	struct device *dma_dev = epf->epc->dev.parent;
-	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
-	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
 	if (!dst_addr) {
@@ -675,7 +672,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	}
 
 	if (command & COMMAND_WRITE) {
-		ret = pci_epf_test_write(epf_test);
+		ret = pci_epf_test_write(epf_test, reg);
 		if (ret)
 			reg->status |= STATUS_WRITE_FAIL;
 		else
@@ -686,7 +683,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	}
 
 	if (command & COMMAND_READ) {
-		ret = pci_epf_test_read(epf_test);
+		ret = pci_epf_test_read(epf_test, reg);
 		if (!ret)
 			reg->status |= STATUS_READ_SUCCESS;
 		else
@@ -697,7 +694,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	}
 
 	if (command & COMMAND_COPY) {
-		ret = pci_epf_test_copy(epf_test);
+		ret = pci_epf_test_copy(epf_test, reg);
 		if (!ret)
 			reg->status |= STATUS_COPY_SUCCESS;
 		else
-- 
2.39.2

[PATCH v2 07/16] PCI: epf-test: Simply pci_epf_test_raise_irq()

[Damien Le Moal]

Change the interface of the function pci_epf_test_raise_irq() to
directly pass a pointer to the struct pci_epf_test_reg defining the test
being executed. This avoids the need for grabbing this pointer with a
cast of the register bar and simplifies the call sites as the irq type
and irq numbers do not have to be passed as arguments.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 1fc245d79a8e..6f4ef5251452 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -609,29 +609,27 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
 	return ret;
 }
 
-static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
-				   u16 irq)
+static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
+				   struct pci_epf_test_reg *reg)
 {
 	struct pci_epf *epf = epf_test->epf;
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
-	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
-	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	reg->status |= STATUS_IRQ_RAISED;
 
-	switch (irq_type) {
+	switch (reg->irq_type) {
 	case IRQ_TYPE_LEGACY:
 		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
 				  PCI_EPC_IRQ_LEGACY, 0);
 		break;
 	case IRQ_TYPE_MSI:
 		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_MSI, irq);
+				  PCI_EPC_IRQ_MSI, reg->irq_number);
 		break;
 	case IRQ_TYPE_MSIX:
 		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_MSIX, irq);
+				  PCI_EPC_IRQ_MSIX, reg->irq_number);
 		break;
 	default:
 		dev_err(dev, "Failed to raise IRQ, unknown type\n");
@@ -677,8 +675,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 			reg->status |= STATUS_WRITE_FAIL;
 		else
 			reg->status |= STATUS_WRITE_SUCCESS;
-		pci_epf_test_raise_irq(epf_test, reg->irq_type,
-				       reg->irq_number);
+		pci_epf_test_raise_irq(epf_test, reg);
 		goto reset_handler;
 	}
 
@@ -688,8 +685,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 			reg->status |= STATUS_READ_SUCCESS;
 		else
 			reg->status |= STATUS_READ_FAIL;
-		pci_epf_test_raise_irq(epf_test, reg->irq_type,
-				       reg->irq_number);
+		pci_epf_test_raise_irq(epf_test, reg);
 		goto reset_handler;
 	}
 
@@ -699,8 +695,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 			reg->status |= STATUS_COPY_SUCCESS;
 		else
 			reg->status |= STATUS_COPY_FAIL;
-		pci_epf_test_raise_irq(epf_test, reg->irq_type,
-				       reg->irq_number);
+		pci_epf_test_raise_irq(epf_test, reg);
 		goto reset_handler;
 	}
 
-- 
2.39.2

[PATCH v2 08/16] PCI: epf-test: Simplify IRQ test commands execution

[Damien Le Moal]

For the commands COMMAND_RAISE_LEGACY_IRQ, COMMAND_RAISE_MSI_IRQ and
COMMAND_RAISE_MSIX_IRQ, the function pci_epf_test_cmd_handler()
sets the STATUS_IRQ_RAISED status flag and calls the epc function
pci_epc_raise_irq() directly. However, this is also exactly what the
pci_epf_test_raise_irq() function does. Avoid duplicating these
operations by directly using pci_epf_test_raise_irq() for the IRQ test
commands. It is OK to do so as the host side endpoint test driver always
set the correct irq type for the IRQ test commands.

At the same time, the irq number check done for the
COMMAND_RAISE_MSI_IRQ and COMMAND_RAISE_MSIX_IRQ commands can also be
moved to pci_epf_test_raise_irq() to also check the IRQ number requested
by the host for other test commands.

Overall, this significantly simplifies the pci_epf_test_cmd_handler()
function.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 43 ++++++++-----------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 6f4ef5251452..43d623682850 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -615,6 +615,7 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
 	struct pci_epf *epf = epf_test->epf;
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
+	int count;
 
 	reg->status |= STATUS_IRQ_RAISED;
 
@@ -624,10 +625,22 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
 				  PCI_EPC_IRQ_LEGACY, 0);
 		break;
 	case IRQ_TYPE_MSI:
+		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
+		if (reg->irq_number > count || count <= 0) {
+			dev_err(dev, "Invalid MSI IRQ number %d / %d\n",
+				reg->irq_number, count);
+			return;
+		}
 		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
 				  PCI_EPC_IRQ_MSI, reg->irq_number);
 		break;
 	case IRQ_TYPE_MSIX:
+		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
+		if (reg->irq_number > count || count <= 0) {
+			dev_err(dev, "Invalid MSIX IRQ number %d / %d\n",
+				reg->irq_number, count);
+			return;
+		}
 		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
 				  PCI_EPC_IRQ_MSIX, reg->irq_number);
 		break;
@@ -640,13 +653,11 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
 static void pci_epf_test_cmd_handler(struct work_struct *work)
 {
 	int ret;
-	int count;
 	u32 command;
 	struct pci_epf_test *epf_test = container_of(work, struct pci_epf_test,
 						     cmd_handler.work);
 	struct pci_epf *epf = epf_test->epf;
 	struct device *dev = &epf->dev;
-	struct pci_epc *epc = epf->epc;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
@@ -662,10 +673,10 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 		goto reset_handler;
 	}
 
-	if (command & COMMAND_RAISE_LEGACY_IRQ) {
-		reg->status = STATUS_IRQ_RAISED;
-		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_LEGACY, 0);
+	if ((command & COMMAND_RAISE_LEGACY_IRQ) ||
+	    (command & COMMAND_RAISE_MSI_IRQ) ||
+	    (command & COMMAND_RAISE_MSIX_IRQ)) {
+		pci_epf_test_raise_irq(epf_test, reg);
 		goto reset_handler;
 	}
 
@@ -699,26 +710,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 		goto reset_handler;
 	}
 
-	if (command & COMMAND_RAISE_MSI_IRQ) {
-		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
-		if (reg->irq_number > count || count <= 0)
-			goto reset_handler;
-		reg->status = STATUS_IRQ_RAISED;
-		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_MSI, reg->irq_number);
-		goto reset_handler;
-	}
-
-	if (command & COMMAND_RAISE_MSIX_IRQ) {
-		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
-		if (reg->irq_number > count || count <= 0)
-			goto reset_handler;
-		reg->status = STATUS_IRQ_RAISED;
-		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_MSIX, reg->irq_number);
-		goto reset_handler;
-	}
-
 reset_handler:
 	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
 			   msecs_to_jiffies(1));
-- 
2.39.2

[PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Damien Le Moal]

The pci-epf-test driver uses the test register bar memory directly
to get and execute a test registers set by the RC side and defined
using a struct pci_epf_test_reg. This direct use relies on a casts of
the register bar to get a pointer to a struct pci_epf_test_reg to
execute the test case and sending back the test result through the
status field of struct pci_epf_test_reg. In practice, the status field
is always updated before an interrupt is raised in
pci_epf_test_raise_irq(), to ensure that the RC side sees the updated
status when receiving the interrupts.

However, such cast-based direct access does not ensure that changes to
the status register make it to memory, and so visible to the host,
before an interrupt is raised, thus potentially resulting in the RC host
not seeing the correct status result for a test.

Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when
accessing the command and status fields of a pci_epf_test_reg structure.
This ensure that a test start (pci_epf_test_cmd_handler() function) and
completion (with the function pci_epf_test_raise_irq()) achive a correct
synchronization with the host side mmio register accesses.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 43d623682850..e0cf8c2bf6db 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
 	struct pci_epf *epf = epf_test->epf;
 	struct device *dev = &epf->dev;
 	struct pci_epc *epc = epf->epc;
+	u32 status = reg->status | STATUS_IRQ_RAISED;
 	int count;
 
-	reg->status |= STATUS_IRQ_RAISED;
+	/*
+	 * Set the status before raising the IRQ to ensure that the host sees
+	 * the updated value when it gets the IRQ.
+	 */
+	WRITE_ONCE(reg->status, status);
 
 	switch (reg->irq_type) {
 	case IRQ_TYPE_LEGACY:
@@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
-	command = reg->command;
+	command = READ_ONCE(reg->command);
 	if (!command)
 		goto reset_handler;
 
-	reg->command = 0;
-	reg->status = 0;
+	WRITE_ONCE(reg->command, 0);
+	WRITE_ONCE(reg->status, 0);
 
 	if (reg->irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Failed to detect IRQ type\n");
-- 
2.39.2

[PATCH v2 10/16] PCI: epf-test: Cleanup pci_epf_test_cmd_handler()

[Damien Le Moal]

Command codes are never combined together as flags into a single value.
Thus we can replace the series of "if" tests in
pci_epf_test_cmd_handler() with a cleaner switch-case statement.
This also allows checking that we got a valid command and print an error
message if we did not.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 30 +++++++++----------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index e0cf8c2bf6db..d1b5441391fb 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -678,41 +678,39 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 		goto reset_handler;
 	}
 
-	if ((command & COMMAND_RAISE_LEGACY_IRQ) ||
-	    (command & COMMAND_RAISE_MSI_IRQ) ||
-	    (command & COMMAND_RAISE_MSIX_IRQ)) {
+	switch (command) {
+	case COMMAND_RAISE_LEGACY_IRQ:
+	case COMMAND_RAISE_MSI_IRQ:
+	case COMMAND_RAISE_MSIX_IRQ:
 		pci_epf_test_raise_irq(epf_test, reg);
-		goto reset_handler;
-	}
-
-	if (command & COMMAND_WRITE) {
+		break;
+	case COMMAND_WRITE:
 		ret = pci_epf_test_write(epf_test, reg);
 		if (ret)
 			reg->status |= STATUS_WRITE_FAIL;
 		else
 			reg->status |= STATUS_WRITE_SUCCESS;
 		pci_epf_test_raise_irq(epf_test, reg);
-		goto reset_handler;
-	}
-
-	if (command & COMMAND_READ) {
+		break;
+	case COMMAND_READ:
 		ret = pci_epf_test_read(epf_test, reg);
 		if (!ret)
 			reg->status |= STATUS_READ_SUCCESS;
 		else
 			reg->status |= STATUS_READ_FAIL;
 		pci_epf_test_raise_irq(epf_test, reg);
-		goto reset_handler;
-	}
-
-	if (command & COMMAND_COPY) {
+		break;
+	case COMMAND_COPY:
 		ret = pci_epf_test_copy(epf_test, reg);
 		if (!ret)
 			reg->status |= STATUS_COPY_SUCCESS;
 		else
 			reg->status |= STATUS_COPY_FAIL;
 		pci_epf_test_raise_irq(epf_test, reg);
-		goto reset_handler;
+		break;
+	default:
+		dev_err(dev, "Invalid command\n");
+		break;
 	}
 
 reset_handler:
-- 
2.39.2

[PATCH v2 11/16] PCI: epf-test: Simplify dma support checks

[Damien Le Moal]

There is no need to have each read, write and copy test functions check
for the FLAG_USE_DMA flag against the dma support status indicated by
epf_test->dma_supported. Move this test to the command handler function
pci_epf_test_cmd_handler() to check once for all cases.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 45 +++++++------------
 1 file changed, 15 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index d1b5441391fb..eaa252a170a2 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -331,7 +331,6 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
 			     struct pci_epf_test_reg *reg)
 {
 	int ret;
-	bool use_dma;
 	void __iomem *src_addr;
 	void __iomem *dst_addr;
 	phys_addr_t src_phys_addr;
@@ -374,14 +373,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
 	}
 
 	ktime_get_ts64(&start);
-	use_dma = !!(reg->flags & FLAG_USE_DMA);
-	if (use_dma) {
-		if (!epf_test->dma_supported) {
-			dev_err(dev, "Cannot transfer data using DMA\n");
-			ret = -EINVAL;
-			goto err_map_addr;
-		}
-
+	if (reg->flags & FLAG_USE_DMA) {
 		if (epf_test->dma_private) {
 			dev_err(dev, "Cannot transfer data using DMA\n");
 			ret = -EINVAL;
@@ -407,7 +399,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
 		kfree(buf);
 	}
 	ktime_get_ts64(&end);
-	pci_epf_test_print_rate("COPY", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate("COPY", reg->size, &start, &end,
+				reg->flags & FLAG_USE_DMA);
 
 err_map_addr:
 	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
@@ -432,7 +425,6 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
 	void __iomem *src_addr;
 	void *buf;
 	u32 crc32;
-	bool use_dma;
 	phys_addr_t phys_addr;
 	phys_addr_t dst_phys_addr;
 	struct timespec64 start, end;
@@ -463,14 +455,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
 		goto err_map_addr;
 	}
 
-	use_dma = !!(reg->flags & FLAG_USE_DMA);
-	if (use_dma) {
-		if (!epf_test->dma_supported) {
-			dev_err(dev, "Cannot transfer data using DMA\n");
-			ret = -EINVAL;
-			goto err_dma_map;
-		}
-
+	if (reg->flags & FLAG_USE_DMA) {
 		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
 					       DMA_FROM_DEVICE);
 		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
@@ -495,7 +480,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("READ", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate("READ", reg->size, &start, &end,
+				reg->flags & FLAG_USE_DMA);
 
 	crc32 = crc32_le(~0, buf, reg->size);
 	if (crc32 != reg->checksum)
@@ -520,7 +506,6 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
 	int ret;
 	void __iomem *dst_addr;
 	void *buf;
-	bool use_dma;
 	phys_addr_t phys_addr;
 	phys_addr_t src_phys_addr;
 	struct timespec64 start, end;
@@ -554,14 +539,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
 	get_random_bytes(buf, reg->size);
 	reg->checksum = crc32_le(~0, buf, reg->size);
 
-	use_dma = !!(reg->flags & FLAG_USE_DMA);
-	if (use_dma) {
-		if (!epf_test->dma_supported) {
-			dev_err(dev, "Cannot transfer data using DMA\n");
-			ret = -EINVAL;
-			goto err_dma_map;
-		}
-
+	if (reg->flags & FLAG_USE_DMA) {
 		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
 					       DMA_TO_DEVICE);
 		if (dma_mapping_error(dma_dev, src_phys_addr)) {
@@ -588,7 +566,8 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("WRITE", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate("WRITE", reg->size, &start, &end,
+				reg->flags & FLAG_USE_DMA);
 
 	/*
 	 * wait 1ms inorder for the write to complete. Without this delay L3
@@ -673,6 +652,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	WRITE_ONCE(reg->command, 0);
 	WRITE_ONCE(reg->status, 0);
 
+	if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) &&
+	    !epf_test->dma_supported) {
+		dev_err(dev, "Cannot transfer data using DMA\n");
+		goto reset_handler;
+	}
+
 	if (reg->irq_type > IRQ_TYPE_MSIX) {
 		dev_err(dev, "Failed to detect IRQ type\n");
 		goto reset_handler;
-- 
2.39.2

[PATCH v2 12/16] PCI: epf-test: Simplify transfers result print

[Damien Le Moal]

In pci_epf_test_print_rate(), instead of open coding a reduction loop to
allow for a division by a 32-bits ns value, simply use div64_u64() to
calculate the transfer rate. To match the printed unit of KB/s, this
calculation divides the rate by 1000 instead of 1024 (that would be
KiB/s unit).

The format of the results printed by pci_epf_test_print_rate() is also
changed to be more compact without the double new line. dev_info() is
used instead of pr_info().

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 39 +++++++------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index eaa252a170a2..9021c4acc743 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -297,34 +297,23 @@ static void pci_epf_test_clean_dma_chan(struct pci_epf_test *epf_test)
 	return;
 }
 
-static void pci_epf_test_print_rate(const char *ops, u64 size,
+static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
+				    const char *op, u64 size,
 				    struct timespec64 *start,
 				    struct timespec64 *end, bool dma)
 {
-	struct timespec64 ts;
-	u64 rate, ns;
-
-	ts = timespec64_sub(*end, *start);
-
-	/* convert both size (stored in 'rate') and time in terms of 'ns' */
-	ns = timespec64_to_ns(&ts);
-	rate = size * NSEC_PER_SEC;
-
-	/* Divide both size (stored in 'rate') and ns by a common factor */
-	while (ns > UINT_MAX) {
-		rate >>= 1;
-		ns >>= 1;
-	}
-
-	if (!ns)
-		return;
+	struct timespec64 ts = timespec64_sub(*end, *start);
+	u64 rate = 0, ns;
 
 	/* calculate the rate */
-	do_div(rate, (uint32_t)ns);
+	ns = timespec64_to_ns(&ts);
+	if (ns)
+		rate = div64_u64(size * NSEC_PER_SEC, ns * 1000);
 
-	pr_info("\n%s => Size: %llu bytes\t DMA: %s\t Time: %llu.%09u seconds\t"
-		"Rate: %llu KB/s\n", ops, size, dma ? "YES" : "NO",
-		(u64)ts.tv_sec, (u32)ts.tv_nsec, rate / 1024);
+	dev_info(&epf_test->epf->dev,
+		 "%s => Size: %llu B, DMA: %s, Time: %llu.%09u s, Rate: %llu KB/s\n",
+		 op, size, dma ? "YES" : "NO",
+		 (u64)ts.tv_sec, (u32)ts.tv_nsec, rate);
 }
 
 static int pci_epf_test_copy(struct pci_epf_test *epf_test,
@@ -399,7 +388,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
 		kfree(buf);
 	}
 	ktime_get_ts64(&end);
-	pci_epf_test_print_rate("COPY", reg->size, &start, &end,
+	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
 				reg->flags & FLAG_USE_DMA);
 
 err_map_addr:
@@ -480,7 +469,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("READ", reg->size, &start, &end,
+	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
 				reg->flags & FLAG_USE_DMA);
 
 	crc32 = crc32_le(~0, buf, reg->size);
@@ -566,7 +555,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("WRITE", reg->size, &start, &end,
+	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
 				reg->flags & FLAG_USE_DMA);
 
 	/*
-- 
2.39.2

[PATCH v2 13/16] misc: pci_endpoint_test: Free IRQs before removing the device

[Damien Le Moal]

In pci_endpoint_test_remove(), freeing the IRQs after removing the
device creates a small race window for IRQs to be received with the test
device memory already released, causing the IRQ handler to access
invalid memory, resulting in an oops.

Free the device IRQs before removing the device to avoid this issue.

Fixes: e03327122e2c ("pci_endpoint_test: Add 2 ioctl commands")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index a7244de081ec..01235236e9bc 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -938,6 +938,9 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 	if (id < 0)
 		return;
 
+	pci_endpoint_test_release_irq(test);
+	pci_endpoint_test_free_irq_vectors(test);
+
 	misc_deregister(&test->miscdev);
 	kfree(misc_device->name);
 	kfree(test->name);
@@ -947,9 +950,6 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)
 			pci_iounmap(pdev, test->bar[bar]);
 	}
 
-	pci_endpoint_test_release_irq(test);
-	pci_endpoint_test_free_irq_vectors(test);
-
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 }
-- 
2.39.2

[PATCH v2 14/16] misc: pci_endpoint_test: Re-init completion for every test

[Damien Le Moal]

The irq_raised completion used to detect the end of a test case is
initialized when the test device is probed, but never reinitialized
again before a test case. As a result, the irq_raised completion
synchronization is effective only for the first ioctl test case
executed. Any subsequent call to wait_for_completion() by another
ioctl() call will immediately return, potentially too early, leading to
false positive failures.

Fix this by reinitializing the irq_raised completion before starting a
new ioctl() test command.

Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/misc/pci_endpoint_test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 01235236e9bc..24efe3b88a1f 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -729,6 +729,10 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 	struct pci_dev *pdev = test->pdev;
 
 	mutex_lock(&test->mutex);
+
+	reinit_completion(&test->irq_raised);
+	test->last_irq = -ENODATA;
+
 	switch (cmd) {
 	case PCITEST_BAR:
 		bar = arg;
-- 
2.39.2

[PATCH v2 15/16] misc: pci_endpoint_test: Do not write status in IRQ handler

[Damien Le Moal]

pci_endpoint_test_irqhandler() always rewrites the status register when
an IRQ is raised, either as-is if STATUS_IRQ_RAISED is not set, or with
STATUS_IRQ_RAISED cleared if that flag is set. The first case creates a
race window with the endpoint side, meaning that the host side test
driver may end up reading what it just wrote, thus loosing the real
status as set by the endpoint side before raising the next interrupt.
This can prevent detecting that the STATUS_IRQ_RAISED flag was set by
the endpoint.

Remove this race window by not clearing the STATUS_IRQ_RAISED status
flag and not rewriting that register for every IRQ received.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 24efe3b88a1f..afd2577261f8 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -159,10 +159,7 @@ static irqreturn_t pci_endpoint_test_irqhandler(int irq, void *dev_id)
 	if (reg & STATUS_IRQ_RAISED) {
 		test->last_irq = irq;
 		complete(&test->irq_raised);
-		reg &= ~STATUS_IRQ_RAISED;
 	}
-	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS,
-				 reg);
 
 	return IRQ_HANDLED;
 }
-- 
2.39.2

[PATCH v2 16/16] misc: pci_endpoint_test: Simplify pci_endpoint_test_msi_irq()

[Damien Le Moal]

Simplify the code of pci_endpoint_test_msi_irq() by correctly using
booleans: remove the msix comparison to false as that variable is
already a boolean, and directly return the result of the comparison of
the raised interrupt number.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index afd2577261f8..ed4d0ef5e5c3 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -313,21 +313,17 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
 	struct pci_dev *pdev = test->pdev;
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
-				 msix == false ? IRQ_TYPE_MSI :
-				 IRQ_TYPE_MSIX);
+				 msix ? IRQ_TYPE_MSIX : IRQ_TYPE_MSI);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_NUMBER, msi_num);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_COMMAND,
-				 msix == false ? COMMAND_RAISE_MSI_IRQ :
-				 COMMAND_RAISE_MSIX_IRQ);
+				 msix ? COMMAND_RAISE_MSIX_IRQ :
+				 COMMAND_RAISE_MSI_IRQ);
 	val = wait_for_completion_timeout(&test->irq_raised,
 					  msecs_to_jiffies(1000));
 	if (!val)
 		return false;
 
-	if (pci_irq_vector(pdev, msi_num - 1) == test->last_irq)
-		return true;
-
-	return false;
+	return pci_irq_vector(pdev, msi_num - 1) == test->last_irq;
 }
 
 static int pci_endpoint_test_validate_xfer_params(struct device *dev,
-- 
2.39.2

Re: [PATCH v2 01/16] PCI: endpoint: Automatically create a function specific attributes group

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:02:58PM +0900, Damien Le Moal wrote:
> A PCI endpoint function driver can define function specific attributes
> under its function configfs directory using the add_cfs() endpoint
> driver operation. This is done by tighing up the mkdir operation for
> the function configfs directory to a call to the add_cfs() operation.
> However, there are no checks preventing the user from repeatedly
> creating function specific attribute directories with different names,
> resulting in the same endpoing specific attributes group being added

endpoint

> multiple times, which also result in an invalid refernce counting for

reference

> the attribute groups. E.g., using the pci-epf-ntb function driver as an
> example, the user creates the function as follows:
> 
>  modprobe pci-epf-ntb
> func0/
> |-- baseclass_code
> |-- cache_line_size
> |-- ...
> `-- vendorid
> 
> func0/
> |-- attrs
> |   |-- db_count
> |   |-- mw1
> |   |-- mw2
> |   |-- mw3
> |   |-- mw4
> |   |-- num_mws
> |   `-- spad_count
> |-- baseclass_code
> |-- cache_line_size
> |-- ...
> `-- vendorid
> 
> At this point, the function can be started by linking the EP controller.
> However, if the user mistakenly creates again a directory:
> 
> func0/
> |-- attrs
> |   |-- db_count
> |   |-- mw1
> |   |-- mw2
> |   |-- mw3
> |   |-- mw4
> |   |-- num_mws
> |   `-- spad_count
> |-- attrs2
> |   |-- db_count
> |   |-- mw1
> |   |-- mw2
> |   |-- mw3
> |   |-- mw4
> |   |-- num_mws
> |   `-- spad_count
> |-- baseclass_code
> |-- cache_line_size
> |-- ...
> `-- vendorid
> 
> The function specific attributes are duplicated and cause a crash when
> the function is tore down:
> 
> [ 9740.729598] ------------[ cut here ]------------
> [ 9740.730071] refcount_t: addition on 0; use-after-free.
> [ 9740.730564] WARNING: CPU: 2 PID: 834 at lib/refcount.c:25 refcount_warn_saturate+0xc8/0x144
> [ 9740.735593] CPU: 2 PID: 834 Comm: rmdir Not tainted 6.3.0-rc1 #1
> [ 9740.736133] Hardware name: Pine64 RockPro64 v2.1 (DT)
> [ 9740.736586] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 9740.737210] pc : refcount_warn_saturate+0xc8/0x144
> [ 9740.737648] lr : refcount_warn_saturate+0xc8/0x144
> [ 9740.738085] sp : ffff800009cebc90
> [ 9740.738385] x29: ffff800009cebc90 x28: ffff0000019ed700 x27: ffff0000040c3900
> [ 9740.739032] x26: 0000000000000000 x25: ffff800009325320 x24: ffff0000012da000
> [ 9740.739678] x23: ffff000003bd9a80 x22: ffff000005ee9580 x21: ffff000003bd9ad8
> [ 9740.740324] x20: ffff0000f36cd2c8 x19: ffff0000012da2b8 x18: 0000000000000006
> [ 9740.740969] x17: 0000000000000000 x16: 0000000000000000 x15: 0765076507720766
> [ 9740.741615] x14: 072d077207650774 x13: ffff800009281000 x12: 000000000000056d
> [ 9740.742261] x11: 00000000000001cf x10: ffff8000092d9000 x9 : ffff800009281000
> [ 9740.742906] x8 : 00000000ffffefff x7 : ffff8000092d9000 x6 : 80000000fffff000
> [ 9740.743552] x5 : ffff0000f7771b88 x4 : 0000000000000000 x3 : 0000000000000027
> [ 9740.744197] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000019ed700
> [ 9740.744842] Call trace:
> [ 9740.745068]  refcount_warn_saturate+0xc8/0x144
> [ 9740.745475]  config_item_get+0x7c/0x80
> [ 9740.745822]  configfs_rmdir+0x17c/0x30c
> [ 9740.746174]  vfs_rmdir+0x8c/0x204
> [ 9740.746482]  do_rmdir+0x158/0x184
> [ 9740.746787]  __arm64_sys_unlinkat+0x64/0x80
> [ 9740.747171]  invoke_syscall+0x48/0x114
> [ 9740.747519]  el0_svc_common.constprop.0+0x44/0xec
> [ 9740.747948]  do_el0_svc+0x38/0x98
> [ 9740.748255]  el0_svc+0x2c/0x84
> [ 9740.748541]  el0t_64_sync_handler+0xf4/0x120
> [ 9740.748932]  el0t_64_sync+0x190/0x194
> [ 9740.749269] ---[ end trace 0000000000000000 ]---
> [ 9740.749754] ------------[ cut here ]------------
> 
> Fix this by modifying pci_epf_cfs_work() to execute the new function
> pci_ep_cfs_add_type_group() which itself calls pci_epf_type_add_cfs()
> to obtain the function specific attribute group and the group name
> (directory name) from the endpoint function driver. If the function
> driver defines an attribute group, pci_ep_cfs_add_type_group() then
> proceeds to register this group using configfs_register_group(), thus
> automatically exposing the function type pecific onfigfs attributes to

specific configfs

> the user. E.g.:
> 
> func0/
> |-- baseclass_code
> |-- cache_line_size
> |-- ...
> |-- pci_epf_ntb.0
> |   |-- db_count
> |   |-- mw1
> |   |-- mw2
> |   |-- mw3
> |   |-- mw4
> |   |-- num_mws
> |   `-- spad_count
> |-- primary
> |-- ...
> `-- vendorid
> 
> With this change, there is no need for the user to create/delete
> directories in the endpoint function configfs directory. The
> pci_epf_type_group_ops group operations are thus removed.
> 

Now you also need a documentation change 

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

With the above comments addressed,

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/pci-ep-cfs.c | 41 ++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index 4b8ac0ac84d5..b16fc6093c20 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -23,6 +23,7 @@ struct pci_epf_group {
>  	struct config_group group;
>  	struct config_group primary_epc_group;
>  	struct config_group secondary_epc_group;
> +	struct config_group *type_group;
>  	struct delayed_work cfs_work;
>  	struct pci_epf *epf;
>  	int index;
> @@ -502,34 +503,28 @@ static struct configfs_item_operations pci_epf_ops = {
>  	.release		= pci_epf_release,
>  };
>  
> -static struct config_group *pci_epf_type_make(struct config_group *group,
> -					      const char *name)
> -{
> -	struct pci_epf_group *epf_group = to_pci_epf_group(&group->cg_item);
> -	struct config_group *epf_type_group;
> -
> -	epf_type_group = pci_epf_type_add_cfs(epf_group->epf, group);
> -	return epf_type_group;
> -}
> -
> -static void pci_epf_type_drop(struct config_group *group,
> -			      struct config_item *item)
> -{
> -	config_item_put(item);
> -}
> -
> -static struct configfs_group_operations pci_epf_type_group_ops = {
> -	.make_group     = &pci_epf_type_make,
> -	.drop_item      = &pci_epf_type_drop,
> -};
> -
>  static const struct config_item_type pci_epf_type = {
> -	.ct_group_ops	= &pci_epf_type_group_ops,
>  	.ct_item_ops	= &pci_epf_ops,
>  	.ct_attrs	= pci_epf_attrs,
>  	.ct_owner	= THIS_MODULE,
>  };
>  
> +static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group)
> +{
> +	struct config_group *group;
> +
> +	group = pci_epf_type_add_cfs(epf_group->epf, &epf_group->group);
> +	if (!group)
> +		return;
> +
> +	if (IS_ERR(group)) {
> +		pr_err("failed to create epf type specific attributes\n");
> +		return;
> +	}
> +
> +	configfs_register_group(&epf_group->group, group);
> +}
> +
>  static void pci_epf_cfs_work(struct work_struct *work)
>  {
>  	struct pci_epf_group *epf_group;
> @@ -547,6 +542,8 @@ static void pci_epf_cfs_work(struct work_struct *work)
>  		pr_err("failed to create 'secondary' EPC interface\n");
>  		return;
>  	}
> +
> +	pci_ep_cfs_add_type_group(epf_group);
>  }
>  
>  static struct config_group *pci_epf_make(struct config_group *group,
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 02/16] PCI: endpoint: Move pci_epf_type_add_cfs() code

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:02:59PM +0900, Damien Le Moal wrote:
> pci_epf_type_add_cfs() is called only from pci_ep_cfs_add_type_group()
> in drivers/pci/endpoint/pci-ep-cfs.c, so there is no need to export this
> function and we can move its code from pci-epf-core.c to pci-ep-cfs.c
> as a static function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/pci-ep-cfs.c   | 20 ++++++++++++++++++
>  drivers/pci/endpoint/pci-epf-core.c | 32 -----------------------------
>  include/linux/pci-epf.h             |  2 --
>  3 files changed, 20 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index b16fc6093c20..3a05e9b5a4e9 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -509,6 +509,26 @@ static const struct config_item_type pci_epf_type = {
>  	.ct_owner	= THIS_MODULE,
>  };
>  
> +static struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
> +						 struct config_group *group)
> +{
> +	struct config_group *epf_type_group;
> +
> +	if (!epf->driver) {
> +		dev_err(&epf->dev, "epf device not bound to driver\n");
> +		return NULL;
> +	}
> +
> +	if (!epf->driver->ops->add_cfs)
> +		return NULL;
> +
> +	mutex_lock(&epf->lock);
> +	epf_type_group = epf->driver->ops->add_cfs(epf, group);
> +	mutex_unlock(&epf->lock);
> +
> +	return epf_type_group;
> +}
> +
>  static void pci_ep_cfs_add_type_group(struct pci_epf_group *epf_group)
>  {
>  	struct config_group *group;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 2036e38be093..355a6f56fcea 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -20,38 +20,6 @@ static DEFINE_MUTEX(pci_epf_mutex);
>  static struct bus_type pci_epf_bus_type;
>  static const struct device_type pci_epf_type;
>  
> -/**
> - * pci_epf_type_add_cfs() - Help function drivers to expose function specific
> - *                          attributes in configfs
> - * @epf: the EPF device that has to be configured using configfs
> - * @group: the parent configfs group (corresponding to entries in
> - *         pci_epf_device_id)
> - *
> - * Invoke to expose function specific attributes in configfs. If the function
> - * driver does not have anything to expose (attributes configured by user),
> - * return NULL.
> - */
> -struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
> -					  struct config_group *group)
> -{
> -	struct config_group *epf_type_group;
> -
> -	if (!epf->driver) {
> -		dev_err(&epf->dev, "epf device not bound to driver\n");
> -		return NULL;
> -	}
> -
> -	if (!epf->driver->ops->add_cfs)
> -		return NULL;
> -
> -	mutex_lock(&epf->lock);
> -	epf_type_group = epf->driver->ops->add_cfs(epf, group);
> -	mutex_unlock(&epf->lock);
> -
> -	return epf_type_group;
> -}
> -EXPORT_SYMBOL_GPL(pci_epf_type_add_cfs);
> -
>  /**
>   * pci_epf_unbind() - Notify the function driver that the binding between the
>   *		      EPF device and EPC device has been lost
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index a215dc8ce693..b8441db2fa52 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -214,8 +214,6 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>  			enum pci_epc_interface_type type);
>  int pci_epf_bind(struct pci_epf *epf);
>  void pci_epf_unbind(struct pci_epf *epf);
> -struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
> -					  struct config_group *group);
>  int pci_epf_add_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
>  void pci_epf_remove_vepf(struct pci_epf *epf_pf, struct pci_epf *epf_vf);
>  #endif /* __LINUX_PCI_EPF_H */
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 03/16] PCI: epf-test: Fix DMA transfer completion initialization

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:00PM +0900, Damien Le Moal wrote:
> Re-initialize the transfer_complete DMA transfer completion before
> calling tx_submit(), to avoid seeing the DMA transfer complete before
> the completion is initialized, thus potentially losing the completion
> notification.
> 
> Fixes: 8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")

Please also CC stable list for backporting

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 0f9d2ec822ac..d65419735d2e 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -151,10 +151,10 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>  		return -EIO;
>  	}
>  
> +	reinit_completion(&epf_test->transfer_complete);
>  	tx->callback = pci_epf_test_dma_callback;
>  	tx->callback_param = epf_test;
>  	cookie = tx->tx_submit(tx);
> -	reinit_completion(&epf_test->transfer_complete);
>  
>  	ret = dma_submit_error(cookie);
>  	if (ret) {
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 04/16] PCI: epf-test: Fix DMA transfer completion detection

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:01PM +0900, Damien Le Moal wrote:
> pci_epf_test_data_transfer() and pci_epf_test_dma_callback() are not
> handling DMA transfer completion correctly, leading to completion
> notifications to the RP side that are too early. This problem can be

Can you say RC which stands for Root Complex instead of RP?

> detected when the RP side is running an IOMMU with messages such as:
> 
> pci-endpoint-test 0000:0b:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT
> domain=0x001c address=0xfff00000 flags=0x0000]
> 
> When running the pcitest.sh tests: the address used for a previous
> test transfer generates the above error while the next test transfer is
> running.
> 
> Fix this by testing the dma transfer status in
> pci_epf_test_dma_callback() and notifying the completion only when the
> transfer status is DMA_COMPLETE or DMA_ERROR. Furthermore, in
> pci_epf_test_data_transfer(), be paranoid and check again the transfer
> status and always call dmaengine_terminate_sync() before returning.
> 
> Fixes: 8353813c88ef ("PCI: endpoint: Enable DMA tests for endpoints with DMA capabilities")
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 40 ++++++++++++++-----
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index d65419735d2e..557fbb91c729 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -54,6 +54,9 @@ struct pci_epf_test {
>  	struct delayed_work	cmd_handler;
>  	struct dma_chan		*dma_chan_tx;
>  	struct dma_chan		*dma_chan_rx;
> +	struct dma_chan		*transfer_chan;
> +	dma_cookie_t		transfer_cookie;
> +	enum dma_status		transfer_status;
>  	struct completion	transfer_complete;
>  	bool			dma_supported;
>  	bool			dma_private;
> @@ -85,8 +88,14 @@ static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
>  static void pci_epf_test_dma_callback(void *param)
>  {
>  	struct pci_epf_test *epf_test = param;
> -
> -	complete(&epf_test->transfer_complete);
> +	struct dma_tx_state state;
> +
> +	epf_test->transfer_status =
> +		dmaengine_tx_status(epf_test->transfer_chan,
> +				    epf_test->transfer_cookie, &state);
> +	if (epf_test->transfer_status == DMA_COMPLETE ||
> +	    epf_test->transfer_status == DMA_ERROR)
> +		complete(&epf_test->transfer_complete);
>  }
>  
>  /**
> @@ -120,7 +129,6 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>  	struct dma_async_tx_descriptor *tx;
>  	struct dma_slave_config sconf = {};
>  	struct device *dev = &epf->dev;
> -	dma_cookie_t cookie;
>  	int ret;
>  
>  	if (IS_ERR_OR_NULL(chan)) {
> @@ -152,25 +160,35 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>  	}
>  
>  	reinit_completion(&epf_test->transfer_complete);
> +	epf_test->transfer_chan = chan;
>  	tx->callback = pci_epf_test_dma_callback;
>  	tx->callback_param = epf_test;
> -	cookie = tx->tx_submit(tx);
> +	epf_test->transfer_cookie = tx->tx_submit(tx);
>  
> -	ret = dma_submit_error(cookie);
> +	ret = dma_submit_error(epf_test->transfer_cookie);
>  	if (ret) {
> -		dev_err(dev, "Failed to do DMA tx_submit %d\n", cookie);
> -		return -EIO;
> +		dev_err(dev, "Failed to do DMA tx_submit %d\n", ret);
> +		goto terminate;
>  	}
>  
>  	dma_async_issue_pending(chan);
>  	ret = wait_for_completion_interruptible(&epf_test->transfer_complete);
>  	if (ret < 0) {
> -		dmaengine_terminate_sync(chan);
> -		dev_err(dev, "DMA wait_for_completion_timeout\n");
> -		return -ETIMEDOUT;
> +		dev_err(dev, "DMA wait_for_completion interrupted\n");
> +		goto terminate;
>  	}
>  
> -	return 0;
> +	if (epf_test->transfer_status == DMA_ERROR) {
> +		dev_err(dev, "DMA transfer failed\n");
> +		ret = -EIO;
> +	}
> +
> +	WARN_ON(epf_test->transfer_status != DMA_COMPLETE);

Why do you need this check? Even if required, WARN_ON is superfluous here.

Thanks,
Mani

> +
> +terminate:
> +	dmaengine_terminate_sync(chan);
> +
> +	return ret;
>  }
>  
>  struct epf_dma_filter {
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 05/16] PCI: epf-test: Use dmaengine_submit() to initiate DMA transfer

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:02PM +0900, Damien Le Moal wrote:
> Instead of an open coded call to the tx_submit() operation of struct
> dma_async_tx_descriptor, use the helper function dmaengine_submit().
> No functional change is introduced with this.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 557fbb91c729..3dce9827bd2a 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -163,7 +163,7 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>  	epf_test->transfer_chan = chan;
>  	tx->callback = pci_epf_test_dma_callback;
>  	tx->callback_param = epf_test;
> -	epf_test->transfer_cookie = tx->tx_submit(tx);
> +	epf_test->transfer_cookie = dmaengine_submit(tx);
>  
>  	ret = dma_submit_error(epf_test->transfer_cookie);
>  	if (ret) {
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 06/16] PCI: epf-test: Simplify read/write/copy test functions

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:03PM +0900, Damien Le Moal wrote:
> The function pci_epf_test_cmd_handler() casts the register bar to a
> struct pci_epf_test_reg to determine the command sent by the host and
> execute the test function accordingly. So there is no need for doing
> this cast again in each of the read, write and copy test functions. We
> can simply pass the reg pointer as an argument to the functions
> pci_epf_test_write(), pci_epf_test_read() and pci_epf_test_copy().
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 21 ++++++++-----------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 3dce9827bd2a..1fc245d79a8e 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -327,7 +327,8 @@ static void pci_epf_test_print_rate(const char *ops, u64 size,
>  		(u64)ts.tv_sec, (u32)ts.tv_nsec, rate / 1024);
>  }
>  
> -static int pci_epf_test_copy(struct pci_epf_test *epf_test)
> +static int pci_epf_test_copy(struct pci_epf_test *epf_test,
> +			     struct pci_epf_test_reg *reg)
>  {
>  	int ret;
>  	bool use_dma;
> @@ -339,8 +340,6 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> -	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> -	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
>  	if (!src_addr) {
> @@ -426,7 +425,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
>  	return ret;
>  }
>  
> -static int pci_epf_test_read(struct pci_epf_test *epf_test)
> +static int pci_epf_test_read(struct pci_epf_test *epf_test,
> +			     struct pci_epf_test_reg *reg)
>  {
>  	int ret;
>  	void __iomem *src_addr;
> @@ -440,8 +440,6 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
>  	struct device *dma_dev = epf->epc->dev.parent;
> -	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> -	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
>  	if (!src_addr) {
> @@ -516,7 +514,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
>  	return ret;
>  }
>  
> -static int pci_epf_test_write(struct pci_epf_test *epf_test)
> +static int pci_epf_test_write(struct pci_epf_test *epf_test,
> +			      struct pci_epf_test_reg *reg)
>  {
>  	int ret;
>  	void __iomem *dst_addr;
> @@ -529,8 +528,6 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
>  	struct device *dma_dev = epf->epc->dev.parent;
> -	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> -	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
>  	if (!dst_addr) {
> @@ -675,7 +672,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	}
>  
>  	if (command & COMMAND_WRITE) {
> -		ret = pci_epf_test_write(epf_test);
> +		ret = pci_epf_test_write(epf_test, reg);
>  		if (ret)
>  			reg->status |= STATUS_WRITE_FAIL;
>  		else
> @@ -686,7 +683,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	}
>  
>  	if (command & COMMAND_READ) {
> -		ret = pci_epf_test_read(epf_test);
> +		ret = pci_epf_test_read(epf_test, reg);
>  		if (!ret)
>  			reg->status |= STATUS_READ_SUCCESS;
>  		else
> @@ -697,7 +694,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	}
>  
>  	if (command & COMMAND_COPY) {
> -		ret = pci_epf_test_copy(epf_test);
> +		ret = pci_epf_test_copy(epf_test, reg);
>  		if (!ret)
>  			reg->status |= STATUS_COPY_SUCCESS;
>  		else
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 07/16] PCI: epf-test: Simply pci_epf_test_raise_irq()

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:04PM +0900, Damien Le Moal wrote:
> Change the interface of the function pci_epf_test_raise_irq() to
> directly pass a pointer to the struct pci_epf_test_reg defining the test
> being executed. This avoids the need for grabbing this pointer with a
> cast of the register bar and simplifies the call sites as the irq type
> and irq numbers do not have to be passed as arguments.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 21 +++++++------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 1fc245d79a8e..6f4ef5251452 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -609,29 +609,27 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
>  	return ret;
>  }
>  
> -static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
> -				   u16 irq)
> +static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> +				   struct pci_epf_test_reg *reg)
>  {
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> -	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> -	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	reg->status |= STATUS_IRQ_RAISED;
>  
> -	switch (irq_type) {
> +	switch (reg->irq_type) {
>  	case IRQ_TYPE_LEGACY:
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
>  				  PCI_EPC_IRQ_LEGACY, 0);
>  		break;
>  	case IRQ_TYPE_MSI:
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSI, irq);
> +				  PCI_EPC_IRQ_MSI, reg->irq_number);
>  		break;
>  	case IRQ_TYPE_MSIX:
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSIX, irq);
> +				  PCI_EPC_IRQ_MSIX, reg->irq_number);
>  		break;
>  	default:
>  		dev_err(dev, "Failed to raise IRQ, unknown type\n");
> @@ -677,8 +675,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  			reg->status |= STATUS_WRITE_FAIL;
>  		else
>  			reg->status |= STATUS_WRITE_SUCCESS;
> -		pci_epf_test_raise_irq(epf_test, reg->irq_type,
> -				       reg->irq_number);
> +		pci_epf_test_raise_irq(epf_test, reg);
>  		goto reset_handler;
>  	}
>  
> @@ -688,8 +685,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  			reg->status |= STATUS_READ_SUCCESS;
>  		else
>  			reg->status |= STATUS_READ_FAIL;
> -		pci_epf_test_raise_irq(epf_test, reg->irq_type,
> -				       reg->irq_number);
> +		pci_epf_test_raise_irq(epf_test, reg);
>  		goto reset_handler;
>  	}
>  
> @@ -699,8 +695,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  			reg->status |= STATUS_COPY_SUCCESS;
>  		else
>  			reg->status |= STATUS_COPY_FAIL;
> -		pci_epf_test_raise_irq(epf_test, reg->irq_type,
> -				       reg->irq_number);
> +		pci_epf_test_raise_irq(epf_test, reg);
>  		goto reset_handler;
>  	}
>  
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 08/16] PCI: epf-test: Simplify IRQ test commands execution

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:05PM +0900, Damien Le Moal wrote:
> For the commands COMMAND_RAISE_LEGACY_IRQ, COMMAND_RAISE_MSI_IRQ and
> COMMAND_RAISE_MSIX_IRQ, the function pci_epf_test_cmd_handler()
> sets the STATUS_IRQ_RAISED status flag and calls the epc function
> pci_epc_raise_irq() directly. However, this is also exactly what the
> pci_epf_test_raise_irq() function does. Avoid duplicating these
> operations by directly using pci_epf_test_raise_irq() for the IRQ test
> commands. It is OK to do so as the host side endpoint test driver always
> set the correct irq type for the IRQ test commands.
> 
> At the same time, the irq number check done for the
> COMMAND_RAISE_MSI_IRQ and COMMAND_RAISE_MSIX_IRQ commands can also be
> moved to pci_epf_test_raise_irq() to also check the IRQ number requested
> by the host for other test commands.
> 
> Overall, this significantly simplifies the pci_epf_test_cmd_handler()
> function.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 43 ++++++++-----------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 6f4ef5251452..43d623682850 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -615,6 +615,7 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	int count;
>  
>  	reg->status |= STATUS_IRQ_RAISED;
>  
> @@ -624,10 +625,22 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  				  PCI_EPC_IRQ_LEGACY, 0);
>  		break;
>  	case IRQ_TYPE_MSI:
> +		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
> +		if (reg->irq_number > count || count <= 0) {
> +			dev_err(dev, "Invalid MSI IRQ number %d / %d\n",
> +				reg->irq_number, count);
> +			return;
> +		}
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
>  				  PCI_EPC_IRQ_MSI, reg->irq_number);
>  		break;
>  	case IRQ_TYPE_MSIX:
> +		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
> +		if (reg->irq_number > count || count <= 0) {
> +			dev_err(dev, "Invalid MSIX IRQ number %d / %d\n",
> +				reg->irq_number, count);
> +			return;
> +		}
>  		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
>  				  PCI_EPC_IRQ_MSIX, reg->irq_number);
>  		break;
> @@ -640,13 +653,11 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  static void pci_epf_test_cmd_handler(struct work_struct *work)
>  {
>  	int ret;
> -	int count;
>  	u32 command;
>  	struct pci_epf_test *epf_test = container_of(work, struct pci_epf_test,
>  						     cmd_handler.work);
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
> -	struct pci_epc *epc = epf->epc;
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
> @@ -662,10 +673,10 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  		goto reset_handler;
>  	}
>  
> -	if (command & COMMAND_RAISE_LEGACY_IRQ) {
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_LEGACY, 0);
> +	if ((command & COMMAND_RAISE_LEGACY_IRQ) ||
> +	    (command & COMMAND_RAISE_MSI_IRQ) ||
> +	    (command & COMMAND_RAISE_MSIX_IRQ)) {
> +		pci_epf_test_raise_irq(epf_test, reg);
>  		goto reset_handler;
>  	}
>  
> @@ -699,26 +710,6 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  		goto reset_handler;
>  	}
>  
> -	if (command & COMMAND_RAISE_MSI_IRQ) {
> -		count = pci_epc_get_msi(epc, epf->func_no, epf->vfunc_no);
> -		if (reg->irq_number > count || count <= 0)
> -			goto reset_handler;
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSI, reg->irq_number);
> -		goto reset_handler;
> -	}
> -
> -	if (command & COMMAND_RAISE_MSIX_IRQ) {
> -		count = pci_epc_get_msix(epc, epf->func_no, epf->vfunc_no);
> -		if (reg->irq_number > count || count <= 0)
> -			goto reset_handler;
> -		reg->status = STATUS_IRQ_RAISED;
> -		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
> -				  PCI_EPC_IRQ_MSIX, reg->irq_number);
> -		goto reset_handler;
> -	}
> -
>  reset_handler:
>  	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
>  			   msecs_to_jiffies(1));
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote:
> The pci-epf-test driver uses the test register bar memory directly
> to get and execute a test registers set by the RC side and defined
> using a struct pci_epf_test_reg. This direct use relies on a casts of
> the register bar to get a pointer to a struct pci_epf_test_reg to
> execute the test case and sending back the test result through the
> status field of struct pci_epf_test_reg. In practice, the status field
> is always updated before an interrupt is raised in
> pci_epf_test_raise_irq(), to ensure that the RC side sees the updated
> status when receiving the interrupts.
> 
> However, such cast-based direct access does not ensure that changes to
> the status register make it to memory, and so visible to the host,
> before an interrupt is raised, thus potentially resulting in the RC host
> not seeing the correct status result for a test.
> 
> Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when
> accessing the command and status fields of a pci_epf_test_reg structure.
> This ensure that a test start (pci_epf_test_cmd_handler() function) and
> completion (with the function pci_epf_test_raise_irq()) achive a correct
> synchronization with the host side mmio register accesses.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 43d623682850..e0cf8c2bf6db 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	u32 status = reg->status | STATUS_IRQ_RAISED;
>  	int count;
>  
> -	reg->status |= STATUS_IRQ_RAISED;
> +	/*
> +	 * Set the status before raising the IRQ to ensure that the host sees
> +	 * the updated value when it gets the IRQ.
> +	 */
> +	WRITE_ONCE(reg->status, status);

For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write
has reached the memory (it could be stored in a write buffer). If you really
care about synchronization, then you should do a read back of the variable.

Thanks,
Mani

>  
>  	switch (reg->irq_type) {
>  	case IRQ_TYPE_LEGACY:
> @@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
> -	command = reg->command;
> +	command = READ_ONCE(reg->command);
>  	if (!command)
>  		goto reset_handler;
>  
> -	reg->command = 0;
> -	reg->status = 0;
> +	WRITE_ONCE(reg->command, 0);
> +	WRITE_ONCE(reg->status, 0);
>  
>  	if (reg->irq_type > IRQ_TYPE_MSIX) {
>  		dev_err(dev, "Failed to detect IRQ type\n");
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 10/16] PCI: epf-test: Cleanup pci_epf_test_cmd_handler()

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:07PM +0900, Damien Le Moal wrote:
> Command codes are never combined together as flags into a single value.
> Thus we can replace the series of "if" tests in
> pci_epf_test_cmd_handler() with a cleaner switch-case statement.
> This also allows checking that we got a valid command and print an error
> message if we did not.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 30 +++++++++----------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index e0cf8c2bf6db..d1b5441391fb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -678,41 +678,39 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  		goto reset_handler;
>  	}
>  
> -	if ((command & COMMAND_RAISE_LEGACY_IRQ) ||
> -	    (command & COMMAND_RAISE_MSI_IRQ) ||
> -	    (command & COMMAND_RAISE_MSIX_IRQ)) {
> +	switch (command) {
> +	case COMMAND_RAISE_LEGACY_IRQ:
> +	case COMMAND_RAISE_MSI_IRQ:
> +	case COMMAND_RAISE_MSIX_IRQ:
>  		pci_epf_test_raise_irq(epf_test, reg);
> -		goto reset_handler;
> -	}
> -
> -	if (command & COMMAND_WRITE) {
> +		break;
> +	case COMMAND_WRITE:
>  		ret = pci_epf_test_write(epf_test, reg);
>  		if (ret)
>  			reg->status |= STATUS_WRITE_FAIL;
>  		else
>  			reg->status |= STATUS_WRITE_SUCCESS;
>  		pci_epf_test_raise_irq(epf_test, reg);
> -		goto reset_handler;
> -	}
> -
> -	if (command & COMMAND_READ) {
> +		break;
> +	case COMMAND_READ:
>  		ret = pci_epf_test_read(epf_test, reg);
>  		if (!ret)
>  			reg->status |= STATUS_READ_SUCCESS;
>  		else
>  			reg->status |= STATUS_READ_FAIL;
>  		pci_epf_test_raise_irq(epf_test, reg);
> -		goto reset_handler;
> -	}
> -
> -	if (command & COMMAND_COPY) {
> +		break;
> +	case COMMAND_COPY:
>  		ret = pci_epf_test_copy(epf_test, reg);
>  		if (!ret)
>  			reg->status |= STATUS_COPY_SUCCESS;
>  		else
>  			reg->status |= STATUS_COPY_FAIL;
>  		pci_epf_test_raise_irq(epf_test, reg);
> -		goto reset_handler;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid command\n");
> +		break;
>  	}
>  
>  reset_handler:
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 14/16] misc: pci_endpoint_test: Re-init completion for every test

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:11PM +0900, Damien Le Moal wrote:
> The irq_raised completion used to detect the end of a test case is
> initialized when the test device is probed, but never reinitialized
> again before a test case. As a result, the irq_raised completion
> synchronization is effective only for the first ioctl test case
> executed. Any subsequent call to wait_for_completion() by another
> ioctl() call will immediately return, potentially too early, leading to
> false positive failures.
> 
> Fix this by reinitializing the irq_raised completion before starting a
> new ioctl() test command.
> 
> Fixes: 2c156ac71c6b ("misc: Add host side PCI driver for PCI test function device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/misc/pci_endpoint_test.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 01235236e9bc..24efe3b88a1f 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -729,6 +729,10 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  	struct pci_dev *pdev = test->pdev;
>  
>  	mutex_lock(&test->mutex);
> +
> +	reinit_completion(&test->irq_raised);
> +	test->last_irq = -ENODATA;
> +
>  	switch (cmd) {
>  	case PCITEST_BAR:
>  		bar = arg;
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 11/16] PCI: epf-test: Simplify dma support checks

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:08PM +0900, Damien Le Moal wrote:
> There is no need to have each read, write and copy test functions check
> for the FLAG_USE_DMA flag against the dma support status indicated by
> epf_test->dma_supported. Move this test to the command handler function
> pci_epf_test_cmd_handler() to check once for all cases.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 45 +++++++------------
>  1 file changed, 15 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index d1b5441391fb..eaa252a170a2 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -331,7 +331,6 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
>  			     struct pci_epf_test_reg *reg)
>  {
>  	int ret;
> -	bool use_dma;
>  	void __iomem *src_addr;
>  	void __iomem *dst_addr;
>  	phys_addr_t src_phys_addr;
> @@ -374,14 +373,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
>  	}
>  
>  	ktime_get_ts64(&start);
> -	use_dma = !!(reg->flags & FLAG_USE_DMA);
> -	if (use_dma) {
> -		if (!epf_test->dma_supported) {
> -			dev_err(dev, "Cannot transfer data using DMA\n");
> -			ret = -EINVAL;
> -			goto err_map_addr;
> -		}
> -
> +	if (reg->flags & FLAG_USE_DMA) {
>  		if (epf_test->dma_private) {
>  			dev_err(dev, "Cannot transfer data using DMA\n");
>  			ret = -EINVAL;
> @@ -407,7 +399,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test,
>  		kfree(buf);
>  	}
>  	ktime_get_ts64(&end);
> -	pci_epf_test_print_rate("COPY", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate("COPY", reg->size, &start, &end,
> +				reg->flags & FLAG_USE_DMA);
>  
>  err_map_addr:
>  	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
> @@ -432,7 +425,6 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
>  	void __iomem *src_addr;
>  	void *buf;
>  	u32 crc32;
> -	bool use_dma;
>  	phys_addr_t phys_addr;
>  	phys_addr_t dst_phys_addr;
>  	struct timespec64 start, end;
> @@ -463,14 +455,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
>  		goto err_map_addr;
>  	}
>  
> -	use_dma = !!(reg->flags & FLAG_USE_DMA);
> -	if (use_dma) {
> -		if (!epf_test->dma_supported) {
> -			dev_err(dev, "Cannot transfer data using DMA\n");
> -			ret = -EINVAL;
> -			goto err_dma_map;
> -		}
> -
> +	if (reg->flags & FLAG_USE_DMA) {
>  		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
>  					       DMA_FROM_DEVICE);
>  		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> @@ -495,7 +480,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test,
>  		ktime_get_ts64(&end);
>  	}
>  
> -	pci_epf_test_print_rate("READ", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate("READ", reg->size, &start, &end,
> +				reg->flags & FLAG_USE_DMA);
>  
>  	crc32 = crc32_le(~0, buf, reg->size);
>  	if (crc32 != reg->checksum)
> @@ -520,7 +506,6 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
>  	int ret;
>  	void __iomem *dst_addr;
>  	void *buf;
> -	bool use_dma;
>  	phys_addr_t phys_addr;
>  	phys_addr_t src_phys_addr;
>  	struct timespec64 start, end;
> @@ -554,14 +539,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
>  	get_random_bytes(buf, reg->size);
>  	reg->checksum = crc32_le(~0, buf, reg->size);
>  
> -	use_dma = !!(reg->flags & FLAG_USE_DMA);
> -	if (use_dma) {
> -		if (!epf_test->dma_supported) {
> -			dev_err(dev, "Cannot transfer data using DMA\n");
> -			ret = -EINVAL;
> -			goto err_dma_map;
> -		}
> -
> +	if (reg->flags & FLAG_USE_DMA) {
>  		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
>  					       DMA_TO_DEVICE);
>  		if (dma_mapping_error(dma_dev, src_phys_addr)) {
> @@ -588,7 +566,8 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test,
>  		ktime_get_ts64(&end);
>  	}
>  
> -	pci_epf_test_print_rate("WRITE", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate("WRITE", reg->size, &start, &end,
> +				reg->flags & FLAG_USE_DMA);
>  
>  	/*
>  	 * wait 1ms inorder for the write to complete. Without this delay L3
> @@ -673,6 +652,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	WRITE_ONCE(reg->command, 0);
>  	WRITE_ONCE(reg->status, 0);
>  
> +	if ((READ_ONCE(reg->flags) & FLAG_USE_DMA) &&
> +	    !epf_test->dma_supported) {
> +		dev_err(dev, "Cannot transfer data using DMA\n");
> +		goto reset_handler;
> +	}
> +
>  	if (reg->irq_type > IRQ_TYPE_MSIX) {
>  		dev_err(dev, "Failed to detect IRQ type\n");
>  		goto reset_handler;
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 04/16] PCI: epf-test: Fix DMA transfer completion detection

[Damien Le Moal]

On 2023/03/16 0:20, Manivannan Sadhasivam wrote:
>> @@ -152,25 +160,35 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>>  	}
>>  
>>  	reinit_completion(&epf_test->transfer_complete);
>> +	epf_test->transfer_chan = chan;
>>  	tx->callback = pci_epf_test_dma_callback;
>>  	tx->callback_param = epf_test;
>> -	cookie = tx->tx_submit(tx);
>> +	epf_test->transfer_cookie = tx->tx_submit(tx);
>>  
>> -	ret = dma_submit_error(cookie);
>> +	ret = dma_submit_error(epf_test->transfer_cookie);
>>  	if (ret) {
>> -		dev_err(dev, "Failed to do DMA tx_submit %d\n", cookie);
>> -		return -EIO;
>> +		dev_err(dev, "Failed to do DMA tx_submit %d\n", ret);
>> +		goto terminate;
>>  	}
>>  
>>  	dma_async_issue_pending(chan);
>>  	ret = wait_for_completion_interruptible(&epf_test->transfer_complete);
>>  	if (ret < 0) {
>> -		dmaengine_terminate_sync(chan);
>> -		dev_err(dev, "DMA wait_for_completion_timeout\n");
>> -		return -ETIMEDOUT;
>> +		dev_err(dev, "DMA wait_for_completion interrupted\n");
>> +		goto terminate;
>>  	}
>>  
>> -	return 0;
>> +	if (epf_test->transfer_status == DMA_ERROR) {
>> +		dev_err(dev, "DMA transfer failed\n");
>> +		ret = -EIO;
>> +	}
>> +
>> +	WARN_ON(epf_test->transfer_status != DMA_COMPLETE);
> 
> Why do you need this check? Even if required, WARN_ON is superfluous here.

The check is needed to return -EIO if there was a problem with the transfer,
because wait_for_completion_interruptible() does not notify such errors (it only
notifies timeouts).

And yes, the WARN can go away. Will remove it.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Damien Le Moal]

On 2023/03/16 0:51, Manivannan Sadhasivam wrote:
> On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote:
>> The pci-epf-test driver uses the test register bar memory directly
>> to get and execute a test registers set by the RC side and defined
>> using a struct pci_epf_test_reg. This direct use relies on a casts of
>> the register bar to get a pointer to a struct pci_epf_test_reg to
>> execute the test case and sending back the test result through the
>> status field of struct pci_epf_test_reg. In practice, the status field
>> is always updated before an interrupt is raised in
>> pci_epf_test_raise_irq(), to ensure that the RC side sees the updated
>> status when receiving the interrupts.
>>
>> However, such cast-based direct access does not ensure that changes to
>> the status register make it to memory, and so visible to the host,
>> before an interrupt is raised, thus potentially resulting in the RC host
>> not seeing the correct status result for a test.
>>
>> Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when
>> accessing the command and status fields of a pci_epf_test_reg structure.
>> This ensure that a test start (pci_epf_test_cmd_handler() function) and
>> completion (with the function pci_epf_test_raise_irq()) achive a correct
>> synchronization with the host side mmio register accesses.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index 43d623682850..e0cf8c2bf6db 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>>  	struct pci_epf *epf = epf_test->epf;
>>  	struct device *dev = &epf->dev;
>>  	struct pci_epc *epc = epf->epc;
>> +	u32 status = reg->status | STATUS_IRQ_RAISED;
>>  	int count;
>>  
>> -	reg->status |= STATUS_IRQ_RAISED;
>> +	/*
>> +	 * Set the status before raising the IRQ to ensure that the host sees
>> +	 * the updated value when it gets the IRQ.
>> +	 */
>> +	WRITE_ONCE(reg->status, status);
> 
> For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write
> has reached the memory (it could be stored in a write buffer). If you really
> care about synchronization, then you should do a read back of the variable.

WRITE_ONCE() does an explicit cast to volatile. So unless the compiler is
seriously buggy, the value should make it to memory. Sure it could be in the CPU
cache, but given that this is memory allocated from dma coherent, I am not sure
that can happen. Or am I missing something ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Arnd Bergmann]

On Wed, Mar 15, 2023, at 16:51, Manivannan Sadhasivam wrote:
> On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote:
>> +	/*
>> +	 * Set the status before raising the IRQ to ensure that the host sees
>> +	 * the updated value when it gets the IRQ.
>> +	 */
>> +	WRITE_ONCE(reg->status, status);
>
> For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write
> has reached the memory (it could be stored in a write buffer). If you really
> care about synchronization, then you should do a read back of the variable.

This is not MMIO, this is the local access to a variable that is accessed
through MMIO from the remote host. Reading it back would not change anything
here as far as I can tell, 

      Arnd

Re: [PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Manivannan Sadhasivam]

On Thu, Mar 16, 2023 at 04:25:58PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 15, 2023, at 16:51, Manivannan Sadhasivam wrote:
> > On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote:
> >> +	/*
> >> +	 * Set the status before raising the IRQ to ensure that the host sees
> >> +	 * the updated value when it gets the IRQ.
> >> +	 */
> >> +	WRITE_ONCE(reg->status, status);
> >
> > For MMIO, it is not sufficient to use WRITE_ONCE() and expect that the write
> > has reached the memory (it could be stored in a write buffer). If you really
> > care about synchronization, then you should do a read back of the variable.
> 
> This is not MMIO, this is the local access to a variable that is accessed
> through MMIO from the remote host. Reading it back would not change anything
> here as far as I can tell, 
> 

Ah, sorry I got confused this with my EPF driver... Yeah for a variable
WRITE_ONCE is sufficient.

Thanks,
Mani

>       Arnd

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 09/16] PCI: epf-test: Improve handling of command and status registers

[Manivannan Sadhasivam]

On Wed, Mar 08, 2023 at 06:03:06PM +0900, Damien Le Moal wrote:
> The pci-epf-test driver uses the test register bar memory directly
> to get and execute a test registers set by the RC side and defined
> using a struct pci_epf_test_reg. This direct use relies on a casts of
> the register bar to get a pointer to a struct pci_epf_test_reg to
> execute the test case and sending back the test result through the
> status field of struct pci_epf_test_reg. In practice, the status field
> is always updated before an interrupt is raised in
> pci_epf_test_raise_irq(), to ensure that the RC side sees the updated
> status when receiving the interrupts.
> 
> However, such cast-based direct access does not ensure that changes to
> the status register make it to memory, and so visible to the host,
> before an interrupt is raised, thus potentially resulting in the RC host
> not seeing the correct status result for a test.
> 
> Avoid this potential problem by using READ_ONCE()/WRITE_ONCE() when
> accessing the command and status fields of a pci_epf_test_reg structure.
> This ensure that a test start (pci_epf_test_cmd_handler() function) and
> completion (with the function pci_epf_test_raise_irq()) achive a correct
> synchronization with the host side mmio register accesses.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 43d623682850..e0cf8c2bf6db 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -615,9 +615,14 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
>  	struct pci_epf *epf = epf_test->epf;
>  	struct device *dev = &epf->dev;
>  	struct pci_epc *epc = epf->epc;
> +	u32 status = reg->status | STATUS_IRQ_RAISED;
>  	int count;
>  
> -	reg->status |= STATUS_IRQ_RAISED;
> +	/*
> +	 * Set the status before raising the IRQ to ensure that the host sees
> +	 * the updated value when it gets the IRQ.
> +	 */
> +	WRITE_ONCE(reg->status, status);
>  
>  	switch (reg->irq_type) {
>  	case IRQ_TYPE_LEGACY:
> @@ -661,12 +666,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
> -	command = reg->command;
> +	command = READ_ONCE(reg->command);
>  	if (!command)
>  		goto reset_handler;
>  
> -	reg->command = 0;
> -	reg->status = 0;
> +	WRITE_ONCE(reg->command, 0);
> +	WRITE_ONCE(reg->status, 0);
>  
>  	if (reg->irq_type > IRQ_TYPE_MSIX) {
>  		dev_err(dev, "Failed to detect IRQ type\n");
> -- 
> 2.39.2
> 

-- 
மணிவண்ணன் சதாசிவம்