[PATCH 00/12] PCI endpoint fixes and improvements

[PATCH 00/12] 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 the driver attribute group directory multiple times.

The following patches are fixes and improvements for the endpoint test
drivers, EP side and host 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/

Damien Le Moal (12):
  pci: endpoint: Automatically create a function type attributes group
  pci: endpoint: do not export pci_epf_type_add_cfs()
  pci: epf-test: Fix DMA transfer completion detection
  pci: epf-test: Use driver registers as volatile
  pci: epf-test: Simplify dma support checks
  pci: epf-test: Simplify transfers result print
  pci: epf-test: Add debug and error messages
  misc: pci_endpoint_test: Free IRQs before removing the device
  misc: pci_endpoint_test: Do not write status in IRQ handler
  misc: pci_endpoint_test: Re-init completion for every test
  misc: pci_endpoint_test: Simplify pci_endpoint_test_msi_irq()
  misc: pci_endpoint_test: Add debug and error messages

 drivers/misc/pci_endpoint_test.c              |  51 +++--
 drivers/pci/endpoint/functions/pci-epf-test.c | 207 +++++++++++-------
 drivers/pci/endpoint/pci-ep-cfs.c             |  44 ++--
 drivers/pci/endpoint/pci-epf-core.c           |  12 +-
 drivers/pci/endpoint/pci-epf.h                |  14 ++
 include/linux/pci-epf.h                       |   2 -
 6 files changed, 197 insertions(+), 133 deletions(-)
 create mode 100644 drivers/pci/endpoint/pci-epf.h

-- 
2.39.1

[PATCH 01/12] pci: endpoint: Automatically create a function type attributes group

[Damien Le Moal]

A PCI endpoint function driver can define function specific attributes
using the add_cfs() endpoint driver operation. However, this attributes
group is not created automatically when the function is created and
rather relies on the user creating a directory within the endpoint
function configfs directory to initialize the attributes.

While working, this approach is dangerous as nothing prevents the user
from creating multiple directories with differenti (wrong) names that
all will contain the same attributes.

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 from the 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 configfs
attributes to the user.

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 d4850bdd837f..1fb31f07199f 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.1

Re: [PATCH 01/12] pci: endpoint: Automatically create a function type attributes group

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:44PM +0900, Damien Le Moal wrote:
> A PCI endpoint function driver can define function specific attributes
> using the add_cfs() endpoint driver operation. However, this attributes
> group is not created automatically when the function is created and
> rather relies on the user creating a directory within the endpoint
> function configfs directory to initialize the attributes.
> 

This is not accurate. An endpoint function will only be created when a
directory is created under /sys/kernel/debug/pci_ep/functions/<func_name>/

Here, func_name is just a debugfs group created during EPF registration and
doesn't represent a function unless a subdirectory is created.

> While working, this approach is dangerous as nothing prevents the user
> from creating multiple directories with differenti (wrong) names that
> all will contain the same attributes.
> 
> 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 from the 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 configfs
> attributes to the user.
> 
> 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.
> 

No. You are just automating the creation of sub-directories specific to
functions such as NTB. Users still need to create a directory to create an
actual endpoint function.

The commit message sounds like it is automating the whole endpoint function
creation which it is not doing.

Thanks,
Mani

> 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 d4850bdd837f..1fb31f07199f 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.1
> 

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

Re: [PATCH 01/12] pci: endpoint: Automatically create a function type attributes group

[Damien Le Moal]

On 2/16/23 19:04, Manivannan Sadhasivam wrote:
> On Wed, Feb 15, 2023 at 12:21:44PM +0900, Damien Le Moal wrote:
>> A PCI endpoint function driver can define function specific attributes
>> using the add_cfs() endpoint driver operation. However, this attributes
>> group is not created automatically when the function is created and
>> rather relies on the user creating a directory within the endpoint
>> function configfs directory to initialize the attributes.
>>
> 
> This is not accurate. An endpoint function will only be created when a
> directory is created under /sys/kernel/debug/pci_ep/functions/<func_name>/
> 
> Here, func_name is just a debugfs group created during EPF registration and
> doesn't represent a function unless a subdirectory is created.
> 
>> While working, this approach is dangerous as nothing prevents the user
>> from creating multiple directories with differenti (wrong) names that
>> all will contain the same attributes.
>>
>> 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 from the 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 configfs
>> attributes to the user.
>>
>> 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.
>>
> 
> No. You are just automating the creation of sub-directories specific to
> functions such as NTB. Users still need to create a directory to create an
> actual endpoint function.
> 
> The commit message sounds like it is automating the whole endpoint function
> creation which it is not doing.

OK. I was not clear in the wording. It is not the function directory that this
is automating. It is not about:

/sys/kernel/configfs/pci_ep/functions/<func_name>/

It is about automating the creation of the sub-directory under that that
represent the attribute group that the function defines. So it is about:

/sys/kernel/configfs/pci_ep/functions/<func_name>/<whatever-function-specific>

That directory is *not* created automatically, the user must create it, but
worse than that, can do it multiple times with really bad results when
everything is tore down (I got an oops due to bad reference counts).

This patch fixes that, not the function directory creation itself.

> 
> Thanks,
> Mani
> 
>> 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 d4850bdd837f..1fb31f07199f 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.1
>>
> 

-- 
Damien Le Moal
Western Digital Research

[PATCH 02/12] pci: endpoint: do not export pci_epf_type_add_cfs()

