Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1253676 > unrolled thread

[PATCH v2 0/6] VFIO platform reset module rework

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/6] VFIO platform reset module rework Eric Auger <eric.auger@linaro.org> - 2015-10-22 11:50 +0200
    [PATCH v2 1/6] vfio: platform: add capability to register a reset function Eric Auger <eric.auger@linaro.org> - 2015-10-22 11:50 +0200
      Re: [PATCH v2 1/6] vfio: platform: add capability to register a reset function Arnd Bergmann <arnd@arndb.de> - 2015-10-22 12:10 +0200
    [PATCH v2 5/6] vfio: platform: use list of registered reset function Eric Auger <eric.auger@linaro.org> - 2015-10-22 11:50 +0200
      Re: [PATCH v2 5/6] vfio: platform: use list of registered reset function Arnd Bergmann <arnd@arndb.de> - 2015-10-22 12:20 +0200
        Re: [PATCH v2 5/6] vfio: platform: use list of registered reset  function Eric Auger <eric.auger@linaro.org> - 2015-10-22 13:50 +0200

#1253676 — [PATCH v2 0/6] VFIO platform reset module rework

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-rc6-rework-v2

Best Regards

Eric

v1 -> v2:
* in vfio_platform_common.c:
  - move reset lookup at load time and put reset at release: this is to
    prevent a race between the 2 load module loads
  - reset_list becomes static
  - vfio_platform_register/unregister_reset take a const char * as compat
  - fix node link
  - remove old combo struct and cleanup proto of vfio_platform_get_reset
  - add mutex to protect the reset list
* in calxeda xgmac reset module
  - introduce vfio_platform_reset_private.h
  - use module_vfio_reset_handler macro
  - do not export vfio_platform_calxedaxgmac_reset symbol anymore
  - add a pr_info to show the device is reset by vfio reset module


Eric Auger (6):
  vfio: platform: add capability to register a reset function
  vfio: platform: reset: add vfio_platform_reset_private.h
  vfio: platform: reset: calxedaxgmac: add reset function registration
  vfio: platform: add compat in vfio_platform_device
  vfio: platform: use list of registered reset function
  vfio: platform: move get/put reset at open/release

 drivers/vfio/platform/reset/Makefile               |   2 +-
 .../platform/reset/vfio_platform_calxedaxgmac.c    |   9 +-
 .../platform/reset/vfio_platform_reset_private.h   |  66 +++++++++
 drivers/vfio/platform/vfio_platform_common.c       | 151 ++++++++++++++++-----
 drivers/vfio/platform/vfio_platform_private.h      |  15 +-
 5 files changed, 201 insertions(+), 42 deletions(-)
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_reset_private.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1253680 — [PATCH v2 1/6] vfio: platform: add capability to register a reset function

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>

---

v1 -> v2:
- reset_list becomes static
- vfio_platform_register/unregister_reset take a const char * as compat
- fix node leak
- add reset_lock to protect the reset list manipulation
- move vfio_platform_reset_node declaration in vfio_platform_common.c
---
 drivers/vfio/platform/vfio_platform_common.c  | 77 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  7 +++
 2 files changed, 84 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e43efb5..52a4c7b 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -23,6 +23,15 @@
 
 #include "vfio_platform_private.h"
 
+struct vfio_platform_reset_node {
+	struct list_head link;
+	char *compat;
+	struct module *owner;
+	vfio_platform_reset_fn_t reset;
+};
+
+static LIST_HEAD(reset_list);
+static DEFINE_MUTEX(reset_lock);
 static DEFINE_MUTEX(driver_lock);
 
 static const struct vfio_platform_reset_combo reset_lookup_table[] = {
@@ -573,3 +582,71 @@ 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,
+				 const char *compat,
+				 vfio_platform_reset_fn_t reset)
+{
+	struct vfio_platform_reset_node *node, *iter;
+	bool found = false;
+
+	mutex_lock(&reset_lock);
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat)) {
+			found = true;
+			break;
+		}
+	}
+	if (found) {
+		mutex_unlock(&reset_lock);
+		return -EINVAL;
+	}
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	if (!node) {
+		mutex_unlock(&reset_lock);
+		return -ENOMEM;
+	}
+
+	node->compat = kstrdup(compat, GFP_KERNEL);
+	if (!node->compat) {
+		kfree(node);
+		mutex_unlock(&reset_lock);
+		return -ENOMEM;
+	}
+
+	node->owner = reset_owner;
+	node->reset = reset;
+
+	list_add(&node->link, &reset_list);
+	mutex_unlock(&reset_lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_register_reset);
+
+int vfio_platform_unregister_reset(const char *compat)
+{
+	struct vfio_platform_reset_node *iter;
+	bool found = false;
+
+	mutex_lock(&reset_lock);
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat)) {
+			found = true;
+			break;
+		}
+	}
+	if (!found) {
+		mutex_unlock(&reset_lock);
+		return -EINVAL;
+	}
+
+	list_del(&iter->link);
+	kfree(iter->compat);
+	kfree(iter);
+	mutex_unlock(&reset_lock);
+
+	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..1fb1410 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -89,4 +89,11 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 					unsigned start, unsigned count,
 					void *data);
 
+typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
+
+extern int vfio_platform_register_reset(struct module *owner,
+					const char *compat,
+					vfio_platform_reset_fn_t reset);
+extern int vfio_platform_unregister_reset(const char *compat);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253688 — Re: [PATCH v2 1/6] vfio: platform: add capability to register a reset function

On Thursday 22 October 2015 11:41:57 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>

Looks correct to me now, just a little style comments.

>  drivers/vfio/platform/vfio_platform_common.c  | 77 +++++++++++++++++++++++++++
>  drivers/vfio/platform/vfio_platform_private.h |  7 +++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index e43efb5..52a4c7b 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -23,6 +23,15 @@
>  
>  #include "vfio_platform_private.h"
>  
> +struct vfio_platform_reset_node {
> +	struct list_head link;
> +	char *compat;
> +	struct module *owner;
> +	vfio_platform_reset_fn_t reset;
> +};
> +
> +static LIST_HEAD(reset_list);
> +static DEFINE_MUTEX(reset_lock);
>  static DEFINE_MUTEX(driver_lock);

I wonder if having a single mutex here would be enough. If you don't expect
drivers to register/unregister a lot, you could just use driver_lock here
as well.

>  static const struct vfio_platform_reset_combo reset_lookup_table[] = {
> @@ -573,3 +582,71 @@ 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,
> +				 const char *compat,
> +				 vfio_platform_reset_fn_t reset)
> +{
> +	struct vfio_platform_reset_node *node, *iter;
> +	bool found = false;
> +
> +	mutex_lock(&reset_lock);
> +	list_for_each_entry(iter, &reset_list, link) {
> +		if (!strcmp(iter->compat, compat)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (found) {
> +		mutex_unlock(&reset_lock);
> +		return -EINVAL;
> +	}

This seems to be an unnecesssary safeguard. I would not bother
with the search, or otherwise do a WARN_ON here.

> +	node = kmalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node) {
> +		mutex_unlock(&reset_lock);
> +		return -ENOMEM;
> +	}
> +
> +	node->compat = kstrdup(compat, GFP_KERNEL);
> +	if (!node->compat) {
> +		kfree(node);
> +		mutex_unlock(&reset_lock);
> +		return -ENOMEM;
> +	}

If you hold a lock, it's better to use a goto for handling errors
and put the unlock and kfree there. I think it can be avoided
entirely here though:

It should be safe to define the interface as keeping a reference
on the string and not do a kstrdup here. I would expect users to
pass a string literal.

We could even go as far as defining the entire interface as a shim,
like

#define vfio_platform_register_reset(__compat, __reset)	\
static struct vfio_platform_reset_node __reset ## _node = {	\
	.owner = THIS_MODULE,					\
	.compat = __compat,					\
	.reset = __reset,					\
};								\
__vfio_platform_register_reset(&__reset ## node);

to make the function really simple.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253681 — [PATCH v2 5/6] vfio: platform: use list of registered reset function

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.

We also remove the useless struct device pointer parameter in
vfio_platform_get_reset.

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>

---

v1 -> v2:
- use reset_lock in vfio_platform_lookup_reset
- remove vfio_platform_reset_combo declaration
- remove struct device *dev parameter in vfio_platform_get_reset
- set reset_module and reset to NULL in put function
---
 drivers/vfio/platform/vfio_platform_common.c  | 57 ++++++++++++++++-----------
 drivers/vfio/platform/vfio_platform_private.h |  7 +---
 2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e7d615f..95b8294 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -34,37 +34,46 @@ static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(reset_lock);
 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 void vfio_platform_get_reset(struct vfio_platform_device *vdev,
-				    struct device *dev)
+static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
+					struct module **module)
 {
-	int (*reset)(struct vfio_platform_device *);
-	int i;
+	struct vfio_platform_reset_node *iter;
 
-	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;
-			}
+	mutex_lock(&reset_lock);
+	list_for_each_entry(iter, &reset_list, link) {
+		if (!strcmp(iter->compat, compat) &&
+			try_module_get(iter->owner)) {
+			*module = iter->owner;
+			mutex_unlock(&reset_lock);
+			return iter->reset;
 		}
 	}
+
+	mutex_unlock(&reset_lock);
+	return NULL;
+}
+
+static void vfio_platform_get_reset(struct vfio_platform_device *vdev)
+{
+	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);
+	if (vdev->reset) {
+		module_put(vdev->reset_module);
+		vdev->reset_module = NULL;
+		vdev->reset = NULL;
+	}
 }
 
 static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
