[PATCH 0/4] VFIO platform reset module rework

[PATCH 0/4] VFIO platform reset module rework

[Eric Auger]

This series fixes the current implementation by getting rid of the
usage of __symbol_get which caused a compilation issue with
CONFIG_MODULES disabled. On top of this, the usage of MODULE_ALIAS makes
possible to add a new reset module without being obliged to update the
framework. The new implementation relies on the reset module registering
its reset function to the vfio-platform driver.

The series is available at

https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.3-rc5-rework-xgbe-v2

Best Regards

Eric


Eric Auger (4):
  vfio: platform: add capability to register a reset function
  vfio: platform: reset: calxedaxgmac: add reset function registration
  vfio: platform: add compat in vfio_platform_device
  vfio: platform: use list of registered reset function

 .../platform/reset/vfio_platform_calxedaxgmac.c    |  40 +++++++-
 drivers/vfio/platform/vfio_platform_common.c       | 112 ++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h      |  16 +++
 3 files changed, 140 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH 1/4] vfio: platform: add capability to register a reset function

[Eric Auger]

In preparation for subsequent changes in reset function lookup,
lets introduce a dynamic list of reset combos (compat string,
reset module, reset function). The list can be populated/voided with
two new functions, vfio_platform_register/unregister_reset. Those are
not yet used in this patch.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c  | 55 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h | 14 +++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e43efb5..d36afc9 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -23,6 +23,8 @@
 
 #include "vfio_platform_private.h"
 
+struct list_head reset_list;
+LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
 static const struct vfio_platform_reset_combo reset_lookup_table[] = {
@@ -573,3 +575,56 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
 	return vdev;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
+
+int vfio_platform_register_reset(struct module *reset_owner, char *compat,
+				 vfio_platform_reset_fn_t reset)
+{
+	struct vfio_platform_reset_node *node, *iter;
+	bool found = false;
+
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat)) {
+			found = true;
+			break;
+		}
+	}
+	if (found)
+		return -EINVAL;
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	node->compat = kstrdup(compat, GFP_KERNEL);
+	if (!node->compat)
+		return -ENOMEM;
+
+	node->owner = reset_owner;
+	node->reset = reset;
+
+	list_add(&node->link, &reset_list);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_register_reset);
+
+int vfio_platform_unregister_reset(char *compat)
+{
+	struct vfio_platform_reset_node *iter;
+	bool found = false;
+
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat)) {
+			found = true;
+			break;
+		}
+	}
+	if (!found)
+		return -EINVAL;
+
+	list_del(&iter->link);
+	kfree(iter->compat);
+	kfree(iter);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_unregister_reset);
+
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 1c9b3d5..17323f0 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -76,6 +76,15 @@ struct vfio_platform_reset_combo {
 	const char *module_name;
 };
 
+typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
+
+struct vfio_platform_reset_node {
+	struct list_head link;
+	char *compat;
+	struct module *owner;
+	vfio_platform_reset_fn_t reset;
+};
+
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 				      struct device *dev);
 extern struct vfio_platform_device *vfio_platform_remove_common
@@ -89,4 +98,9 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 					unsigned start, unsigned count,
 					void *data);
 
+extern int vfio_platform_register_reset(struct module *owner,
+					char *compat,
+					vfio_platform_reset_fn_t reset);
+extern int vfio_platform_unregister_reset(char *compat);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.9.1

[PATCH 1/4] vfio: platform: add capability to register a reset function

[Arnd Bergmann]

On Sunday 18 October 2015 18:00:12 Eric Auger wrote:
> 
> +struct list_head reset_list;
> +LIST_HEAD(reset_list);
> 

These two should be marked 'static'.

	Arnd

[PATCH 1/4] vfio: platform: add capability to register a reset function

[Alex Williamson]

On Sun, 2015-10-18 at 18:00 +0200, Eric Auger wrote:
> In preparation for subsequent changes in reset function lookup,
> lets introduce a dynamic list of reset combos (compat string,
> reset module, reset function). The list can be populated/voided with
> two new functions, vfio_platform_register/unregister_reset. Those are
> not yet used in this patch.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 55 +++++++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_private.h | 14 +++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e43efb5..d36afc9 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -23,6 +23,8 @@
>  
>  #include "vfio_platform_private.h"
>  
> +struct list_head reset_list;