[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 it
and function drivers should not call this function directly.

Remove the export for this function and move its declaration to the
internal header file drivers/pci/endpoint/pci-epf.h.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/pci/endpoint/pci-ep-cfs.c   |  3 ++-
 drivers/pci/endpoint/pci-epf-core.c | 12 +++++-------
 drivers/pci/endpoint/pci-epf.h      | 14 ++++++++++++++
 include/linux/pci-epf.h             |  2 --
 4 files changed, 21 insertions(+), 10 deletions(-)
 create mode 100644 drivers/pci/endpoint/pci-epf.h

diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index 1fb31f07199f..62b3b9e306fa 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -11,9 +11,10 @@
 #include <linux/slab.h>
 
 #include <linux/pci-epc.h>
-#include <linux/pci-epf.h>
 #include <linux/pci-ep-cfs.h>
 
+#include "pci-epf.h"
+
 static DEFINE_IDR(functions_idr);
 static DEFINE_MUTEX(functions_mutex);
 static struct config_group *functions_group;
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index 9ed556936f48..db121a58a586 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -12,24 +12,23 @@
 #include <linux/module.h>
 
 #include <linux/pci-epc.h>
-#include <linux/pci-epf.h>
 #include <linux/pci-ep-cfs.h>
 
+#include "pci-epf.h"
+
 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
+ * pci_epf_type_add_cfs() - Get a function driver specific attribute group.
  * @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.
+ * Called from pci_ep_cfs_add_type_group() when the function is created.
+ * If the function driver does not have anything to expose, return NULL.
  */
 struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
 					  struct config_group *group)
@@ -50,7 +49,6 @@ struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
 
 	return epf_type_group;
 }
-EXPORT_SYMBOL_GPL(pci_epf_type_add_cfs);
 
 /**
  * pci_epf_unbind() - Notify the function driver that the binding between the
diff --git a/drivers/pci/endpoint/pci-epf.h b/drivers/pci/endpoint/pci-epf.h
new file mode 100644
index 000000000000..b2f351afd623
--- /dev/null
+++ b/drivers/pci/endpoint/pci-epf.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PCI Endpoint *Function* (EPF) internal header file
+ */
+
+#ifndef PCI_EPF_H
+#define PCI_EPF_H
+
+#include <linux/pci-epf.h>
+
+struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
+					  struct config_group *group);
+
+#endif /* PCI_EPF_H */
diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
index 009a07147c61..b89cd8515073 100644
--- a/include/linux/pci-epf.h
+++ b/include/linux/pci-epf.h
@@ -209,8 +209,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.1

Re: [PATCH 02/12] pci: endpoint: do not export pci_epf_type_add_cfs()

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:45PM +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 it
> and function drivers should not call this function directly.
> 

Where is the pci_ep_cfs_add_type_group() function defined?

> Remove the export for this function and move its declaration to the
> internal header file drivers/pci/endpoint/pci-epf.h.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/pci-ep-cfs.c   |  3 ++-
>  drivers/pci/endpoint/pci-epf-core.c | 12 +++++-------
>  drivers/pci/endpoint/pci-epf.h      | 14 ++++++++++++++
>  include/linux/pci-epf.h             |  2 --
>  4 files changed, 21 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/pci/endpoint/pci-epf.h
> 
> diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
> index 1fb31f07199f..62b3b9e306fa 100644
> --- a/drivers/pci/endpoint/pci-ep-cfs.c
> +++ b/drivers/pci/endpoint/pci-ep-cfs.c
> @@ -11,9 +11,10 @@
>  #include <linux/slab.h>
>  
>  #include <linux/pci-epc.h>
> -#include <linux/pci-epf.h>
>  #include <linux/pci-ep-cfs.h>
>  
> +#include "pci-epf.h"
> +
>  static DEFINE_IDR(functions_idr);
>  static DEFINE_MUTEX(functions_mutex);
>  static struct config_group *functions_group;
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index 9ed556936f48..db121a58a586 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -12,24 +12,23 @@
>  #include <linux/module.h>
>  
>  #include <linux/pci-epc.h>
> -#include <linux/pci-epf.h>
>  #include <linux/pci-ep-cfs.h>
>  
> +#include "pci-epf.h"
> +
>  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
> + * pci_epf_type_add_cfs() - Get a function driver specific attribute group.
>   * @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.
> + * Called from pci_ep_cfs_add_type_group() when the function is created.
> + * If the function driver does not have anything to expose, return NULL.
>   */
>  struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
>  					  struct config_group *group)
> @@ -50,7 +49,6 @@ struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
>  
>  	return epf_type_group;
>  }
> -EXPORT_SYMBOL_GPL(pci_epf_type_add_cfs);
>  
>  /**
>   * pci_epf_unbind() - Notify the function driver that the binding between the
> diff --git a/drivers/pci/endpoint/pci-epf.h b/drivers/pci/endpoint/pci-epf.h
> new file mode 100644
> index 000000000000..b2f351afd623
> --- /dev/null
> +++ b/drivers/pci/endpoint/pci-epf.h

When there is already a pci-epf.h header available, creating one more even
under different location will create ambiguity. Please rename it as internal.h
or something relevant.

Thanks,
Mani

> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PCI Endpoint *Function* (EPF) internal header file
> + */
> +
> +#ifndef PCI_EPF_H
> +#define PCI_EPF_H
> +
> +#include <linux/pci-epf.h>
> +
> +struct config_group *pci_epf_type_add_cfs(struct pci_epf *epf,
> +					  struct config_group *group);
> +
> +#endif /* PCI_EPF_H */
> diff --git a/include/linux/pci-epf.h b/include/linux/pci-epf.h
> index 009a07147c61..b89cd8515073 100644
> --- a/include/linux/pci-epf.h
> +++ b/include/linux/pci-epf.h
> @@ -209,8 +209,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.1
> 

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

Re: [PATCH 02/12] pci: endpoint: do not export pci_epf_type_add_cfs()

[Damien Le Moal]

On 2/16/23 19:15, Manivannan Sadhasivam wrote:
> On Wed, Feb 15, 2023 at 12:21:45PM +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 it
>> and function drivers should not call this function directly.
>>
> 
> Where is the pci_ep_cfs_add_type_group() function defined?

In patch 1.


