* [PATCH v2 1/6] vfio: platform: add capability to register a reset function
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
@ 2015-10-22 9:41 ` Eric Auger
2015-10-22 10:06 ` Arnd Bergmann
2015-10-22 9:41 ` [PATCH v2 2/6] vfio: platform: reset: add vfio_platform_reset_private.h Eric Auger
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:41 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 1/6] vfio: platform: add capability to register a reset function
2015-10-22 9:41 ` [PATCH v2 1/6] vfio: platform: add capability to register a reset function Eric Auger
@ 2015-10-22 10:06 ` Arnd Bergmann
0 siblings, 0 replies; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 10:06 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/6] vfio: platform: reset: add vfio_platform_reset_private.h
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
2015-10-22 9:41 ` [PATCH v2 1/6] vfio: platform: add capability to register a reset function Eric Auger
@ 2015-10-22 9:41 ` Eric Auger
2015-10-22 10:12 ` Arnd Bergmann
2015-10-22 9:41 ` [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration Eric Auger
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:41 UTC (permalink / raw)
To: linux-arm-kernel
This header is to be included in all vfio reset modules. It
defines the module_vfio_reset_handler macro whose role is
- to define a module alias
- implement module init/exit function which respectively registers
and unregisters the reset function.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v2: creation
- this defines the module_vfio_reset_handler macro as suggested by Arnd
Although Arnd suggested me to remove the vfio_platform_register_reset
symbol_get (since the module manager can handle the case where the
vfio-platform driver is not loaded), I prefered to keep it while
introducing the macro. The rationale is, when using symbol_get/put
we are able to release the hold from the reset module on vfio-platform
as soon as the registration is complete and I think this makes sense.
---
drivers/vfio/platform/reset/Makefile | 2 +-
.../platform/reset/vfio_platform_reset_private.h | 66 ++++++++++++++++++++++
2 files changed, 67 insertions(+), 1 deletion(-)
create mode 100644 drivers/vfio/platform/reset/vfio_platform_reset_private.h
diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
index 2a486af..154a7d5 100644
--- a/drivers/vfio/platform/reset/Makefile
+++ b/drivers/vfio/platform/reset/Makefile
@@ -1,5 +1,5 @@
vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
-ccflags-y += -Idrivers/vfio/platform
+ccflags-y += -Idrivers/vfio/platform -Idrivers/vfio/platform/reset
obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_reset_private.h b/drivers/vfio/platform/reset/vfio_platform_reset_private.h
new file mode 100644
index 0000000..f212a61
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_reset_private.h
@@ -0,0 +1,66 @@
+/*
+ * Interface used by VFIO platform reset modules to register/unregister
+ * their reset function
+ *
+ * Copyright (c) 2015 Linaro Ltd.
+ * www.linaro.org
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef VFIO_PLATFORM_RESET_PRIVATE_H
+#define VFIO_PLATFORM_RESET_PRIVATE_H
+
+#include <linux/module.h>
+#include "vfio_platform_private.h"
+
+static int reset_module_register(struct module *module,
+ const char *compat,
+ vfio_platform_reset_fn_t reset)
+{
+ int (*register_reset)(struct module *, const char*,
+ vfio_platform_reset_fn_t);
+ int ret;
+
+ register_reset = symbol_get(vfio_platform_register_reset);
+ if (!register_reset)
+ return -EINVAL;
+ ret = register_reset(module, compat, reset);
+ symbol_put(vfio_platform_register_reset);
+ return ret;
+}
+
+static void reset_module_unregister(const char *compat)
+{
+ int (*unregister_reset)(const char *);
+
+ unregister_reset = symbol_get(vfio_platform_unregister_reset);
+ if (!unregister_reset)
+ return;
+
+ unregister_reset(compat);
+
+ symbol_put(vfio_platform_unregister_reset);
+}
+
+#define module_vfio_reset_handler(compat, reset) \
+MODULE_ALIAS("vfio-reset:" compat); \
+static int __init reset ## _module_init(void) \
+{ \
+ return reset_module_register(THIS_MODULE, compat, &reset); \
+}; \
+static void __exit reset ## _module_exit(void) \
+{ \
+ reset_module_unregister(compat); \
+}; \
+module_init(reset ## _module_init); \
+module_exit(reset ## _module_exit)
+
+#endif /* VFIO_PLATFORM_RESET_PRIVATE_H */
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 2/6] vfio: platform: reset: add vfio_platform_reset_private.h
2015-10-22 9:41 ` [PATCH v2 2/6] vfio: platform: reset: add vfio_platform_reset_private.h Eric Auger
@ 2015-10-22 10:12 ` Arnd Bergmann
0 siblings, 0 replies; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 10:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 11:41:58 Eric Auger wrote:
> v2: creation
> - this defines the module_vfio_reset_handler macro as suggested by Arnd
>
> Although Arnd suggested me to remove the vfio_platform_register_reset
> symbol_get (since the module manager can handle the case where the
> vfio-platform driver is not loaded), I prefered to keep it while
> introducing the macro. The rationale is, when using symbol_get/put
> we are able to release the hold from the reset module on vfio-platform
> as soon as the registration is complete and I think this makes sense.
This makes it highly unusual. I can't think of a good reason to allow
unloading the vfio platform module while a reset module is registered,
that just causes a memory leak (or possibly a crash) when you do
unload the core module while it's used by a reset driver, it
prevents the reset module from getting loaded without loading the
vfio module first (because the autoloading doesn't work), and it
means we don't catch build errors in invalid configurations where you
have only the reset driver but not the vfio driver enabled.
Arnd
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
2015-10-22 9:41 ` [PATCH v2 1/6] vfio: platform: add capability to register a reset function Eric Auger
2015-10-22 9:41 ` [PATCH v2 2/6] vfio: platform: reset: add vfio_platform_reset_private.h Eric Auger
@ 2015-10-22 9:41 ` Eric Auger
2015-10-22 10:13 ` Arnd Bergmann
2015-10-22 9:42 ` [PATCH v2 4/6] vfio: platform: add compat in vfio_platform_device Eric Auger
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:41 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds the reset function registration/unregistration.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v1 -> v2:
- uses the module_vfio_reset_handler macro
- add pr_info on vfio reset
- do not export vfio_platform_calxedaxgmac_reset symbol anymore
---
drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index 619dc7d..8b2cccc 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -25,13 +25,12 @@
#include <linux/io.h>
#include "vfio_platform_private.h"
+#include "vfio_platform_reset_private.h"
#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"
-
/* XGMAC Register definitions */
#define XGMAC_CONTROL 0x00000000 /* MAC Configuration */
@@ -70,6 +69,8 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
return -ENOMEM;
}
+ pr_info("VFIO reset of %s\n", vdev->name);
+
/* disable IRQ */
writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
@@ -78,7 +79,9 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
return 0;
}
-EXPORT_SYMBOL_GPL(vfio_platform_calxedaxgmac_reset);
+
+module_vfio_reset_handler("calxeda,hb-xgmac",
+ vfio_platform_calxedaxgmac_reset);
MODULE_VERSION(DRIVER_VERSION);
MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration
2015-10-22 9:41 ` [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration Eric Auger
@ 2015-10-22 10:13 ` Arnd Bergmann
2015-10-22 11:54 ` Eric Auger
0 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 10:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 11:41:59 Eric Auger wrote:
> This patch adds the reset function registration/unregistration.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
Looks good, except one thing:
> @@ -70,6 +69,8 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
> return -ENOMEM;
> }
>
> + pr_info("VFIO reset of %s\n", vdev->name);
> +
> /* disable IRQ */
> writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
>
This probably slipped in from debugging, please remove it.
Arnd
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration
2015-10-22 10:13 ` Arnd Bergmann
@ 2015-10-22 11:54 ` Eric Auger
2015-10-22 12:09 ` Arnd Bergmann
0 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 11:54 UTC (permalink / raw)
To: linux-arm-kernel
On 10/22/2015 12:13 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:41:59 Eric Auger wrote:
>> This patch adds the reset function registration/unregistration.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> Looks good, except one thing:
>> @@ -70,6 +69,8 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>> return -ENOMEM;
>> }
>>
>> + pr_info("VFIO reset of %s\n", vdev->name);
>> +
>> /* disable IRQ */
>> writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
>>
>
> This probably slipped in from debugging, please remove it.
Well actually this is not an oversight but some unappropriate tracing
attempt I am afraid. I wanted to add a trace useful for the end-user to
make sure the VFIO reset function was called. Do you forbid that or do
recommend to use another tracing mechanism/level? In the past I tried
dynamic tracing but with module loading mechanism I found it not that handy.
Eric
>
> Arnd
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration
2015-10-22 11:54 ` Eric Auger
@ 2015-10-22 12:09 ` Arnd Bergmann
2015-10-22 12:29 ` Eric Auger
0 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 12:09 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 13:54:42 Eric Auger wrote:
> On 10/22/2015 12:13 PM, Arnd Bergmann wrote:
> > On Thursday 22 October 2015 11:41:59 Eric Auger wrote:
> >> This patch adds the reset function registration/unregistration.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> >
> > Looks good, except one thing:
> >> @@ -70,6 +69,8 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
> >> return -ENOMEM;
> >> }
> >>
> >> + pr_info("VFIO reset of %s\n", vdev->name);
> >> +
> >> /* disable IRQ */
> >> writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
> >>
> >
> > This probably slipped in from debugging, please remove it.
> Well actually this is not an oversight but some unappropriate tracing
> attempt I am afraid. I wanted to add a trace useful for the end-user to
> make sure the VFIO reset function was called. Do you forbid that or do
> recommend to use another tracing mechanism/level? In the past I tried
> dynamic tracing but with module loading mechanism I found it not that handy.
If you think it's useful to have in the long run, it should be a separate
patch with a description what it's good for, rather than a side-effect of
an unrelated patch. It just looked to me like it's something you do
while debugging the reset code, rather than while using it.
Implementation-wise, you should use dev_info() instead of pr_info() where
possible, and it probably makes sense to put this into the vfio_platform
driver before calling the reset function, for consistency between the
drivers.
Arnd
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration
2015-10-22 12:09 ` Arnd Bergmann
@ 2015-10-22 12:29 ` Eric Auger
0 siblings, 0 replies; 23+ messages in thread
From: Eric Auger @ 2015-10-22 12:29 UTC (permalink / raw)
To: linux-arm-kernel
On 10/22/2015 02:09 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 13:54:42 Eric Auger wrote:
>> On 10/22/2015 12:13 PM, Arnd Bergmann wrote:
>>> On Thursday 22 October 2015 11:41:59 Eric Auger wrote:
>>>> This patch adds the reset function registration/unregistration.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> Looks good, except one thing:
>>>> @@ -70,6 +69,8 @@ int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> + pr_info("VFIO reset of %s\n", vdev->name);
>>>> +
>>>> /* disable IRQ */
>>>> writel(0, reg.ioaddr + XGMAC_DMA_INTR_ENA);
>>>>
>>>
>>> This probably slipped in from debugging, please remove it.
>> Well actually this is not an oversight but some unappropriate tracing
>> attempt I am afraid. I wanted to add a trace useful for the end-user to
>> make sure the VFIO reset function was called. Do you forbid that or do
>> recommend to use another tracing mechanism/level? In the past I tried
>> dynamic tracing but with module loading mechanism I found it not that handy.
>
> If you think it's useful to have in the long run, it should be a separate
> patch with a description what it's good for, rather than a side-effect of
> an unrelated patch. It just looked to me like it's something you do
> while debugging the reset code, rather than while using it.
>
> Implementation-wise, you should use dev_info() instead of pr_info() where
> possible, and it probably makes sense to put this into the vfio_platform
> driver before calling the reset function, for consistency between the
> drivers.
OK thanks
Best Regards
Eric
>
> Arnd
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 4/6] vfio: platform: add compat in vfio_platform_device
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
` (2 preceding siblings ...)
2015-10-22 9:41 ` [PATCH v2 3/6] vfio: platform: reset: calxedaxgmac: add reset function registration Eric Auger
@ 2015-10-22 9:42 ` Eric Auger
2015-10-22 9:42 ` [PATCH v2 5/6] vfio: platform: use list of registered reset function Eric Auger
2015-10-22 9:42 ` [PATCH v2 6/6] vfio: platform: move get/put reset at open/release Eric Auger
5 siblings, 0 replies; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:42 UTC (permalink / raw)
To: linux-arm-kernel
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 52a4c7b..e7d615f 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -45,16 +45,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);
@@ -545,6 +540,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 1fb1410..620ee40 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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 5/6] vfio: platform: use list of registered reset function
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
` (3 preceding siblings ...)
2015-10-22 9:42 ` [PATCH v2 4/6] vfio: platform: add compat in vfio_platform_device Eric Auger
@ 2015-10-22 9:42 ` Eric Auger
2015-10-22 10:19 ` Arnd Bergmann
2015-10-22 9:42 ` [PATCH v2 6/6] vfio: platform: move get/put reset at open/release Eric Auger
5 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 5/6] vfio: platform: use list of registered reset function
2015-10-22 9:42 ` [PATCH v2 5/6] vfio: platform: use list of registered reset function Eric Auger
@ 2015-10-22 10:19 ` Arnd Bergmann
2015-10-22 11:46 ` Eric Auger
0 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 10:19 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v2 5/6] vfio: platform: use list of registered reset function
2015-10-22 10:19 ` Arnd Bergmann
@ 2015-10-22 11:46 ` Eric Auger
0 siblings, 0 replies; 23+ messages in thread
From: Eric Auger @ 2015-10-22 11:46 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 9:41 [PATCH v2 0/6] VFIO platform reset module rework Eric Auger
` (4 preceding siblings ...)
2015-10-22 9:42 ` [PATCH v2 5/6] vfio: platform: use list of registered reset function Eric Auger
@ 2015-10-22 9:42 ` Eric Auger
2015-10-22 10:29 ` Arnd Bergmann
5 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 9:42 UTC (permalink / raw)
To: linux-arm-kernel
Currently reset lookup is done on probe. This introduces a
race with new registration mechanism in the case where the
vfio-platform driver is bound to the device before its module
is loaded: on the load, the probe happens which triggers the
reset module load which itself attempts to get the symbol for
the registration function (vfio_platform_register_reset). The
symbol is not yet available hence the lookup fails. In case we
do the lookup in the first open we are sure the vfio-platform
module is loaded and vfio_platform_register_reset is available.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
drivers/vfio/platform/vfio_platform_common.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 95b8294..5ebae8c 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -155,6 +155,7 @@ static void vfio_platform_release(void *device_data)
vdev->reset(vdev);
vfio_platform_regions_cleanup(vdev);
vfio_platform_irq_cleanup(vdev);
+ vfio_platform_put_reset(vdev);
}
mutex_unlock(&driver_lock);
@@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
if (ret)
goto err_irq;
+ vfio_platform_get_reset(vdev);
+
if (vdev->reset)
vdev->reset(vdev);
}
@@ -570,8 +573,6 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
return ret;
}
- vfio_platform_get_reset(vdev);
-
mutex_init(&vdev->igate);
return 0;
@@ -585,7 +586,6 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
vdev = vfio_del_group_dev(dev);
if (vdev) {
- vfio_platform_put_reset(vdev);
iommu_group_put(dev->iommu_group);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 9:42 ` [PATCH v2 6/6] vfio: platform: move get/put reset at open/release Eric Auger
@ 2015-10-22 10:29 ` Arnd Bergmann
2015-10-22 11:40 ` Eric Auger
2015-10-22 13:26 ` Eric Auger
0 siblings, 2 replies; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 10:29 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
> Currently reset lookup is done on probe. This introduces a
> race with new registration mechanism in the case where the
> vfio-platform driver is bound to the device before its module
> is loaded: on the load, the probe happens which triggers the
> reset module load which itself attempts to get the symbol for
> the registration function (vfio_platform_register_reset). The
> symbol is not yet available hence the lookup fails. In case we
> do the lookup in the first open we are sure the vfio-platform
> module is loaded and vfio_platform_register_reset is available.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
I don't understand the explanation. I would expect the request_module()
call to block until the module is actually loaded. Is this not
what happens?
> mutex_unlock(&driver_lock);
> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
> if (ret)
> goto err_irq;
>
> + vfio_platform_get_reset(vdev);
> +
> if (vdev->reset)
> vdev->reset(vdev);
>
This needs some error handling to ensure that the open() fails
if there is no reset handler.
Arnd
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 10:29 ` Arnd Bergmann
@ 2015-10-22 11:40 ` Eric Auger
2015-10-22 12:05 ` Arnd Bergmann
2015-10-22 13:26 ` Eric Auger
1 sibling, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 11:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
>> Currently reset lookup is done on probe. This introduces a
>> race with new registration mechanism in the case where the
>> vfio-platform driver is bound to the device before its module
>> is loaded: on the load, the probe happens which triggers the
>> reset module load which itself attempts to get the symbol for
>> the registration function (vfio_platform_register_reset). The
>> symbol is not yet available hence the lookup fails. In case we
>> do the lookup in the first open we are sure the vfio-platform
>> module is loaded and vfio_platform_register_reset is available.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> I don't understand the explanation. I would expect the request_module()
> call to block until the module is actually loaded. Is this not
> what happens?
Again many thanks for this new review.
My understanding is the following
1) vfio-platform is attached to the device through the override mechanism
2) vfio-platform get loaded through modprobe:
2-1) the platform driver init function eventually calls the
vfio-platform probe function.
2-2) request_module of vfio-platform-calxedaxgmac gets called.
3) The init of vfio-platform-calxedaxgmac looks for
vfio_platform_register_reset. Unfortunately at that stage the init of
vfio-platform is not completed so the symbol is not available
3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
of today, this latter simply returns -EINVAL. Reset registration failed
but no stall.
3-2) in case symbol_get is *not* used, I think the module loader
attempts to load vfio-platform, which is already under load and this
causes a stall.
Let me know if you think this understanding is not correct.
Best Regards
Eric
>
>> mutex_unlock(&driver_lock);
>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>> if (ret)
>> goto err_irq;
>>
>> + vfio_platform_get_reset(vdev);
>> +
>> if (vdev->reset)
>> vdev->reset(vdev);
>>
>
> This needs some error handling to ensure that the open() fails
> if there is no reset handler.
>
> Arnd
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 11:40 ` Eric Auger
@ 2015-10-22 12:05 ` Arnd Bergmann
2015-10-22 12:27 ` Eric Auger
0 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 12:05 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 13:40:16 Eric Auger wrote:
> On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> > On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
> >> Currently reset lookup is done on probe. This introduces a
> >> race with new registration mechanism in the case where the
> >> vfio-platform driver is bound to the device before its module
> >> is loaded: on the load, the probe happens which triggers the
> >> reset module load which itself attempts to get the symbol for
> >> the registration function (vfio_platform_register_reset). The
> >> symbol is not yet available hence the lookup fails. In case we
> >> do the lookup in the first open we are sure the vfio-platform
> >> module is loaded and vfio_platform_register_reset is available.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> >
> > I don't understand the explanation. I would expect the request_module()
> > call to block until the module is actually loaded. Is this not
> > what happens?
>
> Again many thanks for this new review.
>
> My understanding is the following
> 1) vfio-platform is attached to the device through the override mechanism
> 2) vfio-platform get loaded through modprobe:
> 2-1) the platform driver init function eventually calls the
> vfio-platform probe function.
> 2-2) request_module of vfio-platform-calxedaxgmac gets called.
> 3) The init of vfio-platform-calxedaxgmac looks for
> vfio_platform_register_reset. Unfortunately at that stage the init of
> vfio-platform is not completed so the symbol is not available
> 3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
> of today, this latter simply returns -EINVAL. Reset registration failed
> but no stall.
> 3-2) in case symbol_get is *not* used, I think the module loader
> attempts to load vfio-platform, which is already under load and this
> causes a stall.
>
> Let me know if you think this understanding is not correct.
I was expecting the vfio_platform_register_reset() function to be
available by the time of step 2-2.
What I think is going on here is that we are still inside of the
module_init() function of the vfio-platform driver at the time that
we call the request_module(), and the symbols are not made visible
to other modules until the module_init function returns with success.
This is a side-effect of the probe function for the platform driver
getting called from deep inside of the platform_driver_register()
function for all devices that are already present.
I think it already works for the AMBA case, which uses separate modules,
but we need to restructure the platform_device case slightly to do
the same:
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 9ce8afe28450..a00a44814255 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,10 +1,12 @@
-vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
+vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
-obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += vfio_platform.o
+obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
obj-$(CONFIG_VFIO_PLATFORM) += reset/
vfio-amba-y := vfio_amba.o
obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
+obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
obj-$(CONFIG_VFIO_AMBA) += reset/
Arnd
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 12:05 ` Arnd Bergmann
@ 2015-10-22 12:27 ` Eric Auger
0 siblings, 0 replies; 23+ messages in thread
From: Eric Auger @ 2015-10-22 12:27 UTC (permalink / raw)
To: linux-arm-kernel
On 10/22/2015 02:05 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 13:40:16 Eric Auger wrote:
>> On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
>>> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
>>>> Currently reset lookup is done on probe. This introduces a
>>>> race with new registration mechanism in the case where the
>>>> vfio-platform driver is bound to the device before its module
>>>> is loaded: on the load, the probe happens which triggers the
>>>> reset module load which itself attempts to get the symbol for
>>>> the registration function (vfio_platform_register_reset). The
>>>> symbol is not yet available hence the lookup fails. In case we
>>>> do the lookup in the first open we are sure the vfio-platform
>>>> module is loaded and vfio_platform_register_reset is available.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> I don't understand the explanation. I would expect the request_module()
>>> call to block until the module is actually loaded. Is this not
>>> what happens?
>>
>> Again many thanks for this new review.
>>
>> My understanding is the following
>> 1) vfio-platform is attached to the device through the override mechanism
>> 2) vfio-platform get loaded through modprobe:
>> 2-1) the platform driver init function eventually calls the
>> vfio-platform probe function.
>> 2-2) request_module of vfio-platform-calxedaxgmac gets called.
>> 3) The init of vfio-platform-calxedaxgmac looks for
>> vfio_platform_register_reset. Unfortunately at that stage the init of
>> vfio-platform is not completed so the symbol is not available
>> 3-1) in case symbol_get is used in vfio_platform_calxedaxgmac init, as
>> of today, this latter simply returns -EINVAL. Reset registration failed
>> but no stall.
>> 3-2) in case symbol_get is *not* used, I think the module loader
>> attempts to load vfio-platform, which is already under load and this
>> causes a stall.
>>
>> Let me know if you think this understanding is not correct.
>
> I was expecting the vfio_platform_register_reset() function to be
> available by the time of step 2-2.
>
> What I think is going on here is that we are still inside of the
> module_init() function of the vfio-platform driver at the time that
> we call the request_module(), and the symbols are not made visible
> to other modules until the module_init function returns with success.
> This is a side-effect of the probe function for the platform driver
> getting called from deep inside of the platform_driver_register()
> function for all devices that are already present.
yes we share the same understanding, this is what I meant.
>
> I think it already works for the AMBA case, which uses separate modules,
> but we need to restructure the platform_device case slightly to do
> the same:
OK I will test the new module structure below and eventually remove the
symbol_get (I got the link issue). Thanks for the hint!
Eric
>
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 9ce8afe28450..a00a44814255 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,10 +1,12 @@
>
> -vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
> +vfio-platform-base-y := vfio_platform_common.o vfio_platform_irq.o
>
> -obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> +obj-$(CONFIG_VFIO_PLATFORM) += vfio_platform.o
> +obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
> obj-$(CONFIG_VFIO_PLATFORM) += reset/
>
> vfio-amba-y := vfio_amba.o
>
> obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
> +obj-$(CONFIG_VFIO_AMBA) += vfio-platform-base.o
> obj-$(CONFIG_VFIO_AMBA) += reset/
>
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 10:29 ` Arnd Bergmann
2015-10-22 11:40 ` Eric Auger
@ 2015-10-22 13:26 ` Eric Auger
2015-10-22 14:10 ` Arnd Bergmann
1 sibling, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 13:26 UTC (permalink / raw)
To: linux-arm-kernel
On 10/22/2015 12:29 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 11:42:02 Eric Auger wrote:
>> Currently reset lookup is done on probe. This introduces a
>> race with new registration mechanism in the case where the
>> vfio-platform driver is bound to the device before its module
>> is loaded: on the load, the probe happens which triggers the
>> reset module load which itself attempts to get the symbol for
>> the registration function (vfio_platform_register_reset). The
>> symbol is not yet available hence the lookup fails. In case we
>> do the lookup in the first open we are sure the vfio-platform
>> module is loaded and vfio_platform_register_reset is available.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> I don't understand the explanation. I would expect the request_module()
> call to block until the module is actually loaded. Is this not
> what happens?
>
>> mutex_unlock(&driver_lock);
>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>> if (ret)
>> goto err_irq;
>>
>> + vfio_platform_get_reset(vdev);
>> +
>> if (vdev->reset)
>> vdev->reset(vdev);
>>
>
> This needs some error handling to ensure that the open() fails
> if there is no reset handler.
Is that really what we want? The code was meant to allow the use case
where the VFIO platform driver would be used without such reset module.
I think the imperious need for a reset module depends on the device and
more importantly depends on the IOMMU mapping. With QEMU VFIO
integration this is needed because the whole VM memory is IOMMU mapped
but in a simpler user-space driver context, we might live without.
Any thought?
Eric
>
> Arnd
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 13:26 ` Eric Auger
@ 2015-10-22 14:10 ` Arnd Bergmann
2015-10-22 14:23 ` Eric Auger
0 siblings, 1 reply; 23+ messages in thread
From: Arnd Bergmann @ 2015-10-22 14:10 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
> >> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
> >> if (ret)
> >> goto err_irq;
> >>
> >> + vfio_platform_get_reset(vdev);
> >> +
> >> if (vdev->reset)
> >> vdev->reset(vdev);
> >>
> >
> > This needs some error handling to ensure that the open() fails
> > if there is no reset handler.
>
> Is that really what we want? The code was meant to allow the use case
> where the VFIO platform driver would be used without such reset module.
>
> I think the imperious need for a reset module depends on the device and
> more importantly depends on the IOMMU mapping. With QEMU VFIO
> integration this is needed because the whole VM memory is IOMMU mapped
> but in a simpler user-space driver context, we might live without.
>
> Any thought?
I would think we need a reset driver for any device that can start DMA,
otherwise things can go wrong as soon as you attach it to a different domain
while there is ongoing DMA.
Maybe we could just allow devices to be attached without a reset handler,
but then disallow DMA on them?
Arnd
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 14:10 ` Arnd Bergmann
@ 2015-10-22 14:23 ` Eric Auger
2015-10-22 15:40 ` Alex Williamson
0 siblings, 1 reply; 23+ messages in thread
From: Eric Auger @ 2015-10-22 14:23 UTC (permalink / raw)
To: linux-arm-kernel
On 10/22/2015 04:10 PM, Arnd Bergmann wrote:
> On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
>>>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
>>>> if (ret)
>>>> goto err_irq;
>>>>
>>>> + vfio_platform_get_reset(vdev);
>>>> +
>>>> if (vdev->reset)
>>>> vdev->reset(vdev);
>>>>
>>>
>>> This needs some error handling to ensure that the open() fails
>>> if there is no reset handler.
>>
>> Is that really what we want? The code was meant to allow the use case
>> where the VFIO platform driver would be used without such reset module.
>>
>> I think the imperious need for a reset module depends on the device and
>> more importantly depends on the IOMMU mapping. With QEMU VFIO
>> integration this is needed because the whole VM memory is IOMMU mapped
>> but in a simpler user-space driver context, we might live without.
>>
>> Any thought?
>
> I would think we need a reset driver for any device that can start DMA,
> otherwise things can go wrong as soon as you attach it to a different domain
> while there is ongoing DMA.
>
> Maybe we could just allow devices to be attached without a reset handler,
> but then disallow DMA on them?
Well I am tempted to think that most assigned devices will perform DMA
accesses so to me this somehow comes to the same result, ie disallowing
functional passthrough for devices not properly/fully integrated.
Alex/Baptiste, any opinion on this?
Thanks
Eric
>
> Arnd
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 6/6] vfio: platform: move get/put reset at open/release
2015-10-22 14:23 ` Eric Auger
@ 2015-10-22 15:40 ` Alex Williamson
0 siblings, 0 replies; 23+ messages in thread
From: Alex Williamson @ 2015-10-22 15:40 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2015-10-22 at 16:23 +0200, Eric Auger wrote:
> On 10/22/2015 04:10 PM, Arnd Bergmann wrote:
> > On Thursday 22 October 2015 15:26:55 Eric Auger wrote:
> >>>> @@ -181,6 +182,8 @@ static int vfio_platform_open(void *device_data)
> >>>> if (ret)
> >>>> goto err_irq;
> >>>>
> >>>> + vfio_platform_get_reset(vdev);
> >>>> +
> >>>> if (vdev->reset)
> >>>> vdev->reset(vdev);
> >>>>
> >>>
> >>> This needs some error handling to ensure that the open() fails
> >>> if there is no reset handler.
> >>
> >> Is that really what we want? The code was meant to allow the use case
> >> where the VFIO platform driver would be used without such reset module.
> >>
> >> I think the imperious need for a reset module depends on the device and
> >> more importantly depends on the IOMMU mapping. With QEMU VFIO
> >> integration this is needed because the whole VM memory is IOMMU mapped
> >> but in a simpler user-space driver context, we might live without.
> >>
> >> Any thought?
> >
> > I would think we need a reset driver for any device that can start DMA,
> > otherwise things can go wrong as soon as you attach it to a different domain
> > while there is ongoing DMA.
> >
> > Maybe we could just allow devices to be attached without a reset handler,
> > but then disallow DMA on them?
>
> Well I am tempted to think that most assigned devices will perform DMA
> accesses so to me this somehow comes to the same result, ie disallowing
> functional passthrough for devices not properly/fully integrated.
>
> Alex/Baptiste, any opinion on this?
We have an IOMMU and the user doesn't get access to the device until the
IOMMU domain is established. So, ideally yes, we should have a way to
reset the device, but I don't see it as a requirement. Thanks,
Alex
^ permalink raw reply [flat|nested] 23+ messages in thread