What's the purpose of this one above?

> +LIST_HEAD(reset_list);

static

I think you also need a mutex protecting this list, otherwise nothing
prevents concurrent list changes and walks.  A rw lock probably fits the
usage model best, but I don't expect much contention if you just want to
use a mutex.

>  static DEFINE_MUTEX(driver_lock);
>  
>  static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> @@ -573,3 +575,56 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>  	return vdev;
>  }
>  EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
> +
> +int vfio_platform_register_reset(struct module *reset_owner, char *compat,

const char *

> +				 vfio_platform_reset_fn_t reset)
> +{
> +	struct vfio_platform_reset_node *node, *iter;
> +	bool found = false;
> +
> +	list_for_each_entry(iter, &reset_list, link) {
> +		if (!strcmp(iter->compat, compat)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (found)
> +		return -EINVAL;
> +
> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	node->compat = kstrdup(compat, GFP_KERNEL);
> +	if (!node->compat)
> +		return -ENOMEM;

Leaks node

> +
> +	node->owner = reset_owner;
> +	node->reset = reset;
> +
> +	list_add(&node->link, &reset_list);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_platform_register_reset);
> +
> +int vfio_platform_unregister_reset(char *compat)

const char *

> +{
> +	struct vfio_platform_reset_node *iter;
> +	bool found = false;
> +
> +	list_for_each_entry(iter, &reset_list, link) {
> +		if (!strcmp(iter->compat, compat)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (!found)
> +		return -EINVAL;
> +
> +	list_del(&iter->link);
> +	kfree(iter->compat);
> +	kfree(iter);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_platform_unregister_reset);
> +
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 1c9b3d5..17323f0 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -76,6 +76,15 @@ struct vfio_platform_reset_combo {
>  	const char *module_name;
>  };
>  
> +typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
> +
> +struct vfio_platform_reset_node {
> +	struct list_head link;
> +	char *compat;
> +	struct module *owner;
> +	vfio_platform_reset_fn_t reset;
> +};
> +
>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>  				      struct device *dev);
>  extern struct vfio_platform_device *vfio_platform_remove_common
> @@ -89,4 +98,9 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>  					unsigned start, unsigned count,
>  					void *data);
>  
> +extern int vfio_platform_register_reset(struct module *owner,
> +					char *compat,
> +					vfio_platform_reset_fn_t reset);
> +extern int vfio_platform_unregister_reset(char *compat);
> +
>  #endif /* VFIO_PLATFORM_PRIVATE_H */

[PATCH 1/4] vfio: platform: add capability to register a reset function

[Eric Auger]

Hi Alex,
On 10/19/2015 09:45 PM, Alex Williamson wrote:
> On Sun, 2015-10-18 at 18:00 +0200, Eric Auger wrote:
>> In preparation for subsequent changes in reset function lookup,
>> lets introduce a dynamic list of reset combos (compat string,
>> reset module, reset function). The list can be populated/voided with
>> two new functions, vfio_platform_register/unregister_reset. Those are
>> not yet used in this patch.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 55 +++++++++++++++++++++++++++
>>  drivers/vfio/platform/vfio_platform_private.h | 14 +++++++
>>  2 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index e43efb5..d36afc9 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -23,6 +23,8 @@
>>  
>>  #include "vfio_platform_private.h"
>>  
>> +struct list_head reset_list;
> 
> What's the purpose of this one above?
useless indeed
> 
>> +LIST_HEAD(reset_list);
> 
> static
> 
> I think you also need a mutex protecting this list, otherwise nothing
> prevents concurrent list changes and walks.  A rw lock probably fits the
> usage model best, but I don't expect much contention if you just want to
> use a mutex.
yes you're right. I am going to use a standard mutex I think.
> 
>>  static DEFINE_MUTEX(driver_lock);
>>  
>>  static const struct vfio_platform_reset_combo reset_lookup_table[] = {
>> @@ -573,3 +575,56 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
>>  	return vdev;
>>  }
>>  EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
>> +
>> +int vfio_platform_register_reset(struct module *reset_owner, char *compat,
> 
> const char *
ok
> 
>> +				 vfio_platform_reset_fn_t reset)
>> +{
>> +	struct vfio_platform_reset_node *node, *iter;
>> +	bool found = false;
>> +
>> +	list_for_each_entry(iter, &reset_list, link) {
>> +		if (!strcmp(iter->compat, compat)) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +	if (found)
>> +		return -EINVAL;
>> +
>> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
>> +	if (!node)
>> +		return -ENOMEM;
>> +
>> +	node->compat = kstrdup(compat, GFP_KERNEL);
>> +	if (!node->compat)
>> +		return -ENOMEM;
> 
> Leaks node
thanks
> 
>> +
>> +	node->owner = reset_owner;
>> +	node->reset = reset;
>> +
>> +	list_add(&node->link, &reset_list);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vfio_platform_register_reset);
>> +
>> +int vfio_platform_unregister_reset(char *compat)
> 
> const char *
ok

Thanks for your quick review!

Best Regards

Eric
> 
>> +{
>> +	struct vfio_platform_reset_node *iter;
>> +	bool found = false;
>> +
>> +	list_for_each_entry(iter, &reset_list, link) {
>> +		if (!strcmp(iter->compat, compat)) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +	if (!found)
>> +		return -EINVAL;
>> +
>> +	list_del(&iter->link);
>> +	kfree(iter->compat);
>> +	kfree(iter);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vfio_platform_unregister_reset);
>> +
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 1c9b3d5..17323f0 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -76,6 +76,15 @@ struct vfio_platform_reset_combo {
>>  	const char *module_name;
>>  };
>>  
>> +typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
>> +
>> +struct vfio_platform_reset_node {
>> +	struct list_head link;
>> +	char *compat;
>> +	struct module *owner;
>> +	vfio_platform_reset_fn_t reset;
>> +};
>> +
>>  extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
>>  				      struct device *dev);
>>  extern struct vfio_platform_device *vfio_platform_remove_common
>> @@ -89,4 +98,9 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>>  					unsigned start, unsigned count,
>>  					void *data);
>>  
>> +extern int vfio_platform_register_reset(struct module *owner,
>> +					char *compat,
>> +					vfio_platform_reset_fn_t reset);
>> +extern int vfio_platform_unregister_reset(char *compat);
>> +
>>  #endif /* VFIO_PLATFORM_PRIVATE_H */
> 
> 
> 

[PATCH 2/4] vfio: platform: reset: calxedaxgmac: add reset function registration

[Eric Auger]

This patch adds the reset function registration/unregistration.
Also a MODULE_ALIAS is added.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 .../platform/reset/vfio_platform_calxedaxgmac.c    | 40 ++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index 619dc7d..4f76b17 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -29,8 +29,7 @@
 #define DRIVER_VERSION  "0.1"
 #define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
 #define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
-
-#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
+#define COMPAT		"calxeda,hb-xgmac"
 
 /* XGMAC Register definitions */
 #define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
@@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 }
 EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
 
+static int __init vfio_platform_calxedaxgmac_init(void)
+{
+	int (*register_reset)(struct module *, char*,
+				vfio_platform_reset_fn_t);
+	int ret;
+
+	register_reset = symbol_get(vfio_platform_register_reset);
+	if (!register_reset)
+		return -EINVAL;
+
+	ret = register_reset(THIS_MODULE, COMPAT,
+	vfio_platform_calxedaxgmac_reset);
+
+	symbol_put(vfio_platform_register_reset);
+
+	return ret;
+}
+
+static void __exit vfio_platform_calxedaxgmac_exit(void)
+{
+	int (*unregister_reset)(char *);
+	int ret;
+
+	unregister_reset = symbol_get(vfio_platform_unregister_reset);
+	if (!unregister_reset)
+		return;
+
+	ret = unregister_reset(COMPAT);
+
+	symbol_put(vfio_platform_unregister_reset);
+}
+
+module_init(vfio_platform_calxedaxgmac_init);
+module_exit(vfio_platform_calxedaxgmac_exit);
+
+MODULE_ALIAS("vfio-reset:"  COMPAT);
+
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
1.9.1

[PATCH 2/4] vfio: platform: reset: calxedaxgmac: add reset function registration

[Arnd Bergmann]

On Sunday 18 October 2015 18:00:13 Eric Auger wrote:
> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> index 619dc7d..4f76b17 100644
> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> @@ -29,8 +29,7 @@
>  #define DRIVER_VERSION  "0.1"
>  #define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
>  #define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
> -
> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
> +#define COMPAT		"calxeda,hb-xgmac"

Why the rename?

>  /* XGMAC Register definitions */
>  #define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>  }
>  EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
>  
> +static int __init vfio_platform_calxedaxgmac_init(void)
> +{
> +	int (*register_reset)(struct module *, char*,
> +				vfio_platform_reset_fn_t);
> +	int ret;
> +
> +	register_reset = symbol_get(vfio_platform_register_reset);
> +	if (!register_reset)
> +		return -EINVAL;

I don't understand what you do the symbol_get() here for.
Why not just call that function directly?

> +	ret = register_reset(THIS_MODULE, COMPAT,
> +	vfio_platform_calxedaxgmac_reset);

This is whitespace damaged, the second line needs to be indented
to the opening braces.

I would also suggest defining register_reset as a macro that
implicitly passes the THIS_MODULE argument, as other subsystems
do.

	Arnd

[PATCH 2/4] vfio: platform: reset: calxedaxgmac: add reset function registration

[Eric Auger]

Hi Arnd,
On 10/19/2015 03:04 PM, Arnd Bergmann wrote:
> On Sunday 18 October 2015 18:00:13 Eric Auger wrote:
>> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>> index 619dc7d..4f76b17 100644
>> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>> @@ -29,8 +29,7 @@
>>  #define DRIVER_VERSION  "0.1"
>>  #define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
>>  #define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
>> -
>> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
>> +#define COMPAT		"calxeda,hb-xgmac"
> 
> Why the rename?
This define was not used. Since the code is meant to be duplicated from
one reset module to the other I thought it did not bring any have a
specialized name
> 
>>  /* XGMAC Register definitions */
>>  #define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
>> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>>  }
>>  EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
>>  
>> +static int __init vfio_platform_calxedaxgmac_init(void)
>> +{
>> +	int (*register_reset)(struct module *, char*,
>> +				vfio_platform_reset_fn_t);
>> +	int ret;
>> +
>> +	register_reset = symbol_get(vfio_platform_register_reset);
>> +	if (!register_reset)
>> +		return -EINVAL;
> 
> I don't understand what you do the symbol_get() here for.
> Why not just call that function directly
the function is implemented in a separate module. This is just to make
sure the vfio-platform module is loaded, in case the end-user attempts
to load the reset module without having this latter loaded.
> 
>> +	ret = register_reset(THIS_MODULE, COMPAT,
>> +	vfio_platform_calxedaxgmac_reset);
> 
> This is whitespace damaged, the second line needs to be indented
> to the opening braces.
ok
> 
> I would also suggest defining register_reset as a macro that
> implicitly passes the THIS_MODULE argument, as other subsystems
> do.
ok

Thanks

Best Regards

Eric
> 
> 	Arnd
> 

[PATCH 2/4] vfio: platform: reset: calxedaxgmac: add reset function registration

[Arnd Bergmann]

On Monday 19 October 2015 15:17:30 Eric Auger wrote:
> Hi Arnd,
> On 10/19/2015 03:04 PM, Arnd Bergmann wrote:
> > On Sunday 18 October 2015 18:00:13 Eric Auger wrote:
> >> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> >> index 619dc7d..4f76b17 100644
> >> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> >> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> >> @@ -29,8 +29,7 @@
> >>  #define DRIVER_VERSION  "0.1"
> >>  #define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
> >>  #define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
> >> -
> >> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
> >> +#define COMPAT		"calxeda,hb-xgmac"
> > 
> > Why the rename?
> This define was not used. Since the code is meant to be duplicated from
> one reset module to the other I thought it did not bring any have a
> specialized name

I'd say it would be clearer to remove the macro then, and pass the
string literal in the function call.

> >>  /* XGMAC Register definitions */
> >>  #define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
> >> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
> >>  }
> >>  EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
> >>  
> >> +static int __init vfio_platform_calxedaxgmac_init(void)
> >> +{
> >> +	int (*register_reset)(struct module *, char*,
> >> +				vfio_platform_reset_fn_t);
> >> +	int ret;
> >> +
> >> +	register_reset = symbol_get(vfio_platform_register_reset);
> >> +	if (!register_reset)
> >> +		return -EINVAL;
> > 
> > I don't understand what you do the symbol_get() here for.
> > Why not just call that function directly
> the function is implemented in a separate module. This is just to make
> sure the vfio-platform module is loaded, in case the end-user attempts
> to load the reset module without having this latter loaded.

The module loader does all this for you.

	Arnd

[PATCH 2/4] vfio: platform: reset: calxedaxgmac: add reset function registration

[Eric Auger]

On 10/19/2015 03:25 PM, Arnd Bergmann wrote:
> On Monday 19 October 2015 15:17:30 Eric Auger wrote:
>> Hi Arnd,
>> On 10/19/2015 03:04 PM, Arnd Bergmann wrote:
>>> On Sunday 18 October 2015 18:00:13 Eric Auger wrote:
>>>> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>>> index 619dc7d..4f76b17 100644
>>>> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>>> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
>>>> @@ -29,8 +29,7 @@
>>>>  #define DRIVER_VERSION  "0.1"
>>>>  #define DRIVER_AUTHOR   "Eric Auger <eric.auger@linaro.org>"
>>>>  #define DRIVER_DESC     "Reset support for Calxeda xgmac vfio platform device"
>>>> -
>>>> -#define CALXEDAXGMAC_COMPAT "calxeda,hb-xgmac"
>>>> +#define COMPAT		"calxeda,hb-xgmac"
>>>
>>> Why the rename?
>> This define was not used. Since the code is meant to be duplicated from
>> one reset module to the other I thought it did not bring any have a
>> specialized name
> 
> I'd say it would be clearer to remove the macro then, and pass the
> string literal in the function call.
OK
> 
>>>>  /* XGMAC Register definitions */
>>>>  #define XGMAC_CONTROL           0x00000000      /* MAC Configuration */
>>>> @@ -80,6 +79,43 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
>>>>  
>>>> +static int __init vfio_platform_calxedaxgmac_init(void)
>>>> +{
>>>> +	int (*register_reset)(struct module *, char*,
>>>> +				vfio_platform_reset_fn_t);
>>>> +	int ret;
>>>> +
>>>> +	register_reset = symbol_get(vfio_platform_register_reset);
>>>> +	if (!register_reset)
>>>> +		return -EINVAL;
>>>
>>> I don't understand what you do the symbol_get() here for.
>>> Why not just call that function directly
>> the function is implemented in a separate module. This is just to make
>> sure the vfio-platform module is loaded, in case the end-user attempts
>> to load the reset module without having this latter loaded.
> 
> The module loader does all this for you.
Ah OK

I will respin shortly taking into account all your comments

Thanks for your time!

Best Regards

Eric


> 
> 	Arnd
> 

[PATCH 3/4] vfio: platform: add compat in vfio_platform_device

[Eric Auger]

Let's retrieve the compatibility string on probe and store it
in the vfio_platform_device struct

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 drivers/vfio/platform/vfio_platform_common.c  | 15 ++++++++-------
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index d36afc9..31a6a8c 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -38,16 +38,11 @@ static const struct vfio_platform_reset_combo reset_lookup_table[] = {
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
 				    struct device *dev)
 {
-	const char *compat;
 	int (*reset)(struct vfio_platform_device *);
-	int ret, i;
-
-	ret = device_property_read_string(dev, "compatible", &compat);
-	if (ret)
-		return;
+	int i;
 
 	for (i = 0 ; i < ARRAY_SIZE(reset_lookup_table); i++) {
-		if (!strcmp(reset_lookup_table[i].compat, compat)) {
+		if (!strcmp(reset_lookup_table[i].compat, vdev->compat)) {
 			request_module(reset_lookup_table[i].module_name);
 			reset = __symbol_get(
 				reset_lookup_table[i].reset_function_name);
@@ -538,6 +533,12 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	struct iommu_group *group;
 	int ret;
 
+	ret = device_property_read_string(dev, "compatible", &vdev->compat);
+	if (ret) {
+		pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
+		return -EINVAL;
+	}
+
 	if (!vdev)
 		return -EINVAL;
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 17323f0..b274646 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -56,6 +56,7 @@ struct vfio_platform_device {
 	u32				num_irqs;
 	int				refcnt;
 	struct mutex			igate;
+	const char			*compat;
 
 	/*
 	 * These fields should be filled by the bus specific binder
-- 
1.9.1

[PATCH 4/4] vfio: platform: use list of registered reset function

[Eric Auger]

Remove the static lookup table and use the dynamic list of registered
reset functions instead. Also load the reset module through its alias.
The reset struct module pointer is stored in vfio_platform_device.

This patch fixes the issue related to the usage of __symbol_get, which
besides from being moot, prevented compilation with CONFIG_MODULES
disabled.

Also usage of MODULE_ALIAS makes possible to add a new reset module
without needing to update the framework. This was suggested by Arnd.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/vfio/platform/vfio_platform_common.c  | 46 +++++++++++++++------------
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 31a6a8c..f3b6299 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -27,37 +27,41 @@ struct list_head reset_list;
 LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
-static const struct vfio_platform_reset_combo reset_lookup_table[] = {
-	{
-		.compat = "calxeda,hb-xgmac",
-		.reset_function_name = "vfio_platform_calxedaxgmac_reset",
-		.module_name = "vfio-platform-calxedaxgmac",
-	},
-};
+static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
+					struct module **module)
+{
+	struct vfio_platform_reset_node *iter;
+
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			return iter->reset;
+		}
+	}
+
+	return NULL;
+}
 
 static void vfio_platform_get_reset(struct vfio_platform_device *vdev,
 				    struct device *dev)
 {
-	int (*reset)(struct vfio_platform_device *);
-	int i;
-
-	for (i = 0 ; i < ARRAY_SIZE(reset_lookup_table); i++) {
-		if (!strcmp(reset_lookup_table[i].compat, vdev->compat)) {
-			request_module(reset_lookup_table[i].module_name);
-			reset = __symbol_get(
-				reset_lookup_table[i].reset_function_name);
-			if (reset) {
-				vdev->reset = reset;
-				return;
-			}
-		}
+	char modname[256];
+
+	vdev->reset = vfio_platform_lookup_reset(vdev->compat,
+						&vdev->reset_module);
+	if (!vdev->reset) {
+		snprintf(modname, 256, "vfio-reset:%s", vdev->compat);
+		request_module(modname);
+		vdev->reset = vfio_platform_lookup_reset(vdev->compat,
+							 &vdev->reset_module);
 	}
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 {
 	if (vdev->reset)
-		symbol_put_addr(vdev->reset);
+		module_put(vdev->reset_module);
 }
 
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index b274646..2070dcc 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -57,6 +57,7 @@ struct vfio_platform_device {
 	int				refcnt;
 	struct mutex			igate;
 	const char			*compat;
+	struct module			*reset_module;
 
 	/*
 	 * These fields should be filled by the bus specific binder
-- 
1.9.1

[PATCH 0/4] VFIO platform reset module rework

[Arnd Bergmann]

On Sunday 18 October 2015 18:00:11 Eric Auger wrote:
> This series fixes the current implementation by getting rid of the
> usage of __symbol_get which caused a compilation issue with
> CONFIG_MODULES disabled. On top of this, the usage of MODULE_ALIAS makes
> possible to add a new reset module without being obliged to update the
> framework. The new implementation relies on the reset module registering
> its reset function to the vfio-platform driver.
> 
> The series is available at
> 
> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.3-rc5-rework-xgbe-v2
> 
> 

Looks good for the most part, just a few comments on coding style
for patches 1 and 2.

	Arnd