virtio: make PCI devices take a virtio_pci module ref

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

Fix by simply making each device take a ref on the module.

Signed-off-by Mark McLoughlin <[email protected]>

Reported-by Michael Tokarev <[email protected]>

---
 drivers/virtio/virtio_pci.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Quoted 2 lines · Mark McLoughlin

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

It unbinds the device properly as any other driver. So what's the problem here?

Quoted 1 line

Reported-by Michael Tokarev <[email protected]>

Quoted 4 lines · Jiri Slaby

Quoted 2 lines · Mark McLoughlin

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

It unbinds the device properly as any other driver. So what's the problem here?

Here's what we get when rmmod'ing (a zero-refcounted but in use) virtio_pci (I did it by a chance, cut-n-pasted the wrong line):

WARNING: at drivers/base/core.c:122 device_release+0x5f/0x70()
Device 'virtio1' does not have a release() function, it is broken and must be fixed.
Modules linked in: ext3 jbd mbcache acpiphp dock pci_hotplug virtio_net virtio_blk virtio_pci(-) virtio_ring virtio

Pid: 361, comm: rmmod Tainted: G S 2.6.27-i686smp #2.6.27.7

 [<c012b81f>] warn_slowpath+0x6f/0xa0
 [<c0110030>] prepare_set+0x30/0x80
 [<c012067e>] __wake_up+0x3e/0x60
 [<c01d1d25>] release_sysfs_dirent+0x45/0xb0
 ...

Michael Tokarev napsal(a):

Quoted 14 lines

Quoted 4 lines · Jiri Slaby

Quoted 2 lines · Mark McLoughlin

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

It unbinds the device properly as any other driver. So what's the problem here?

Here's what we get when rmmod'ing (a zero-refcounted but in use) virtio_pci (I did it by a chance, cut-n-pasted the wrong line):

WARNING: at drivers/base/core.c:122 device_release+0x5f/0x70()
Device 'virtio1' does not have a release() function, it is broken and must be fixed.
Modules linked in: ext3 jbd mbcache acpiphp dock pci_hotplug virtio_net virtio_blk virtio_pci(-) virtio_ring virtio

Quoted 6 lines

Pid: 361, comm: rmmod Tainted: G S 2.6.27-i686smp #2.6.27.7

 [<c012b81f>] warn_slowpath+0x6f/0xa0
 [<c0110030>] prepare_set+0x30/0x80
 [<c012067e>] __wake_up+0x3e/0x60
 [<c01d1d25>] release_sysfs_dirent+0x45/0xb0
 ...

So why don't you fix the root cause and add such a crap into the probe function (not even counting probe can fail later)?

Fix the virtio bus instead.

Quoted 1 line

Fix the virtio bus instead.

Yeah, the patch I posted wasn't meant as a fix for this traceback.

Here's one that does fix it.

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] virtio: add device release() function

Add a release() function for virtio_pci devices so as to avoid:

Device 'virtio0' does not have a release() function, it is broken and must be fixed

The struct device is embedded in the struct virtio_pci_device which is freed by virtio_pci_remove(), so virtio_pci_release_dev() need not actually do anything.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio_pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/virtio/virtio_pci.c +0 -0

index c7dc37c..7d4899c 100644

Quoted 3 lines · Mark McLoughlin

Quoted 1 line

Fix the virtio bus instead.

Yeah, the patch I posted wasn't meant as a fix for this traceback.

So what's the module_get patch needed for?

Quoted 1 line

Here's one that does fix it.

...

Quoted 6 lines

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] virtio: add device release() function

Add a release() function for virtio_pci devices so as to avoid:

Device 'virtio0' does not have a release() function, it is broken and must be fixed

Quoted 15 lines · Jiri Slaby

Quoted 3 lines · Mark McLoughlin

Quoted 1 line

Fix the virtio bus instead.

Yeah, the patch I posted wasn't meant as a fix for this traceback.

So what's the module_get patch needed for?

Quoted 1 line

Here's one that does fix it.

...

Quoted 6 lines

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] virtio: add device release() function

Add a release() function for virtio_pci devices so as to avoid:

Device 'virtio0' does not have a release() function, it is broken and must be fixed

Just providing an empty release function to the kernel is the complete wrong thing. Do you not think the kernel is actually trying to tell you something here? If it could test for an empty release function it would complain about that as well, providing one is no "fix" at all.

You need to free your memory in the release function that is owned by the device/structure. Please read the file, Documentation/kobject.txt for details as to what you need to do.

Quoted 22 lines · Greg KH

Quoted 15 lines · Jiri Slaby

Quoted 3 lines · Mark McLoughlin

Quoted 1 line

Fix the virtio bus instead.

Yeah, the patch I posted wasn't meant as a fix for this traceback.

So what's the module_get patch needed for?

Quoted 1 line

Here's one that does fix it.

...

Quoted 6 lines

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] virtio: add device release() function

Add a release() function for virtio_pci devices so as to avoid:

Device 'virtio0' does not have a release() function, it is broken and must be fixed

Just providing an empty release function to the kernel is the complete wrong thing. Do you not think the kernel is actually trying to tell you something here? If it could test for an empty release function it would complain about that as well, providing one is no "fix" at all.

Quoted 3 lines

You need to free your memory in the release function that is owned by the device/structure. Please read the file, Documentation/kobject.txt for details as to what you need to do.

Okay, consider me "mocked mercilessly by the kobject maintainer" :-)

Does this version look a bit more reasonable?

(The virtio_pci_root is statically allocated so I don't see how release() could be non-empty in this case, but let's debate whether we want to keep this dummy device at all)

Apparently we shouldn't be statically allocating the root device object, so dynamically allocate it instead.

Also add a release() function to avoid this warning:

Device 'virtio-pci' does not have a release() function, it is broken and must be fixed

Signed-off-by Mark McLoughlin <[email protected]>

Cc Anthony Liguori <[email protected]>

---
 drivers/virtio/virtio_pci.c |   48 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 40 insertions(+), 8 deletions(-)

drivers/virtio/virtio_pci.c +40 -8

index 265fdf2..939e0b4 100644
@@ -73,10 +73,42 @@ MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
 /* A PCI device has it's own struct device and so does a virtio device so
  * we create a place for the virtio devices to show up in sysfs.  I think it
  * would make more sense for virtio to not insist on having it's own device. */