@@ -561,7 +570,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
-	vfio_platform_get_reset(vdev, dev);
+	vfio_platform_get_reset(vdev);
 
 	mutex_init(&vdev->igate);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 620ee40..e968454 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
@@ -71,12 +72,6 @@ struct vfio_platform_device {
 	int	(*reset)(struct vfio_platform_device *vdev);
 };
 
-struct vfio_platform_reset_combo {
-	const char *compat;
-	const char *reset_function_name;
-	const char *module_name;
-};
-
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 				      struct device *dev);
 extern struct vfio_platform_device *vfio_platform_remove_common
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253692 — Re: [PATCH v2 5/6] vfio: platform: use list of registered reset function

On Thursday 22 October 2015 11:42:01 Eric Auger wrote:
> 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.
> 
> We also remove the useless struct device pointer parameter in
> vfio_platform_get_reset.
> 
> 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>

Looks good, just small style issues:

> 
> -			}
> +	mutex_lock(&reset_lock);
> +	list_for_each_entry(iter, &reset_list, link) {
> +		if (!strcmp(iter->compat, compat) &&
> +			try_module_get(iter->owner)) {
> +			*module = iter->owner;
> +			mutex_unlock(&reset_lock);
> +			return iter->reset;
>  		}
>  	}
> +
> +	mutex_unlock(&reset_lock);
> +	return NULL;

Better use 'break' to have a single mutex_unlock and return  statement.

> +
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>  {
> -	if (vdev->reset)
> -		symbol_put_addr(vdev->reset);
> +	if (vdev->reset) {
> +		module_put(vdev->reset_module);
> +		vdev->reset_module = NULL;
> +		vdev->reset = NULL;
> +	}
>  }

This gets called from the remove callback. No need to clear those
struct members immediately before the kfree().

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1253731 — Re: [PATCH v2 5/6] vfio: platform: use list of registered reset function

On 10/22/2015 12:19 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:42:01 Eric Auger wrote:
>> 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.
>>
>> We also remove the useless struct device pointer parameter in
>> vfio_platform_get_reset.
>>
>> 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>
> 
> Looks good, just small style issues:
> 
>>
>> -			}
>> +	mutex_lock(&reset_lock);
>> +	list_for_each_entry(iter, &reset_list, link) {
>> +		if (!strcmp(iter->compat, compat) &&
>> +			try_module_get(iter->owner)) {
>> +			*module = iter->owner;
>> +			mutex_unlock(&reset_lock);
>> +			return iter->reset;
>>  		}
>>  	}
>> +
>> +	mutex_unlock(&reset_lock);
>> +	return NULL;
> 
> Better use 'break' to have a single mutex_unlock and return  statement.
OK
> 
>> +
>>  
>>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>  {
>> -	if (vdev->reset)
>> -		symbol_put_addr(vdev->reset);
>> +	if (vdev->reset) {
>> +		module_put(vdev->reset_module);
>> +		vdev->reset_module = NULL;
>> +		vdev->reset = NULL;
>> +	}
>>  }
> 
> This gets called from the remove callback. No need to clear those
> struct members immediately before the kfree().
I should have added those assignments in next patch actually. This
latter moves the vfio_platform_put_reset call in the release function.

Best Regards

Eric
> 
> 	Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web