On Fri, Aug 17, 2018 at 11:41:24AM +0200, David Hildenbrand wrote:

On 17.08.2018 11:03, Rafael J. Wysocki wrote:

quoted

On Fri, Aug 17, 2018 at 10:56 AM David Hildenbrand [off-list ref] wrote:

quoted

On 17.08.2018 10:41, Greg Kroah-Hartman wrote:

quoted

On Fri, Aug 17, 2018 at 09:59:00AM +0200, David Hildenbrand wrote:

quoted

From: Vitaly Kuznetsov <vkuznets@redhat.com>

Well require to call add_memory()/add_memory_resource() with
device_hotplug_lock held, to avoid a lock inversion. Allow external modules
(e.g. hv_balloon) that make use of add_memory()/add_memory_resource() to
lock device hotplug.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
[modify patch description]
Signed-off-by: David Hildenbrand <redacted>
---
 drivers/base/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..9010b9e942b5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -700,11 +700,13 @@ void lock_device_hotplug(void)
 {
     mutex_lock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(lock_device_hotplug);

 void unlock_device_hotplug(void)
 {
     mutex_unlock(&device_hotplug_lock);
 }
+EXPORT_SYMBOL_GPL(unlock_device_hotplug);

If these are going to be "global" symbols, let's properly name them.
device_hotplug_lock/unlock would be better.  But I am _really_ nervous
about letting stuff outside of the driver core mess with this, as people
better know what they are doing.

The only "problem" is that we have kernel modules (for paravirtualized
devices) that call add_memory(). This is Hyper-V right now, but we might
have other ones in the future. Without them we would not have to export
it. We might also get kernel modules that want to call remove_memory() -
which will require the device_hotplug_lock as of now.

What we could do is

a) add_memory() -> _add_memory() and don't export it
b) add_memory() takes the device_hotplug_lock and calls _add_memory() .
We export that one.
c) Use add_memory() in external modules only

Similar wrapper would be needed e.g. for remove_memory() later on.

That would be safer IMO, as it would prevent developers from using
add_memory() without the lock, say.

If the lock is always going to be required for add_memory(), make it
hard (or event impossible) to use the latter without it.

If there are no objections, I'll go into that direction. But I'll wait
for more comments regarding the general concept first.

It is the middle of the merge window, and maintainers are really busy
right now.  I doubt you will get many review comments just yet...