-static struct device virtio_pci_root = {
-	.parent		= NULL,
-	.init_name	= "virtio-pci",
-};
+static struct device *virtio_pci_root;
+
+static void virtio_pci_release_root(struct device *root)
+{
+	kfree(root);
+}
+
+static int virtio_pci_init_root(void)
+{
+	struct device *root;
+	int err = -ENOMEM;
+
+	root = kzalloc(sizeof(struct device), GFP_KERNEL);
+	if (root == NULL)
+		goto out;
+
+	err = dev_set_name(root, "virtio-pci");
+	if (err)
+		goto free_root;
+
+	err = device_register(root);
+	if (err)
+		goto free_root;
+
+	root->parent  = NULL;
+	root->release = virtio_pci_release_root;
+
+	virtio_pci_root = root;
+
+	return 0;
+
+free_root:
+	kfree(root);
+out:
+	return err;
+}
 
 /* Convert a generic virtio device to our structure */
 static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)

@@ -343,7 +375,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	if (vp_dev == NULL)
 		return -ENOMEM;
 
-	vp_dev->vdev.dev.parent = &virtio_pci_root;
+	vp_dev->vdev.dev.parent = virtio_pci_root;
 	vp_dev->vdev.dev.release = virtio_pci_release_dev;
 	vp_dev->vdev.config = &virtio_pci_config_ops;
 	vp_dev->pci_dev = pci_dev;

@@ -437,13 +469,13 @@ static int __init virtio_pci_init(void)
 {
 	int err;
 
-	err = device_register(&virtio_pci_root);
+	err = virtio_pci_init_root();
 	if (err)
 		return err;
 
 	err = pci_register_driver(&virtio_pci_driver);
 	if (err)
-		device_unregister(&virtio_pci_root);
+		device_unregister(virtio_pci_root);
 
 	return err;
 }

@@ -452,8 +484,8 @@ module_init(virtio_pci_init);
 
 static void __exit virtio_pci_exit(void)
 {
-	device_unregister(&virtio_pci_root);
 	pci_unregister_driver(&virtio_pci_driver);
+	device_unregister(virtio_pci_root);
 }
 
 module_exit(virtio_pci_exit);

Quoted 5 lines · Mark McLoughlin

Apparently we shouldn't be statically allocating the root device object, so dynamically allocate it instead.

Also add a release() function to avoid this warning:

Quoted 2 lines

Device 'virtio-pci' does not have a release() function, it is broken and must be fixed

Is it really better to dynamically allocate the root device than statically allocate it? Is the advice that we shouldn't statically allocate a device really a suggestion that if you're doing that, you're using struct device wrong?

If so, this conversion should be equally wrong. Have you chosen to keep this device so that your initrd work-around continues to work as-is?

What exactly is the work around and is there a less hackish way we could support it?

Quoted 3 lines · Mark McLoughlin

Quoted 1 line

Fix the virtio bus instead.

Yeah, the patch I posted wasn't meant as a fix for this traceback.

So what's the module_get patch needed for?

Quoted 1 line

Here's one that does fix it.

...

Quoted 22 lines

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] virtio: add device release() function

Add a release() function for virtio_pci devices so as to avoid:

Device 'virtio0' does not have a release() function, it is broken and must be fixed

The struct device is embedded in the struct virtio_pci_device which is freed by virtio_pci_remove(), so virtio_pci_release_dev() need not actually do anything.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio_pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/virtio/virtio_pci.c +0 -0