>>  /**
>>   * pci_epf_unbind() - Notify the function driver that the binding between the
>> diff --git a/drivers/pci/endpoint/pci-epf.h b/drivers/pci/endpoint/pci-epf.h
>> new file mode 100644
>> index 000000000000..b2f351afd623
>> --- /dev/null
>> +++ b/drivers/pci/endpoint/pci-epf.h
> 
> When there is already a pci-epf.h header available, creating one more even
> under different location will create ambiguity. Please rename it as internal.h
> or something relevant.

OK.

-- 
Damien Le Moal
Western Digital Research

[PATCH 03/12] 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 RC side that are too early. This problem can be
detected when the RC 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.

While at it, also modify the channel tx submit call to use
dmaengine_submit() instead of the hard coded call to the tx_submit()
operation.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 55283d2379a6..030769893efb 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)) {
@@ -151,26 +159,36 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
 		return -EIO;
 	}
 
+	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);
-	reinit_completion(&epf_test->transfer_complete);
+	epf_test->transfer_cookie = dmaengine_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.1

Re: [PATCH 03/12] pci: epf-test: Fix DMA transfer completion detection

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:46PM +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 RC side that are too early. This problem can be
> detected when the RC 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.
> 
> While at it, also modify the channel tx submit call to use
> dmaengine_submit() instead of the hard coded call to the tx_submit()
> operation.
> 

This patch is doing 3 different things. So you need to split them into separate
patches.

Thanks,
Mani

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 42 +++++++++++++------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 55283d2379a6..030769893efb 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)) {
> @@ -151,26 +159,36 @@ static int pci_epf_test_data_transfer(struct pci_epf_test *epf_test,
>  		return -EIO;
>  	}
>  
> +	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);
> -	reinit_completion(&epf_test->transfer_complete);
> +	epf_test->transfer_cookie = dmaengine_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.1
> 

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

[PATCH 04/12] pci: epf-test: Use driver registers as volatile

[Damien Le Moal]

The pci-epf-test driver uses struct pci_epf_test_reg directly from the
test register bar memory to execute tests cases sent by the RC side.
Make sure to declare pci_epf_test_reg use as volatile to ensure that
modifications to the fields of that structure make it to memory to be
seen by the RC side on completion.

Also initialize the test register bar to 0 when it is allocated.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 030769893efb..df3074667bbc 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -340,7 +340,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
 	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];
+	volatile 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) {
@@ -441,7 +441,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
 	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];
+	volatile 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) {
@@ -530,7 +530,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
 	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];
+	volatile 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) {
@@ -619,7 +619,7 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
 	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];
+	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	reg->status |= STATUS_IRQ_RAISED;
 
@@ -653,7 +653,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	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];
+	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
 	command = reg->command;
 	if (!command)
@@ -911,6 +911,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 		dev_err(dev, "Failed to allocated register space\n");
 		return -ENOMEM;
 	}
+	memset(base, 0, test_reg_size);
 	epf_test->reg[test_reg_bar] = base;
 
 	for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
-- 
2.39.1

Re: [PATCH 04/12] pci: epf-test: Use driver registers as volatile

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:47PM +0900, Damien Le Moal wrote:
> The pci-epf-test driver uses struct pci_epf_test_reg directly from the
> test register bar memory to execute tests cases sent by the RC side.
> Make sure to declare pci_epf_test_reg use as volatile to ensure that
> modifications to the fields of that structure make it to memory to be
> seen by the RC side on completion.
> 
> Also initialize the test register bar to 0 when it is allocated.
> 

Again, please split this into a separate commit.

Rest LGTM!

Thanks,
Mani

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index 030769893efb..df3074667bbc 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -340,7 +340,7 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test)
>  	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];
> +	volatile 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) {
> @@ -441,7 +441,7 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test)
>  	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];
> +	volatile 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) {
> @@ -530,7 +530,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test)
>  	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];
> +	volatile 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) {
> @@ -619,7 +619,7 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
>  	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];
> +	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	reg->status |= STATUS_IRQ_RAISED;
>  
> @@ -653,7 +653,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	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];
> +	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
>  	command = reg->command;
>  	if (!command)
> @@ -911,6 +911,7 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>  		dev_err(dev, "Failed to allocated register space\n");
>  		return -ENOMEM;
>  	}
> +	memset(base, 0, test_reg_size);
>  	epf_test->reg[test_reg_bar] = base;
>  
>  	for (bar = 0; bar < PCI_STD_NUM_BARS; bar += add) {
> -- 
> 2.39.1
> 

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

[PATCH 05/12] 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. The functions
pci_epf_test_write(), pci_epf_test_read() and pci_epf_test_copy() are
modified to add the use_dma boolean argument to indicate if transfers
should be done using DMA or mmio accesses.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index df3074667bbc..e07868c99531 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -327,10 +327,9 @@ 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, bool use_dma)
 {
 	int ret;
-	bool use_dma;
 	void __iomem *src_addr;
 	void __iomem *dst_addr;
 	phys_addr_t src_phys_addr;
@@ -375,14 +374,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 (epf_test->dma_private) {
 			dev_err(dev, "Cannot transfer data using DMA\n");
 			ret = -EINVAL;
@@ -426,13 +418,12 @@ 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, bool use_dma)
 {
 	int ret;
 	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;
@@ -465,14 +456,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;
-		}
-
 		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
 					       DMA_FROM_DEVICE);
 		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
@@ -516,12 +500,11 @@ 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, bool use_dma)
 {
 	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;
@@ -557,14 +540,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;
-		}
-
 		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
 					       DMA_TO_DEVICE);
 		if (dma_mapping_error(dma_dev, src_phys_addr)) {
@@ -647,6 +623,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	int ret;
 	int count;
 	u32 command;
+	bool use_dma;
 	struct pci_epf_test *epf_test = container_of(work, struct pci_epf_test,
 						     cmd_handler.work);
 	struct pci_epf *epf = epf_test->epf;
@@ -662,6 +639,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 	reg->command = 0;
 	reg->status = 0;
 
+	use_dma = reg->flags & FLAG_USE_DMA;
+	if (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;
@@ -675,7 +658,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, use_dma);
 		if (ret)
 			reg->status |= STATUS_WRITE_FAIL;
 		else
@@ -686,7 +669,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, use_dma);
 		if (!ret)
 			reg->status |= STATUS_READ_SUCCESS;
 		else
@@ -697,7 +680,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, use_dma);
 		if (!ret)
 			reg->status |= STATUS_COPY_SUCCESS;
 		else
-- 
2.39.1

Re: [PATCH 05/12] pci: epf-test: Simplify dma support checks

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:48PM +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. The functions
> pci_epf_test_write(), pci_epf_test_read() and pci_epf_test_copy() are
> modified to add the use_dma boolean argument to indicate if transfers
> should be done using DMA or mmio accesses.
> 

For all the endpoint patches, please follow the conventional subject prefix,
which is,

PCI: endpoint:

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

With that fixed,

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

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 43 ++++++-------------
>  1 file changed, 13 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index df3074667bbc..e07868c99531 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -327,10 +327,9 @@ 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, bool use_dma)
>  {
>  	int ret;
> -	bool use_dma;
>  	void __iomem *src_addr;
>  	void __iomem *dst_addr;
>  	phys_addr_t src_phys_addr;
> @@ -375,14 +374,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 (epf_test->dma_private) {
>  			dev_err(dev, "Cannot transfer data using DMA\n");
>  			ret = -EINVAL;
> @@ -426,13 +418,12 @@ 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, bool use_dma)
>  {
>  	int ret;
>  	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;
> @@ -465,14 +456,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;
> -		}
> -
>  		dst_phys_addr = dma_map_single(dma_dev, buf, reg->size,
>  					       DMA_FROM_DEVICE);
>  		if (dma_mapping_error(dma_dev, dst_phys_addr)) {
> @@ -516,12 +500,11 @@ 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, bool use_dma)
>  {
>  	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;
> @@ -557,14 +540,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;
> -		}
> -
>  		src_phys_addr = dma_map_single(dma_dev, buf, reg->size,
>  					       DMA_TO_DEVICE);
>  		if (dma_mapping_error(dma_dev, src_phys_addr)) {
> @@ -647,6 +623,7 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	int ret;
>  	int count;
>  	u32 command;
> +	bool use_dma;
>  	struct pci_epf_test *epf_test = container_of(work, struct pci_epf_test,
>  						     cmd_handler.work);
>  	struct pci_epf *epf = epf_test->epf;
> @@ -662,6 +639,12 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
>  	reg->command = 0;
>  	reg->status = 0;
>  
> +	use_dma = reg->flags & FLAG_USE_DMA;
> +	if (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;
> @@ -675,7 +658,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, use_dma);
>  		if (ret)
>  			reg->status |= STATUS_WRITE_FAIL;
>  		else
> @@ -686,7 +669,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, use_dma);
>  		if (!ret)
>  			reg->status |= STATUS_READ_SUCCESS;
>  		else
> @@ -697,7 +680,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, use_dma);
>  		if (!ret)
>  			reg->status |= STATUS_COPY_SUCCESS;
>  		else
> -- 
> 2.39.1
> 

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

[PATCH 06/12] 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 disivision by a 32-bits ns value, simply use div64_u64() to
calculate the 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>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 42 ++++++++-----------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index e07868c99531..f630393e8208 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, bool use_dma)
@@ -400,7 +389,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
 		kfree(buf);
 	}
 	ktime_get_ts64(&end);
-	pci_epf_test_print_rate("COPY", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
+				use_dma);
 
 err_map_addr:
 	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
@@ -481,7 +471,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test, bool use_dma)
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("READ", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
+				use_dma);
 
 	crc32 = crc32_le(~0, buf, reg->size);
 	if (crc32 != reg->checksum)
@@ -567,7 +558,8 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test, bool use_dma)
 		ktime_get_ts64(&end);
 	}
 
-	pci_epf_test_print_rate("WRITE", reg->size, &start, &end, use_dma);
+	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
+				use_dma);
 
 	/*
 	 * wait 1ms inorder for the write to complete. Without this delay L3
-- 
2.39.1

Re: [PATCH 06/12] pci: epf-test: Simplify transfers result print

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:49PM +0900, Damien Le Moal wrote:
> In pci_epf_test_print_rate(), instead of open coding a reduction loop to
> allow for a disivision by a 32-bits ns value, simply use div64_u64() to
> calculate the 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>

Thanks,
Mani

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 42 ++++++++-----------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index e07868c99531..f630393e8208 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, bool use_dma)
> @@ -400,7 +389,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>  		kfree(buf);
>  	}
>  	ktime_get_ts64(&end);
> -	pci_epf_test_print_rate("COPY", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate(epf_test, "COPY", reg->size, &start, &end,
> +				use_dma);
>  
>  err_map_addr:
>  	pci_epc_unmap_addr(epc, epf->func_no, epf->vfunc_no, dst_phys_addr);
> @@ -481,7 +471,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test, bool use_dma)
>  		ktime_get_ts64(&end);
>  	}
>  
> -	pci_epf_test_print_rate("READ", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate(epf_test, "READ", reg->size, &start, &end,
> +				use_dma);
>  
>  	crc32 = crc32_le(~0, buf, reg->size);
>  	if (crc32 != reg->checksum)
> @@ -567,7 +558,8 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test, bool use_dma)
>  		ktime_get_ts64(&end);
>  	}
>  
> -	pci_epf_test_print_rate("WRITE", reg->size, &start, &end, use_dma);
> +	pci_epf_test_print_rate(epf_test, "WRITE", reg->size, &start, &end,
> +				use_dma);
>  
>  	/*
>  	 * wait 1ms inorder for the write to complete. Without this delay L3
> -- 
> 2.39.1
> 

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

[PATCH 07/12] pci: epf-test: Add debug and error messages

[Damien Le Moal]

Make the pci-epf-test driver more verbose with dynamic debug messages
using dev_dbg(). Also add some dev_err() error messages to help
troubleshoot issues.

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

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index f630393e8208..9b791f4a7ffb 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
+	dev_dbg(&epf->dev,
+		"COPY src addr 0x%llx, dst addr 0x%llx, %u B\n",
+		reg->src_addr, reg->dst_addr, reg->size);
+
 	src_addr = pci_epc_mem_alloc_addr(epc, &src_phys_addr, reg->size);
 	if (!src_addr) {
 		dev_err(dev, "Failed to allocate source address\n");
@@ -380,6 +384,8 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
 
 		buf = kzalloc(reg->size, GFP_KERNEL);
 		if (!buf) {
+			dev_err(dev, "Alloc %zu B for copy failed\n",
+				(size_t)reg->size);
 			ret = -ENOMEM;
 			goto err_map_addr;
 		}
@@ -424,6 +430,9 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test, bool use_dma)
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
+	dev_dbg(&epf->dev, "READ src addr 0x%llx, %u B\n",
+		reg->src_addr, reg->size);
+
 	src_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
 	if (!src_addr) {
 		dev_err(dev, "Failed to allocate address\n");
@@ -442,6 +451,8 @@ static int pci_epf_test_read(struct pci_epf_test *epf_test, bool use_dma)
 
 	buf = kzalloc(reg->size, GFP_KERNEL);
 	if (!buf) {
+		dev_err(dev, "Alloc %zu B for read failed\n",
+			(size_t)reg->size);
 		ret = -ENOMEM;
 		goto err_map_addr;
 	}
@@ -506,6 +517,9 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test, bool use_dma)
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
 
+	dev_dbg(&epf->dev, "WRITE dst addr 0x%llx, %u B\n",
+		reg->dst_addr, reg->size);
+
 	dst_addr = pci_epc_mem_alloc_addr(epc, &phys_addr, reg->size);
 	if (!dst_addr) {
 		dev_err(dev, "Failed to allocate address\n");
@@ -524,6 +538,8 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test, bool use_dma)
 
 	buf = kzalloc(reg->size, GFP_KERNEL);
 	if (!buf) {
+		dev_err(dev, "Alloc %zu B for write failed\n",
+			(size_t)reg->size);
 		ret = -ENOMEM;
 		goto err_map_addr;
 	}
@@ -580,7 +596,7 @@ static int pci_epf_test_write(struct pci_epf_test *epf_test, bool use_dma)
 	return ret;
 }
 
-static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
+static int pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
 				   u16 irq)
 {
 	struct pci_epf *epf = epf_test->epf;
@@ -588,26 +604,35 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test, u8 irq_type,
 	struct pci_epc *epc = epf->epc;
 	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
 	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	int ret;
 
 	reg->status |= STATUS_IRQ_RAISED;
 
 	switch (irq_type) {
 	case IRQ_TYPE_LEGACY:
-		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_LEGACY, 0);
+		dev_dbg(&epf->dev, "RAISE legacy IRQ\n");
+		ret = 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);
+		dev_dbg(&epf->dev, "RAISE MSI IRQ %d\n", (int)irq);
+		ret = pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
+					PCI_EPC_IRQ_MSI, irq);
 		break;
 	case IRQ_TYPE_MSIX:
-		pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
-				  PCI_EPC_IRQ_MSIX, irq);
+		dev_dbg(&epf->dev, "RAISE MSIX IRQ %d\n", (int)irq);
+		ret = pci_epc_raise_irq(epc, epf->func_no, epf->vfunc_no,
+					PCI_EPC_IRQ_MSIX, irq);
 		break;
 	default:
 		dev_err(dev, "Failed to raise IRQ, unknown type\n");
-		break;
+		return -EINVAL;
 	}
+
+	if (ret)
+		dev_err(dev, "Raise IRQ failed %d\n", ret);
+
+	return ret;
 }
 
 static void pci_epf_test_cmd_handler(struct work_struct *work)
@@ -684,8 +709,11 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 
 	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)
+		if (reg->irq_number > count || count <= 0) {
+			dev_err(dev, "Invalid MSI %d / %d\n",
+				(int)reg->irq_number, (int)count);
 			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);
@@ -694,14 +722,19 @@ static void pci_epf_test_cmd_handler(struct work_struct *work)
 
 	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)
+		if (reg->irq_number > count || count <= 0) {
+			dev_err(dev, "Invalid MSIX %d / %d\n",
+				(int)reg->irq_number, (int)count);
 			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;
 	}
 
+	dev_err(dev, "Unknown command 0x%x\n", command);
+
 reset_handler:
 	queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
 			   msecs_to_jiffies(1));
@@ -828,12 +861,14 @@ static int pci_epf_test_notifier(struct notifier_block *nb, unsigned long val,
 
 	switch (val) {
 	case CORE_INIT:
+		dev_dbg(&epf->dev, "CORE_INIT event\n");
 		ret = pci_epf_test_core_init(epf);
 		if (ret)
 			return NOTIFY_BAD;
 		break;
 
 	case LINK_UP:
+		dev_dbg(&epf->dev, "LINK_UP event\n");
 		queue_delayed_work(kpcitest_workqueue, &epf_test->cmd_handler,
 				   msecs_to_jiffies(1));
 		break;
@@ -875,8 +910,12 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	test_reg_size = test_reg_bar_size + msix_table_size + pba_size;
 
 	if (epc_features->bar_fixed_size[test_reg_bar]) {
-		if (test_reg_size > bar_size[test_reg_bar])
+		if (test_reg_size > bar_size[test_reg_bar]) {
+			dev_err(&epf->dev, "BAR %d: %zu B > %zu B\n",
+				(int)test_reg_bar, test_reg_size,
+				(size_t)bar_size[test_reg_bar]);
 			return -ENOMEM;
+		}
 		test_reg_size = bar_size[test_reg_bar];
 	}
 
@@ -938,8 +977,10 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	bool linkup_notifier = false;
 	bool core_init_notifier = false;
 
-	if (WARN_ON_ONCE(!epc))
+	if (WARN_ON_ONCE(!epc)) {
+		dev_err(&epf->dev, "No controller\n");
 		return -EINVAL;
+	}
 
 	epc_features = pci_epc_get_features(epc, epf->func_no, epf->vfunc_no);
 	if (!epc_features) {
@@ -950,8 +991,10 @@ static int pci_epf_test_bind(struct pci_epf *epf)
 	linkup_notifier = epc_features->linkup_notifier;
 	core_init_notifier = epc_features->core_init_notifier;
 	test_reg_bar = pci_epc_get_first_free_bar(epc_features);
-	if (test_reg_bar < 0)
+	if (test_reg_bar < 0) {
+		dev_err(&epf->dev, "No free BAR\n");
 		return -EINVAL;
+	}
 	pci_epf_configure_bar(epf, epc_features);
 
 	epf_test->test_reg_bar = test_reg_bar;
-- 
2.39.1

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
> Make the pci-epf-test driver more verbose with dynamic debug messages
> using dev_dbg(). Also add some dev_err() error messages to help
> troubleshoot issues.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index f630393e8208..9b791f4a7ffb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>  
> +	dev_dbg(&epf->dev,
> +		"COPY src addr 0x%llx, dst addr 0x%llx, %u B\n",
> +		reg->src_addr, reg->dst_addr, reg->size);

Again, no, please just use ftrace.

thanks,

greg k-h

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Damien Le Moal]

On 2/15/23 20:34, Greg Kroah-Hartman wrote:
> On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
>> Make the pci-epf-test driver more verbose with dynamic debug messages
>> using dev_dbg(). Also add some dev_err() error messages to help
>> troubleshoot issues.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index f630393e8208..9b791f4a7ffb 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>>  
>> +	dev_dbg(&epf->dev,
>> +		"COPY src addr 0x%llx, dst addr 0x%llx, %u B\n",
>> +		reg->src_addr, reg->dst_addr, reg->size);
> 
> Again, no, please just use ftrace.

OK. Got it.

> 
> thanks,
> 
> greg k-h

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
> Make the pci-epf-test driver more verbose with dynamic debug messages
> using dev_dbg(). Also add some dev_err() error messages to help
> troubleshoot issues.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
>  1 file changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index f630393e8208..9b791f4a7ffb 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];

note, volatile is almost always wrong, please fix that up.

thanks,

greg k-h

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Damien Le Moal]

On 2/15/23 20:34, Greg Kroah-Hartman wrote:
> On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
>> Make the pci-epf-test driver more verbose with dynamic debug messages
>> using dev_dbg(). Also add some dev_err() error messages to help
>> troubleshoot issues.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> index f630393e8208..9b791f4a7ffb 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> 
> note, volatile is almost always wrong, please fix that up.

OK. Will think of something else.
Thanks for the feedback.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 08:45:50PM +0900, Damien Le Moal wrote:
> On 2/15/23 20:34, Greg Kroah-Hartman wrote:
> > On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
> >> Make the pci-epf-test driver more verbose with dynamic debug messages
> >> using dev_dbg(). Also add some dev_err() error messages to help
> >> troubleshoot issues.
> >>
> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> >> ---
> >>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
> >>  1 file changed, 56 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> index f630393e8208..9b791f4a7ffb 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
> >>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> >>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> > 
> > note, volatile is almost always wrong, please fix that up.
> 
> OK. Will think of something else.

If this is io memory, use the proper accessors to access it.  If it is
not io memory, then why is it marked volatile at all?

thanks,

greg k-h

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Damien Le Moal]

On 2/15/23 21:01, Greg Kroah-Hartman wrote:
> On Wed, Feb 15, 2023 at 08:45:50PM +0900, Damien Le Moal wrote:
>> On 2/15/23 20:34, Greg Kroah-Hartman wrote:
>>> On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
>>>> Make the pci-epf-test driver more verbose with dynamic debug messages
>>>> using dev_dbg(). Also add some dev_err() error messages to help
>>>> troubleshoot issues.
>>>>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>> ---
>>>>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
>>>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> index f630393e8208..9b791f4a7ffb 100644
>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>>>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>>>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>>>
>>> note, volatile is almost always wrong, please fix that up.
>>
>> OK. Will think of something else.
> 
> If this is io memory, use the proper accessors to access it.  If it is
> not io memory, then why is it marked volatile at all?

This is a PCI bar memory. So I can simply copy the structure locally with
memcpy_fromio() and memcpy_toio().

> 
> thanks,
> 
> greg k-h

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 09:18:48PM +0900, Damien Le Moal wrote:
> On 2/15/23 21:01, Greg Kroah-Hartman wrote:
> > On Wed, Feb 15, 2023 at 08:45:50PM +0900, Damien Le Moal wrote:
> >> On 2/15/23 20:34, Greg Kroah-Hartman wrote:
> >>> On Wed, Feb 15, 2023 at 12:21:50PM +0900, Damien Le Moal wrote:
> >>>> Make the pci-epf-test driver more verbose with dynamic debug messages
> >>>> using dev_dbg(). Also add some dev_err() error messages to help
> >>>> troubleshoot issues.
> >>>>
> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> >>>> ---
> >>>>  drivers/pci/endpoint/functions/pci-epf-test.c | 69 +++++++++++++++----
> >>>>  1 file changed, 56 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> index f630393e8208..9b791f4a7ffb 100644
> >>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >>>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
> >>>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> >>>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> >>>
> >>> note, volatile is almost always wrong, please fix that up.
> >>
> >> OK. Will think of something else.
> > 
> > If this is io memory, use the proper accessors to access it.  If it is
> > not io memory, then why is it marked volatile at all?
> 
> This is a PCI bar memory. So I can simply copy the structure locally with
> memcpy_fromio() and memcpy_toio().

Great, please do so instead of trying to access it directly like this,
which will break on some platforms.

thanks,

greg k-h

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Arnd Bergmann]

On Wed, Feb 15, 2023, at 14:24, Greg Kroah-Hartman wrote:
> On Wed, Feb 15, 2023 at 09:18:48PM +0900, Damien Le Moal wrote:
>> On 2/15/23 21:01, Greg Kroah-Hartman wrote:
>> >>>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>> >>>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>> >>>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>> >>>
>> >>> note, volatile is almost always wrong, please fix that up.
>> >>
>> >> OK. Will think of something else.
>> > 
>> > If this is io memory, use the proper accessors to access it.  If it is
>> > not io memory, then why is it marked volatile at all?
>> 
>> This is a PCI bar memory. So I can simply copy the structure locally with
>> memcpy_fromio() and memcpy_toio().
>
> Great, please do so instead of trying to access it directly like this,
> which will break on some platforms.

I think the reverse is true here: looking at where the pointer comes
from, 'reg' is actually the result of dma_alloc_coherent() in the
memory of the local (endpoint) machine, though it appears as a BAR on
the remote (host) side and gets mapped with ioremap() there.

This means that the host must use readl/write/memcpy_fromio/memcpy_toio
to access the buffer, matching the __iomem token there, while the
endpoint side not use those. On some machines, readl/write take
arguments that are not compatible with normal pointers, and will
do something completely different there.

A volatile access is not the worst option here, though this conflicts
with the '__packed' annotation in the structure definition that
may require bytewise access on architectures without unaligned
access.

I would drop the __packed in the definition, possibly annotating
only the 64-bit src_addr and dst_addr members as __packed to ensure
the layout is unchanged but the structure as a whole is 32-bit
aligned, and then use READ_ONCE()/WRITE_ONCE() to atomically
access each member in the coherent buffer.

If ordering between the accesses is required, you can add
dma_rmb() and dma_wmb() barriers.

     Arnd

Re: [PATCH 07/12] pci: epf-test: Add debug and error messages

[Damien Le Moal]

On 2/15/23 22:49, Arnd Bergmann wrote:
> On Wed, Feb 15, 2023, at 14:24, Greg Kroah-Hartman wrote:
>> On Wed, Feb 15, 2023 at 09:18:48PM +0900, Damien Le Moal wrote:
>>> On 2/15/23 21:01, Greg Kroah-Hartman wrote:
>>>>>>> @@ -330,6 +330,10 @@ static int pci_epf_test_copy(struct pci_epf_test *epf_test, bool use_dma)
>>>>>>>  	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>>>>>>>  	volatile struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
>>>>>>
>>>>>> note, volatile is almost always wrong, please fix that up.
>>>>>
>>>>> OK. Will think of something else.
>>>>
>>>> If this is io memory, use the proper accessors to access it.  If it is
>>>> not io memory, then why is it marked volatile at all?
>>>
>>> This is a PCI bar memory. So I can simply copy the structure locally with
>>> memcpy_fromio() and memcpy_toio().
>>
>> Great, please do so instead of trying to access it directly like this,
>> which will break on some platforms.
> 
> I think the reverse is true here: looking at where the pointer comes
> from, 'reg' is actually the result of dma_alloc_coherent() in the
> memory of the local (endpoint) machine, though it appears as a BAR on
> the remote (host) side and gets mapped with ioremap() there.
> 
> This means that the host must use readl/write/memcpy_fromio/memcpy_toio
> to access the buffer, matching the __iomem token there, while the
> endpoint side not use those. On some machines, readl/write take
> arguments that are not compatible with normal pointers, and will
> do something completely different there.
> 
> A volatile access is not the worst option here, though this conflicts
> with the '__packed' annotation in the structure definition that
> may require bytewise access on architectures without unaligned
> access.
> 
> I would drop the __packed in the definition, possibly annotating
> only the 64-bit src_addr and dst_addr members as __packed to ensure
> the layout is unchanged but the structure as a whole is 32-bit
> aligned, and then use READ_ONCE()/WRITE_ONCE() to atomically
> access each member in the coherent buffer.

I guess that would work too. But given that there are accesses to individual
members all over the place, I think it would be easier to get a local copy of
the reg structure in pci_epf_test_cmd_handler() and pass a pointer of that local
copy to the pci_epf_test_xxx() functions. The only READ_ONCE() needed would be
to test the command field on entry to pci_epf_test_cmd_handler() to be sure that
we have a valid command.

The host side always sets the reg command field last, which I think kind of
assumes an ordered update on the EP side (all other fields set before the
command field). That does seem a bit fragile to me as my understanding is that
PCI does not necessarily guarantees ordering of IO TLPs. But I may be wrong here.

> If ordering between the accesses is required, you can add
> dma_rmb() and dma_wmb() barriers.

Which I guess is the one thing we need after testing the reg command field in
pci_epf_test_cmd_handler() and before making the local copy, to avoid problems
with ordering of the reg fields writes from the host.

Will use that in v2.


-- 
Damien Le Moal
Western Digital Research

[PATCH 08/12] 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.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 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 11530b4ec389..e27d471cc847 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -937,6 +937,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);
@@ -946,9 +949,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.1

Re: [PATCH 08/12] misc: pci_endpoint_test: Free IRQs before removing the device

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:51PM +0900, Damien Le Moal wrote:
> 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.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

This looks like a bug. So there should be Fixes tag and stable list has to be
CCed for backporting.

With that,

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

Thanks,
Mani

> ---
>  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 11530b4ec389..e27d471cc847 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -937,6 +937,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);
> @@ -946,9 +949,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.1
> 

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

[PATCH 09/12] 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>
---
 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 e27d471cc847..c1370950c79d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -158,10 +158,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.1

Re: [PATCH 09/12] misc: pci_endpoint_test: Do not write status in IRQ handler

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:52PM +0900, Damien Le Moal wrote:
> 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>

Thanks,
Mani

> ---
>  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 e27d471cc847..c1370950c79d 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -158,10 +158,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.1
> 

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

[PATCH 10/12] 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.

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 c1370950c79d..baab08f983a2 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -725,6 +725,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 = -1;
+
 	switch (cmd) {
 	case PCITEST_BAR:
 		bar = arg;
-- 
2.39.1

Re: [PATCH 10/12] misc: pci_endpoint_test: Re-init completion for every test

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:53PM +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.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Fixes tag? CC stable?

> ---
>  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 c1370950c79d..baab08f983a2 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -725,6 +725,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 = -1;

-ENODATA?

Thanks,
Mani

> +
>  	switch (cmd) {
>  	case PCITEST_BAR:
>  		bar = arg;
> -- 
> 2.39.1
> 

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

Re: [PATCH 10/12] misc: pci_endpoint_test: Re-init completion for every test

[Damien Le Moal]

On 2/16/23 19:55, Manivannan Sadhasivam wrote:
> On Wed, Feb 15, 2023 at 12:21:53PM +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.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Fixes tag? CC stable?

I could not find a ref for this one... git blame did not lead to anything. Not
sure if this file was renamed in the past or something.

> 
>> ---
>>  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 c1370950c79d..baab08f983a2 100644
>> --- a/drivers/misc/pci_endpoint_test.c
>> +++ b/drivers/misc/pci_endpoint_test.c
>> @@ -725,6 +725,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 = -1;
> 
> -ENODATA?

Sure. Anything works here.


-- 
Damien Le Moal
Western Digital Research

[PATCH 11/12] 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>
---
 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 baab08f983a2..b05d3db85da8 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -312,21 +312,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.1

Re: [PATCH 11/12] misc: pci_endpoint_test: Simplify pci_endpoint_test_msi_irq()

[Manivannan Sadhasivam]

On Wed, Feb 15, 2023 at 12:21:54PM +0900, Damien Le Moal wrote:
> 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>

Thanks,
Mani

> ---
>  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 baab08f983a2..b05d3db85da8 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -312,21 +312,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.1
> 

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

[PATCH 12/12] misc: pci_endpoint_test: Add debug and error messages

[Damien Le Moal]

Add dynamic debug messages with dev_dbg() to help troubleshoot issues
when running the endpoint tests. The debug messages for errors detected
in pci_endpoint_test_validate_xfer_params() are changed to error
messages.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/misc/pci_endpoint_test.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index b05d3db85da8..c47f6e708ea2 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -267,12 +267,15 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 	u32 val;
 	int size;
 	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
 
 	if (!test->bar[barno])
 		return false;
 
 	size = pci_resource_len(pdev, barno);
 
+	dev_dbg(dev, "Test BAR %d, %d B\n", (int)barno, size);
+
 	if (barno == test->test_reg_bar)
 		size = 0x4;
 
@@ -291,6 +294,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
 {
 	u32 val;
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+
+	dev_dbg(dev, "Test legacy IRQ\n");
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
 				 IRQ_TYPE_LEGACY);
@@ -310,6 +317,9 @@ static bool pci_endpoint_test_msi_irq(struct pci_endpoint_test *test,
 {
 	u32 val;
 	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+
+	dev_dbg(dev, "Test MSI%s %d\n", msix ? "X" : "", msi_num);
 
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_IRQ_TYPE,
 				 msix ? IRQ_TYPE_MSIX : IRQ_TYPE_MSI);
@@ -329,12 +339,12 @@ static int pci_endpoint_test_validate_xfer_params(struct device *dev,
 		struct pci_endpoint_test_xfer_param *param, size_t alignment)
 {
 	if (!param->size) {
-		dev_dbg(dev, "Data size is zero\n");
+		dev_err(dev, "Data size is zero\n");
 		return -EINVAL;
 	}
 
 	if (param->size > SIZE_MAX - alignment) {
-		dev_dbg(dev, "Maximum transfer data size exceeded\n");
+		dev_err(dev, "Maximum transfer data size exceeded\n");
 		return -EINVAL;
 	}
 
@@ -444,6 +454,10 @@ static bool pci_endpoint_test_copy(struct pci_endpoint_test *test,
 		dst_addr = orig_dst_addr;
 	}
 
+	dev_dbg(dev,
+		"Test COPY align %zu, src phys addr 0x%llx, dst phys addr 0x%llx, %zu B\n",
+		alignment, src_phys_addr, dst_phys_addr, size);
+
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
 				 lower_32_bits(dst_phys_addr));
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
@@ -549,6 +563,10 @@ static bool pci_endpoint_test_write(struct pci_endpoint_test *test,
 		addr = orig_addr;
 	}
 
+	dev_dbg(dev,
+		"Test WRITE align %zu, phys addr 0x%llx, %zu B\n",
+		alignment, phys_addr, size);
+
 	crc32 = crc32_le(~0, addr, size);
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_CHECKSUM,
 				 crc32);
@@ -647,6 +665,10 @@ static bool pci_endpoint_test_read(struct pci_endpoint_test *test,
 		addr = orig_addr;
 	}
 
+	dev_dbg(dev,
+		"Test READ align %zu, phys addr 0x%llx, %zu B\n",
+		alignment, phys_addr, size);
+
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_LOWER_DST_ADDR,
 				 lower_32_bits(phys_addr));
 	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_UPPER_DST_ADDR,
-- 
2.39.1

Re: [PATCH 12/12] misc: pci_endpoint_test: Add debug and error messages

[Greg Kroah-Hartman]

On Wed, Feb 15, 2023 at 12:21:55PM +0900, Damien Le Moal wrote:
> Add dynamic debug messages with dev_dbg() to help troubleshoot issues
> when running the endpoint tests. The debug messages for errors detected
> in pci_endpoint_test_validate_xfer_params() are changed to error
> messages.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index b05d3db85da8..c47f6e708ea2 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -267,12 +267,15 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  	u32 val;
>  	int size;
>  	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
>  
>  	if (!test->bar[barno])
>  		return false;
>  
>  	size = pci_resource_len(pdev, barno);
>  
> +	dev_dbg(dev, "Test BAR %d, %d B\n", (int)barno, size);
> +
>  	if (barno == test->test_reg_bar)
>  		size = 0x4;
>  
> @@ -291,6 +294,10 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_legacy_irq(struct pci_endpoint_test *test)
>  {
>  	u32 val;
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +
> +	dev_dbg(dev, "Test legacy IRQ\n");

Please don't do this, it's one of the things that we remove from
drivers before merging.

If you need to follow the driver flow, then use ftrace, that's what it
is there for, don't add this type of "now in this function!" debug
lines, as that is redundant.

thanks,

greg k-h