index c7dc37c..7d4899c 100644
@@ -70,12 +70,17 @@ static struct pci_device_id virtio_pci_id_table[] = {
 

drivers/virtio/virtio_pci.c static struct pci_device_id virtio_pci_id_table[] = { · line 75

Quoted 5 lines

drivers/virtio/virtio_pci.c +3 -0

 MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
 
+static void virtio_pci_release_dev(struct device *dev)
+{
+}

You have to have a strong reason to have empty release. This is not the case, you should do the free here, not in remove, I suppose.

drivers/virtio/virtio_pci.c static int __devinit virtio_pci_probe(struct pci_dev *pci_dev, · line 336

Quoted 5 lines

drivers/virtio/virtio_pci.c +1 -0

@@ -328,6 +333,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 		return -ENOMEM;
 
 	vp_dev->vdev.dev.parent = &virtio_pci_root;
+	vp_dev->vdev.dev.release = virtio_pci_release_dev;

This should rather be in register_virtio_device

On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:

drivers/virtio/virtio_pci.c +2 -0

+static void virtio_pci_release_dev(struct device *dev)
+{

/* Convert a generic virtio device to our structure */

drivers/virtio/virtio_pci.c +1 -0

@@ -328,6 +333,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 		return -ENOMEM;
 
 	vp_dev->vdev.dev.parent = &virtio_pci_root;
+	vp_dev->vdev.dev.release = virtio_pci_release_dev;
 	vp_dev->vdev.config = &virtio_pci_config_ops;
 	vp_dev->pci_dev = pci_dev;
 	INIT_LIST_HEAD(&vp_dev->virtqueues);

Actually, we should be able to delete this virtio_pci_root entirely. The device is a dummy one anyway.

Quoted 5 lines · Mark McLoughlin

On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:

drivers/virtio/virtio_pci.c +2 -0

+static void virtio_pci_release_dev(struct device *dev)
+{

drivers/virtio/virtio_pci.c static struct pci_device_id virtio_pci_id_table[] = { · line 84

Quoted 10 lines

drivers/virtio/virtio_pci.c +3 -0

+}
+
 /* A PCI device has it's own struct device and so does a virtio device so
  * we create a place for the virtio devices to show up in sysfs.  I think it
  * would make more sense for virtio to not insist on having it's own device. */
 static struct device virtio_pci_root = {
 	.parent		= NULL,
 	.bus_id		= "virtio-pci",
+	.release	= virtio_pci_release_dev,
 };

Actually, we should be able to delete this virtio_pci_root entirely. The device is a dummy one anyway.

Quoted 4 lines · Anthony Liguori

Quoted 2 lines · Mark McLoughlin

On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:

Quoted 16 lines

Quoted 4 lines

drivers/virtio/virtio_pci.c +2 -0

+static void virtio_pci_release_dev(struct device *dev)
+{

drivers/virtio/virtio_pci.c static struct pci_device_id virtio_pci_id_table[] = { · line 84

Quoted 10 lines

drivers/virtio/virtio_pci.c +3 -0

+}
+
 /* A PCI device has it's own struct device and so does a virtio device so
  * we create a place for the virtio devices to show up in sysfs.  I think it
  * would make more sense for virtio to not insist on having it's own device. */
 static struct device virtio_pci_root = {
 	.parent		= NULL,
 	.bus_id		= "virtio-pci",
+	.release	= virtio_pci_release_dev,
 };

Actually, we should be able to delete this virtio_pci_root entirely. The device is a dummy one anyway.

Care to recall why it was added initially and what's changed?

One side effect of removing it is that each device appears on its own in /sys/devices rather than neatly under /sys/devices/virtio-pci.

(And one side effect of that is that the aforementioned Fedora mkinitrd kludge stops working which would make me sad :-)

Cheers,
mark.

Michael Tokarev napsal(a):

Quoted 14 lines

Quoted 4 lines · Jiri Slaby

Quoted 2 lines · Mark McLoughlin

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

It unbinds the device properly as any other driver. So what's the problem here?

Here's what we get when rmmod'ing (a zero-refcounted but in use) virtio_pci (I did it by a chance, cut-n-pasted the wrong line):

WARNING: at drivers/base/core.c:122 device_release+0x5f/0x70()
Device 'virtio1' does not have a release() function, it is broken and must be fixed.
Modules linked in: ext3 jbd mbcache acpiphp dock pci_hotplug virtio_net virtio_blk virtio_pci(-) virtio_ring virtio

Quoted 6 lines

Pid: 361, comm: rmmod Tainted: G S 2.6.27-i686smp #2.6.27.7

 [<c012b81f>] warn_slowpath+0x6f/0xa0
 [<c0110030>] prepare_set+0x30/0x80
 [<c012067e>] __wake_up+0x3e/0x60
 [<c01d1d25>] release_sysfs_dirent+0x45/0xb0
 ...

So why don't you fix the root cause and add such a crap into the probe function (not even counting probe can fail later)?

Fix the virtio bus instead.

Quoted 1 line

Fix the virtio bus instead.

Incoherent? CHECK

Rude to bug reporter? CHECK

Unhelpful? CHECK

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

Fix by simply making each device take a ref on the module.

Quoted 4 lines · On Thursday 04 December 2008 23:14:31 Mark McLoughlin wrote:

Nothing takes a ref on virtio_pci, so even if you have devices in use, rmmod will attempt to unload the module.

Fix by simply making each device take a ref on the module.

Hi Mark,

Taking a reference to oneself is almost always wrong.

I'm a little surprised that a successful call to pci_device->probe doesn't bump the module count though.

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 10 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

But there really is no explicit dependency between virtio modules and virtio_pci. There just is for kvm/x86 at the moment, since that is how they use virtio. Running over another bus is certainly possible, though may never happen for x86 (happens today for s390).

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

Quoted 5 lines

But there really is no explicit dependency between virtio modules and virtio_pci. There just is for kvm/x86 at the moment, since that is how they use virtio. Running over another bus is certainly possible, though may never happen for x86 (happens today for s390).

Right, and in the case of both kvm/s390 and lguest the bus implementation is built-in, not a module.

Cheers,
Mark.

Quoted 47 lines · Mark McLoughlin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

Quoted 4 lines

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

What if we have multiple virtio transports?

Is there a way that we can
expose the relationship with virtio-blk and virtio-pci in sysfs? We have a struct device for the PCI device, it's just a matter of making the link visible.

On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote:

It feels a bit like busy work to generalise this since only virtio_pci can be built as a module, but here's a patch.

The mkinitrd hack turns into:

# Handle finding virtio bus implementations if [ -L ./virtio_module ] ; then
findmodule $(basename $(readlink ./virtio_module))
else if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi; fi

Cheers,
Mark.

[PATCH] virtio: add a 'virtio_module' sysfs symlink

Add a way for userspace to determine which virtio bus transport a given device is associated with.

This will be used by Fedora mkinitrd to generically determine e.g. that virtio_pci is needed to mount a given root filesystem.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio.c       |   21 ++++++++++++++++++++-
 drivers/virtio/virtio_pci.c   |    1 +
 include/linux/virtio_config.h |    2 ++
 3 files changed, 23 insertions(+), 1 deletions(-)

drivers/virtio/virtio.c +20 -1

index 018c070..640ede8 100644
@@ -189,13 +189,32 @@ int register_virtio_device(struct virtio_device *dev)
 	 * matching driver. */
 	err = device_register(&dev->dev);
 	if (err)
-		add_status(dev, VIRTIO_CONFIG_S_FAILED);
+		goto out;
+
+	/* Create a virtio_module symlink */
+	if (dev->config->owner) {
+		struct module_kobject *mk = &dev->config->owner->mkobj;
+
+		err = sysfs_create_link(&dev->dev.kobj, &mk->kobj,
+					"virtio_module");
+		if (err)
+			goto unreg;
+	}
+
+	return 0;
+
+unreg:
+	device_unregister(&dev->dev);
+out:
+	add_status(dev, VIRTIO_CONFIG_S_FAILED);
 	return err;
 }
 EXPORT_SYMBOL_GPL(register_virtio_device);
 
 void unregister_virtio_device(struct virtio_device *dev)
 {
+	if (dev->config->owner)
+		sysfs_remove_link(&dev->dev.kobj, "virtio_module");
 	device_unregister(&dev->dev);
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);

drivers/virtio/virtio_pci.c +1 -0

index 939e0b4..59e928d 100644
@@ -328,6 +328,7 @@ static void vp_del_vq(struct virtqueue *vq)
 }
 
 static struct virtio_config_ops virtio_pci_config_ops = {
+	.owner		= THIS_MODULE,
 	.get		= vp_get,
 	.set		= vp_set,
 	.get_status	= vp_get_status,

include/linux/virtio_config.h +2 -0

index bf8ec28..0a01cda 100644
@@ -33,6 +33,7 @@
 
 /**
  * virtio_config_ops - operations for configuring a virtio device
+ * @owner: the module implementing these ops, usually THIS_MODULE
  * @get: read the value of a configuration field
  *	vdev: the virtio_device
  *	offset: the offset of the configuration field

@@ -68,6 +69,7 @@
  */
 struct virtio_config_ops
 {
+	struct module *owner;
 	void (*get)(struct virtio_device *vdev, unsigned offset,
 		    void *buf, unsigned len);
 	void (*set)(struct virtio_device *vdev, unsigned offset,

Quoted 47 lines · Anthony Liguori

Quoted 46 lines · Mark McLoughlin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

Quoted 6 lines

Quoted 4 lines

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

What if we have multiple virtio transports?

I don't think that's so much an an issue (just build in any transport supported by KVM), but rather that you might build a non-pv_ops kernel to run on QEMU which would benefit from using virtio drivers ...

Quoted 4 lines

Is there a way that we can
expose the relationship with virtio-blk and virtio-pci in sysfs? We have a struct device for the PCI device, it's just a matter of making the link visible.

It feels a bit like busy work to generalise this since only virtio_pci can be built as a module, but here's a patch.

The mkinitrd hack turns into:

# Handle finding virtio bus implementations if [ -L ./virtio_module ] ; then
findmodule $(basename $(readlink ./virtio_module))
else if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi; fi

Cheers,
Mark.

[PATCH] virtio: add a 'virtio_module' sysfs symlink

Quoted 76 lines · Mark McLoughlin

Quoted 47 lines · Anthony Liguori

Quoted 46 lines · Mark McLoughlin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

Quoted 6 lines

Quoted 4 lines

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

What if we have multiple virtio transports?

I don't think that's so much an an issue (just build in any transport supported by KVM), but rather that you might build a non-pv_ops kernel to run on QEMU which would benefit from using virtio drivers ...

Quoted 4 lines

Is there a way that we can
expose the relationship with virtio-blk and virtio-pci in sysfs? We have a struct device for the PCI device, it's just a matter of making the link visible.

It feels a bit like busy work to generalise this since only virtio_pci can be built as a module, but here's a patch.

The mkinitrd hack turns into:

# Handle finding virtio bus implementations if [ -L ./virtio_module ] ; then
findmodule $(basename $(readlink ./virtio_module))
else if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi; fi

Quoted 4 lines

Cheers,
Mark.

[PATCH] virtio: add a 'virtio_module' sysfs symlink

Doesn't the device have a "driver" link already? If yes, the driver it points to should have a "module" link.

Quoted 77 lines · Kay Sievers

Quoted 76 lines · Mark McLoughlin

Quoted 47 lines · Anthony Liguori

Quoted 46 lines · Mark McLoughlin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

Quoted 6 lines

Quoted 4 lines

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

What if we have multiple virtio transports?

I don't think that's so much an an issue (just build in any transport supported by KVM), but rather that you might build a non-pv_ops kernel to run on QEMU which would benefit from using virtio drivers ...

Quoted 4 lines

Is there a way that we can
expose the relationship with virtio-blk and virtio-pci in sysfs? We have a struct device for the PCI device, it's just a matter of making the link visible.

It feels a bit like busy work to generalise this since only virtio_pci can be built as a module, but here's a patch.

The mkinitrd hack turns into:

# Handle finding virtio bus implementations if [ -L ./virtio_module ] ; then
findmodule $(basename $(readlink ./virtio_module))
else if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi; fi

Quoted 4 lines

Quoted 1 line

[PATCH] virtio: add a 'virtio_module' sysfs symlink

Doesn't the device have a "driver" link already? If yes, the driver it points to should have a "module" link.

The virtio bus is an abstraction that has several different backend implementations - currently virtio-pci, lguest and kvm-s390.

So yes, the driver/module link gives us the device driver, but the virtio_module link is to the virtio bus driver (aka implementation, transport, backend, ...):

$> basename $(readlink virtio_module)
virtio_pci
$> basename $(readlink driver/module)
virtio_net

Quoted 86 lines · Mark McLoughlin

Quoted 77 lines · Kay Sievers

Quoted 76 lines · Mark McLoughlin

Quoted 47 lines · Anthony Liguori

Quoted 46 lines · Mark McLoughlin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote:

Another example of a lack of an explicit dependency causing problems is Fedora's mkinitrd having this hack:

if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi

which basically says "if this is a virtio device, don't forget to include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, but this is a particularly unusual one.

Quoted 6 lines

Um, I don't know what this does, sorry.

I have no idea how Fedora chooses what to put in an initrd; I can't think of a sensible way of deciding what goes in and what doesn't other than lists and heuristics.

Fedora's mkinitrd creates an initrd suitable to boot the machine you run mkinitrd on, rather than creating an initrd suitable to boot any machine.

So, it goes "ah, / is mounted from /dev/vda, we need to include virtio_blk and it's dependencies". It does that in a generic way that works well for most setups:

1) Find the device name (e.g. vda) below /sys/block

2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1

3) Find the module need for this through either 'modalias' or the 'driver/module' symlink

4) Use modprobe to list any dependencies of that module

Clearly, virtio-pci won't be pulled in by any of this so we've added a hack to say "oh, it's a virtio device, let's include virtio_pci just in case".

It's not even the case that mkinitrd needs to know how to include the the module for the bus, because in our case that's virtio.ko ... we've pretty effectively hidden the the bus *implementation* from userspace.

Quoted 6 lines

Quoted 4 lines

I don't think this is worth wasting too much time fixing, that's why I'm thinking we should just make virtio_pci built-in by default with CONFIG_KVM_GUEST.

What if we have multiple virtio transports?

I don't think that's so much an an issue (just build in any transport supported by KVM), but rather that you might build a non-pv_ops kernel to run on QEMU which would benefit from using virtio drivers ...

Quoted 4 lines

Is there a way that we can
expose the relationship with virtio-blk and virtio-pci in sysfs? We have a struct device for the PCI device, it's just a matter of making the link visible.

It feels a bit like busy work to generalise this since only virtio_pci can be built as a module, but here's a patch.

The mkinitrd hack turns into:

# Handle finding virtio bus implementations if [ -L ./virtio_module ] ; then
findmodule $(basename $(readlink ./virtio_module))
else if echo $PWD | grep -q /virtio-pci/ ; then
findmodule virtio_pci
fi; fi

Quoted 4 lines

Quoted 1 line

[PATCH] virtio: add a 'virtio_module' sysfs symlink

Doesn't the device have a "driver" link already? If yes, the driver it points to should have a "module" link.

The virtio bus is an abstraction that has several different backend implementations - currently virtio-pci, lguest and kvm-s390.

Quoted 8 lines

So yes, the driver/module link gives us the device driver, but the virtio_module link is to the virtio bus driver (aka implementation, transport, backend, ...):

$> basename $(readlink virtio_module)
virtio_pci
$> basename $(readlink driver/module)
virtio_net

I see. But why not just call it "module", like we do in all other places, when it points to /sys/module/.

To find dependent modules, you would walk up the chain of parents, and include everything that is found by looking for "driver/module" and "module" links?

Wouldn't that make it completely generic, without any virtio specific hacks?

This is basically a no-op change, since it does exactly the same thing as s390_root_dev_register() when the caller isn't a module.

drivers/s390/kvm/kvm_virtio.c +0 -0

drivers/s390/kvm/kvm_virtio.c +1 -1

-	kvm_root = s390_root_dev_register("kvm_s390");
+	kvm_root = register_virtio_root_device("kvm_s390");

This is basically a no-op change, since it does exactly the same thing as s390_root_dev_register() when the caller isn't a module.

drivers/s390/kvm/kvm_virtio.c +0 -0

drivers/s390/kvm/kvm_virtio.c +1 -1

-	kvm_root = s390_root_dev_register("kvm_s390");
+	kvm_root = register_virtio_root_device("kvm_s390");

Am Mittwoch, 10. Dezember 2008 schrieb Mark McLoughlin:

Quoted 3 lines

This is basically a no-op change, since it does exactly the same thing as s390_root_dev_register() when the caller isn't a module.

Ok, I gave it a short test and it seems to work.
Some comments: I agree with your comment in patch0, that a generic device_register_root() function might be useful.

Quoted 3 lines

drivers/s390/kvm/kvm_virtio.c +0 -0

[...]

Quoted 2 lines

drivers/s390/kvm/kvm_virtio.c +1 -1

-	kvm_root = s390_root_dev_register("kvm_s390");
+	kvm_root = register_virtio_root_device("kvm_s390");

[...]

drivers/s390/kvm/kvm_virtio.c static int __init kvm_devices_init(void) · line 347

Quoted 2 lines

drivers/s390/kvm/kvm_virtio.c +1 -1

-		s390_root_dev_unregister(kvm_root);
+		unregister_virtio_root_device(kvm_root);

You can now remove the include <asm/s390_rdev.h>

Add a function to allocate a root device object to group the devices from a given virtio implementation.

Also add a 'module' sysfs symlink to allow so that userspace can generically determine which virtio implementation a device is associated with. This will be used by Fedora mkinitrd to generically determine e.g. that virtio_pci is needed to mount a given root filesystem.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h  |   10 ++++++
 2 files changed, 81 insertions(+), 0 deletions(-)

drivers/virtio/virtio.c +12 -0

index 018c070..61e6597 100644
@@ -1,6 +1,7 @@
 #include <linux/virtio.h>
 #include <linux/spinlock.h>
 #include <linux/virtio_config.h>
+#include <linux/err.h>
 
 /* Unique numbering for virtio devices. */
 static unsigned int dev_index;

@@ -200,6 +201,76 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+/* A root device for virtio devices from a given backend.  This makes them
+ * appear as /sys/devices/{name}/0,1,2 not /sys/devices/0,1,2. It also allows
+ * us to have a /sys/devices/{name}/module symlink to the backend module. */
+struct virtio_root_device
+{
+	struct device dev;
+	struct module *owner;
+};
+
+static struct virtio_root_device *to_virtio_root(struct device *dev)
+{

drivers/virtio/virtio.c +11 -0

+
+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner)
+{
+	struct virtio_root_device *root;
+	int err = -ENOMEM;
+
+	root = kzalloc(sizeof(struct virtio_root_device), GFP_KERNEL);
+	if (!root)
+		goto out;
+

drivers/virtio/virtio.c +7 -0

+
+	if (owner) {
+		struct module_kobject *mk = &owner->mkobj;
+
+		err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
+                if (err) {
+			device_unregister(&root->dev);

drivers/virtio/virtio.c +11 -0

+out:
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(__register_virtio_root_device);
+
+void unregister_virtio_root_device(struct device *root)
+{
+	device_unregister(root);
+}
+EXPORT_SYMBOL_GPL(unregister_virtio_root_device);
+
 static int virtio_init(void)
 {
 	if (bus_register(&virtio_bus) != 0)

include/linux/virtio.h +10 -0

index 06005fa..66e6c67 100644
@@ -93,6 +93,16 @@ struct virtio_device
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
 
+/* A root device is a dummy device used to group virtio devices from each
+ * implementation. */
+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner);
+static inline struct device *register_virtio_root_device(const char *name)
+{
+	return __register_virtio_root_device(name, THIS_MODULE);
+}
+void unregister_virtio_root_device(struct device *root);
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).

Signature · 1 line

1.5.4.3

Add a function to allocate a root device object to group the devices from a given virtio implementation.

Also add a 'module' sysfs symlink to allow so that userspace can generically determine which virtio implementation a device is associated with. This will be used by Fedora mkinitrd to generically determine e.g. that virtio_pci is needed to mount a given root filesystem.

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h  |   10 ++++++
 2 files changed, 81 insertions(+), 0 deletions(-)

drivers/virtio/virtio.c +12 -0

index 018c070..61e6597 100644
@@ -1,6 +1,7 @@
 #include <linux/virtio.h>
 #include <linux/spinlock.h>
 #include <linux/virtio_config.h>
+#include <linux/err.h>
 
 /* Unique numbering for virtio devices. */
 static unsigned int dev_index;

@@ -200,6 +201,76 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+/* A root device for virtio devices from a given backend.  This makes them
+ * appear as /sys/devices/{name}/0,1,2 not /sys/devices/0,1,2. It also allows
+ * us to have a /sys/devices/{name}/module symlink to the backend module. */
+struct virtio_root_device
+{
+	struct device dev;
+	struct module *owner;
+};
+
+static struct virtio_root_device *to_virtio_root(struct device *dev)
+{

drivers/virtio/virtio.c +11 -0

+
+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner)
+{
+	struct virtio_root_device *root;
+	int err = -ENOMEM;
+
+	root = kzalloc(sizeof(struct virtio_root_device), GFP_KERNEL);
+	if (!root)
+		goto out;
+

drivers/virtio/virtio.c +7 -0

+
+	if (owner) {
+		struct module_kobject *mk = &owner->mkobj;
+
+		err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
+                if (err) {
+			device_unregister(&root->dev);

drivers/virtio/virtio.c +11 -0

+out:
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(__register_virtio_root_device);
+
+void unregister_virtio_root_device(struct device *root)
+{
+	device_unregister(root);
+}
+EXPORT_SYMBOL_GPL(unregister_virtio_root_device);
+
 static int virtio_init(void)
 {
 	if (bus_register(&virtio_bus) != 0)

include/linux/virtio.h +10 -0

index 06005fa..66e6c67 100644
@@ -93,6 +93,16 @@ struct virtio_device
 int register_virtio_device(struct virtio_device *dev);
 void unregister_virtio_device(struct virtio_device *dev);
 
+/* A root device is a dummy device used to group virtio devices from each
+ * implementation. */
+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner);
+static inline struct device *register_virtio_root_device(const char *name)
+{
+	return __register_virtio_root_device(name, THIS_MODULE);
+}
+void unregister_virtio_root_device(struct device *root);
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).

Signature · 1 line

1.5.4.3

Quoted 8 lines · Mark McLoughlin

Add a function to allocate a root device object to group the devices from a given virtio implementation.

Also add a 'module' sysfs symlink to allow so that userspace can generically determine which virtio implementation a device is associated with. This will be used by Fedora mkinitrd to generically determine e.g. that virtio_pci is needed to mount a given root filesystem.

Nothing about this is really virtio-specific (just as s390_root_dev_register() is not really s390-specific), and a 'module' symlink doesn't really hurt in a generic implementation, even if it is unneeded. I'm voting to put this in some generic, always built-in code (or have the users select it) so we could also use it from s390.

Quoted 38 lines

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h  |   10 ++++++
 2 files changed, 81 insertions(+), 0 deletions(-)

drivers/virtio/virtio.c +17 -0

index 018c070..61e6597 100644
@@ -1,6 +1,7 @@
 #include <linux/virtio.h>
 #include <linux/spinlock.h>
 #include <linux/virtio_config.h>
+#include <linux/err.h>
 
 /* Unique numbering for virtio devices. */
 static unsigned int dev_index;

@@ -200,6 +201,76 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+/* A root device for virtio devices from a given backend.  This makes them
+ * appear as /sys/devices/{name}/0,1,2 not /sys/devices/0,1,2. It also allows
+ * us to have a /sys/devices/{name}/module symlink to the backend module. */
+struct virtio_root_device
+{
+	struct device dev;
+	struct module *owner;
+};
+
+static struct virtio_root_device *to_virtio_root(struct device *dev)
+{
+        return container_of(dev, struct virtio_root_device, dev);
+}
+
+static void release_virtio_root_device(struct device *dev)
+{

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 224

Quoted 5 lines

drivers/virtio/virtio.c +5 -0

+	struct virtio_root_device *root = to_virtio_root(dev);
+	if (root->owner)
+		sysfs_remove_link(&root->dev.kobj, "module");
+	kfree(root);
+}

Can this code be a module? If yes, move the release callback to a build-in as there are races with release-functions in modules.

Quoted 15 lines

drivers/virtio/virtio.c +15 -0

+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner)
+{
+	struct virtio_root_device *root;
+	int err = -ENOMEM;
+
+	root = kzalloc(sizeof(struct virtio_root_device), GFP_KERNEL);
+	if (!root)
+		goto out;
+
+	err = dev_set_name(&root->dev, name);
+	if (err)
+		goto free_root;
+
+	err = device_register(&root->dev);

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 245

Quoted 5 lines

drivers/virtio/virtio.c +5 -0

+	if (err)
+		goto free_root;
+
+	root->dev.parent  = NULL;
+	root->dev.release = release_virtio_root_device;

You must set ->release before calling device_register(), and setting the parent is unneeded.

Quoted 10 lines

drivers/virtio/virtio.c +10 -0

+	if (owner) {
+		struct module_kobject *mk = &owner->mkobj;
+
+		err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
+                if (err) {
+			device_unregister(&root->dev);
+			return ERR_PTR(err);
+		}
+
+		root->owner = owner;

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 262

Quoted 6 lines

drivers/virtio/virtio.c +6 -0

+	}
+
+	return &root->dev;
+
+free_root:
+	kfree(root);

You need to call device_put() if you called device_register().

Quoted 8 lines · Mark McLoughlin

Add a function to allocate a root device object to group the devices from a given virtio implementation.

Also add a 'module' sysfs symlink to allow so that userspace can generically determine which virtio implementation a device is associated with. This will be used by Fedora mkinitrd to generically determine e.g. that virtio_pci is needed to mount a given root filesystem.

Nothing about this is really virtio-specific (just as s390_root_dev_register() is not really s390-specific), and a 'module' symlink doesn't really hurt in a generic implementation, even if it is unneeded. I'm voting to put this in some generic, always built-in code (or have the users select it) so we could also use it from s390.

Quoted 38 lines

Signed-off-by Mark McLoughlin <[email protected]>

---
 drivers/virtio/virtio.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h  |   10 ++++++
 2 files changed, 81 insertions(+), 0 deletions(-)

drivers/virtio/virtio.c +17 -0

index 018c070..61e6597 100644
@@ -1,6 +1,7 @@
 #include <linux/virtio.h>
 #include <linux/spinlock.h>
 #include <linux/virtio_config.h>
+#include <linux/err.h>
 
 /* Unique numbering for virtio devices. */
 static unsigned int dev_index;

@@ -200,6 +201,76 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+/* A root device for virtio devices from a given backend.  This makes them
+ * appear as /sys/devices/{name}/0,1,2 not /sys/devices/0,1,2. It also allows
+ * us to have a /sys/devices/{name}/module symlink to the backend module. */
+struct virtio_root_device
+{
+	struct device dev;
+	struct module *owner;
+};
+
+static struct virtio_root_device *to_virtio_root(struct device *dev)
+{
+        return container_of(dev, struct virtio_root_device, dev);
+}
+
+static void release_virtio_root_device(struct device *dev)
+{

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 224

Quoted 5 lines

drivers/virtio/virtio.c +5 -0

+	struct virtio_root_device *root = to_virtio_root(dev);
+	if (root->owner)
+		sysfs_remove_link(&root->dev.kobj, "module");
+	kfree(root);
+}

Can this code be a module? If yes, move the release callback to a build-in as there are races with release-functions in modules.

Quoted 15 lines

drivers/virtio/virtio.c +15 -0

+struct device *__register_virtio_root_device(const char *name,
+					     struct module *owner)
+{
+	struct virtio_root_device *root;
+	int err = -ENOMEM;
+
+	root = kzalloc(sizeof(struct virtio_root_device), GFP_KERNEL);
+	if (!root)
+		goto out;
+
+	err = dev_set_name(&root->dev, name);
+	if (err)
+		goto free_root;
+
+	err = device_register(&root->dev);

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 245

Quoted 5 lines

drivers/virtio/virtio.c +5 -0

+	if (err)
+		goto free_root;
+
+	root->dev.parent  = NULL;
+	root->dev.release = release_virtio_root_device;

You must set ->release before calling device_register(), and setting the parent is unneeded.

Quoted 10 lines

drivers/virtio/virtio.c +10 -0

+	if (owner) {
+		struct module_kobject *mk = &owner->mkobj;
+
+		err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
+                if (err) {
+			device_unregister(&root->dev);
+			return ERR_PTR(err);
+		}
+
+		root->owner = owner;

drivers/virtio/virtio.c void unregister_virtio_device(struct virtio_device *dev) · line 262

Quoted 6 lines

drivers/virtio/virtio.c +6 -0

+	}
+
+	return &root->dev;
+
+free_root:
+	kfree(root);

You need to call device_put() if you called device_register().

Replace s390_root_dev_register() with root_device_register() etc.

Signed-off-by Mark McLoughlin <[email protected]>

---
 arch/s390/include/asm/s390_rdev.h |   15 -----------
 drivers/s390/Makefile             |    2 +-
 drivers/s390/block/dcssblk.c      |   11 +++----
 drivers/s390/crypto/ap_bus.c      |    7 ++---
 drivers/s390/kvm/kvm_virtio.c     |    5 +--
 drivers/s390/net/cu3088.c         |    7 ++---
 drivers/s390/net/qeth_core_main.c |    7 ++---
 drivers/s390/net/qeth_l2_main.c   |    2 -
 drivers/s390/net/qeth_l3_main.c   |    2 -
 drivers/s390/s390_rdev.c          |   51 -------------------------------------
 net/iucv/iucv.c                   |    5 +--
 11 files changed, 19 insertions(+), 95 deletions(-)
 delete mode 100644 arch/s390/include/asm/s390_rdev.h
 delete mode 100644 drivers/s390/s390_rdev.c

arch/s390/include/asm/s390_rdev.h deleted +0 -15

deleted file mode 100644
index 6fa2044..0000000
@@ -1,15 +0,0 @@
-/*
- *  include/asm-s390/ccwdev.h
- *
- *    Copyright (C) 2002,2005 IBM Deutschland Entwicklung GmbH, IBM Corporation
- *    Author(s): Cornelia Huck <[email protected]>
- *               Carsten Otte  <[email protected]>
- *
- *  Interface for s390 root device
- */
-
-#ifndef _S390_RDEV_H_
-#define _S390_RDEV_H_
-extern struct device *s390_root_dev_register(const char *);
-extern void s390_root_dev_unregister(struct device *);
-#endif /* _S390_RDEV_H_ */

drivers/s390/Makefile +1 -1

index 4f4e7cf..d0eae59 100644
@@ -4,7 +4,7 @@
 
 CFLAGS_sysinfo.o += -Iinclude/math-emu -Iarch/s390/math-emu -w
 
-obj-y += s390mach.o sysinfo.o s390_rdev.o
+obj-y += s390mach.o sysinfo.o
 obj-y += cio/ block/ char/ crypto/ net/ scsi/ kvm/
 
 drivers-y += drivers/s390/built-in.o

drivers/s390/block/dcssblk.c +5 -6

index 63f26a1..20fca50 100644
@@ -15,7 +15,6 @@
 #include <asm/io.h>
 #include <linux/completion.h>
 #include <linux/interrupt.h>
-#include <asm/s390_rdev.h>
 
 //#define DCSSBLK_DEBUG		/* Debug messages on/off */
 #define DCSSBLK_NAME "dcssblk"

@@ -951,7 +950,7 @@ dcssblk_check_params(void)
 static void __exit
 dcssblk_exit(void)
 {
-	s390_root_dev_unregister(dcssblk_root_dev);
+	root_device_unregister(dcssblk_root_dev);
 	unregister_blkdev(dcssblk_major, DCSSBLK_NAME);
 }
 

@@ -960,22 +959,22 @@ dcssblk_init(void)
 {
 	int rc;
 
-	dcssblk_root_dev = s390_root_dev_register("dcssblk");
+	dcssblk_root_dev = root_device_register("dcssblk");
 	if (IS_ERR(dcssblk_root_dev))
 		return PTR_ERR(dcssblk_root_dev);
 	rc = device_create_file(dcssblk_root_dev, &dev_attr_add);
 	if (rc) {
-		s390_root_dev_unregister(dcssblk_root_dev);
+		root_device_unregister(dcssblk_root_dev);
 		return rc;
 	}
 	rc = device_create_file(dcssblk_root_dev, &dev_attr_remove);
 	if (rc) {
-		s390_root_dev_unregister(dcssblk_root_dev);
+		root_device_unregister(dcssblk_root_dev);
 		return rc;
 	}
 	rc = register_blkdev(0, DCSSBLK_NAME);
 	if (rc < 0) {
-		s390_root_dev_unregister(dcssblk_root_dev);
+		root_device_unregister(dcssblk_root_dev);
 		return rc;
 	}
 	dcssblk_major = rc;

drivers/s390/crypto/ap_bus.c +3 -4

index e3fe683..0689c22 100644
@@ -32,7 +32,6 @@
 #include <linux/notifier.h>
 #include <linux/kthread.h>
 #include <linux/mutex.h>
-#include <asm/s390_rdev.h>
 #include <asm/reset.h>
 #include <linux/hrtimer.h>
 #include <linux/ktime.h>

@@ -1358,7 +1357,7 @@ int __init ap_module_init(void)
 	}
 
 	/* Create /sys/devices/ap. */
-	ap_root_device = s390_root_dev_register("ap");
+	ap_root_device = root_device_register("ap");
 	rc = IS_ERR(ap_root_device) ? PTR_ERR(ap_root_device) : 0;
 	if (rc)
 		goto out_bus;

@@ -1401,7 +1400,7 @@ out_work:
 	hrtimer_cancel(&ap_poll_timer);
 	destroy_workqueue(ap_work_queue);
 out_root:
-	s390_root_dev_unregister(ap_root_device);
+	root_device_unregister(ap_root_device);
 out_bus:
 	while (i--)
 		bus_remove_file(&ap_bus_type, ap_bus_attrs[i]);

@@ -1432,7 +1431,7 @@ void ap_module_exit(void)
 	hrtimer_cancel(&ap_poll_timer);
 	destroy_workqueue(ap_work_queue);
 	tasklet_kill(&ap_tasklet);
-	s390_root_dev_unregister(ap_root_device);
+	root_device_unregister(ap_root_device);
 	while ((dev = bus_find_device(&ap_bus_type, NULL, NULL,
 		    __ap_match_all)))
 	{

drivers/s390/kvm/kvm_virtio.c +2 -3

index c79cf05..db61252 100644
@@ -24,7 +24,6 @@
 #include <asm/kvm_virtio.h>
 #include <asm/setup.h>
 #include <asm/s390_ext.h>
-#include <asm/s390_rdev.h>
 
 #define VIRTIO_SUBCODE_64 0x0D00
 

@@ -335,7 +334,7 @@ static int __init kvm_devices_init(void)
 	if (!MACHINE_IS_KVM)
 		return -ENODEV;
 
-	kvm_root = s390_root_dev_register("kvm_s390");
+	kvm_root = root_device_register("kvm_s390");
 	if (IS_ERR(kvm_root)) {
 		rc = PTR_ERR(kvm_root);
 		printk(KERN_ERR "Could not register kvm_s390 root device");

@@ -344,7 +343,7 @@ static int __init kvm_devices_init(void)
 
 	rc = vmem_add_mapping(real_memory_size, PAGE_SIZE);
 	if (rc) {
-		s390_root_dev_unregister(kvm_root);
+		root_device_unregister(kvm_root);
 		return rc;
 	}
 

drivers/s390/net/cu3088.c +3 -4

index f4a3237..4838345 100644
@@ -25,7 +25,6 @@
 #include <linux/module.h>
 #include <linux/err.h>
 
-#include <asm/s390_rdev.h>
 #include <asm/ccwdev.h>
 #include <asm/ccwgroup.h>
 

@@ -120,12 +119,12 @@ cu3088_init (void)
 {
 	int rc;
 
-	cu3088_root_dev = s390_root_dev_register("cu3088");
+	cu3088_root_dev = root_device_register("cu3088");
 	if (IS_ERR(cu3088_root_dev))
 		return PTR_ERR(cu3088_root_dev);
 	rc = ccw_driver_register(&cu3088_driver);
 	if (rc)
-		s390_root_dev_unregister(cu3088_root_dev);
+		root_device_unregister(cu3088_root_dev);
 
 	return rc;
 }

@@ -134,7 +133,7 @@ static void __exit
 cu3088_exit (void)
 {
 	ccw_driver_unregister(&cu3088_driver);
-	s390_root_dev_unregister(cu3088_root_dev);
+	root_device_unregister(cu3088_root_dev);
 }
 
 MODULE_DEVICE_TABLE(ccw,cu3088_ids);

drivers/s390/net/qeth_core_main.c +3 -4

index 52d2659..ffeb47b 100644
@@ -21,7 +21,6 @@
 
 #include <asm/ebcdic.h>
 #include <asm/io.h>
-#include <asm/s390_rdev.h>
 
 #include "qeth_core.h"
 #include "qeth_core_offl.h"

@@ -4465,7 +4464,7 @@ static int __init qeth_core_init(void)
 				&driver_attr_group);
 	if (rc)
 		goto driver_err;
-	qeth_core_root_dev = s390_root_dev_register("qeth");
+	qeth_core_root_dev = root_device_register("qeth");
 	rc = IS_ERR(qeth_core_root_dev) ? PTR_ERR(qeth_core_root_dev) : 0;
 	if (rc)
 		goto register_err;

@@ -4479,7 +4478,7 @@ static int __init qeth_core_init(void)
 
 	return 0;
 slab_err:
-	s390_root_dev_unregister(qeth_core_root_dev);
+	root_device_unregister(qeth_core_root_dev);
 register_err:
 	driver_remove_file(&qeth_core_ccwgroup_driver.driver,
 			   &driver_attr_group);

@@ -4496,7 +4495,7 @@ out_err:
 
 static void __exit qeth_core_exit(void)
 {
-	s390_root_dev_unregister(qeth_core_root_dev);
+	root_device_unregister(qeth_core_root_dev);
 	driver_remove_file(&qeth_core_ccwgroup_driver.driver,
 			   &driver_attr_group);
 	ccwgroup_driver_unregister(&qeth_core_ccwgroup_driver);

drivers/s390/net/qeth_l2_main.c +0 -2

index 1b1e803..33c8fe2 100644
@@ -17,8 +17,6 @@
 #include <linux/mii.h>
 #include <linux/ip.h>
 
-#include <asm/s390_rdev.h>
-
 #include "qeth_core.h"
 #include "qeth_core_offl.h"
 

drivers/s390/net/qeth_l3_main.c +0 -2

index ed59fed..5dd2210 100644
@@ -23,8 +23,6 @@
 #include <net/ip.h>
 #include <net/arp.h>
 
-#include <asm/s390_rdev.h>
-
 #include "qeth_l3.h"
 #include "qeth_core_offl.h"
 

drivers/s390/s390_rdev.c deleted +0 -51

deleted file mode 100644
index 64371c0..0000000
@@ -1,51 +0,0 @@
-/*
- *  drivers/s390/s390_rdev.c
- *  s390 root device
- *
- *    Copyright (C) 2002, 2005 IBM Deutschland Entwicklung GmbH,
- *			 IBM Corporation
- *    Author(s): Cornelia Huck ([email protected])
- *		  Carsten Otte  ([email protected])
- */
-
-#include <linux/slab.h>
-#include <linux/err.h>
-#include <linux/device.h>
-#include <asm/s390_rdev.h>
-
-static void
-s390_root_dev_release(struct device *dev)
-{
-	kfree(dev);
-}
-
-struct device *
-s390_root_dev_register(const char *name)
-{
-	struct device *dev;
-	int ret;
-
-	if (!strlen(name))
-		return ERR_PTR(-EINVAL);
-	dev = kzalloc(sizeof(struct device), GFP_KERNEL);
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
-	dev_set_name(dev, name);
-	dev->release = s390_root_dev_release;
-	ret = device_register(dev);
-	if (ret) {
-		kfree(dev);
-		return ERR_PTR(ret);
-	}
-	return dev;
-}
-
-void
-s390_root_dev_unregister(struct device *dev)
-{
-	if (dev)
-		device_unregister(dev);
-}
-
-EXPORT_SYMBOL(s390_root_dev_register);
-EXPORT_SYMBOL(s390_root_dev_unregister);

net/iucv/iucv.c +2 -3

index d7b54b5..6314e1b 100644
@@ -47,7 +47,6 @@
 #include <asm/ebcdic.h>
 #include <asm/io.h>
 #include <asm/s390_ext.h>
-#include <asm/s390_rdev.h>
 #include <asm/smp.h>
 
 /*

@@ -1609,7 +1608,7 @@ static int __init iucv_init(void)
 	rc = register_external_interrupt(0x4000, iucv_external_interrupt);
 	if (rc)
 		goto out;
-	iucv_root = s390_root_dev_register("iucv");
+	iucv_root = root_device_register("iucv");
 	if (IS_ERR(iucv_root)) {
 		rc = PTR_ERR(iucv_root);
 		goto out_int;

@@ -1653,7 +1652,7 @@ out_free:
 		kfree(iucv_irq_data[cpu]);
 		iucv_irq_data[cpu] = NULL;
 	}
-	s390_root_dev_unregister(iucv_root);
+	root_device_unregister(iucv_root);
 out_int:
 	unregister_external_interrupt(0x4000, iucv_external_interrupt);
 out:

Signature · 1 line

1.5.4.3

Quoted 1 line · Mark McLoughlin

Replace s390_root_dev_register() with root_device_register() etc.

Nice, one more special case generalized :) I'll give it a run.

Quoted 6 lines · Cornelia Huck

Quoted 1 line · Mark McLoughlin

Replace s390_root_dev_register() with root_device_register() etc.

Nice, one more special case generalized :) I'll give it a run.

You missed one occurrence in iucv (see below);

with that patch applied,
everything seems to work as expected, both for root devices created by built-ins and by modules.

From: Mark McLoughlin <[email protected]>
Subject: [PATCH] s390: remove s390_root_dev_*()

Replace s390_root_dev_register() with root_device_register() etc.

[Includes fix from Cornelia Huck]

Signed-off-by Mark McLoughlin <[email protected]>