[PATCH] virtio: make PCI devices take a virtio_pci module ref

[PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

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 <markmc@redhat.com>
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
---
 drivers/virtio/virtio_pci.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index c7dc37c..147a17f 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -322,6 +322,9 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 		return -ENODEV;
 	}
 
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
 	/* allocate our structure and fill it out */
 	vp_dev = kzalloc(sizeof(struct virtio_pci_device), GFP_KERNEL);
 	if (vp_dev == NULL)
@@ -393,6 +396,7 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 	kfree(vp_dev);
+	module_put(THIS_MODULE);
 }
 
 #ifdef CONFIG_PM
-- 
1.6.0.3

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jiri Slaby]

On 12/04/2008 01:44 PM, 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.

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

> Fix by simply making each device take a ref on the module.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> Reported-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  drivers/virtio/virtio_pci.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Michael Tokarev]

Jiri Slaby wrote:
> On 12/04/2008 01:44 PM, 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.
> 
> 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
 ...

>> Reported-by: Michael Tokarev <mjt@tls.msk.ru>

It was in my original report to kvm@vger.

Thanks!

/mjt

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jiri Slaby]

Michael Tokarev napsal(a):
> Jiri Slaby wrote:
>> On 12/04/2008 01:44 PM, 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.
>> 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
>  ...

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.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:
> Michael Tokarev napsal(a):
> > Jiri Slaby wrote:
> >> On 12/04/2008 01:44 PM, 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.
> >> 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
> >  ...
> 
> 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.

Yeah, the patch I posted wasn't meant as a fix for this traceback.
Here's one that does fix it.

Cheers,
Mark.

From: Mark McLoughlin <markmc@redhat.com>
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 <markmc@redhat.com>
---
 drivers/virtio/virtio_pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index c7dc37c..7d4899c 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -70,12 +70,17 @@ static struct pci_device_id virtio_pci_id_table[] = {
 
 MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
 
+static void virtio_pci_release_dev(struct device *dev)
+{
+}
+
 /* 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,
 };
 
 /* Convert a generic virtio device to our structure */
@@ -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);
-- 
1.6.0.3

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jiri Slaby]

Mark McLoughlin wrote:
>> 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?

> Here's one that does fix it.
...
> From: Mark McLoughlin <markmc@redhat.com>
> 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 <markmc@redhat.com>
> ---
>  drivers/virtio/virtio_pci.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index c7dc37c..7d4899c 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -70,12 +70,17 @@ static struct pci_device_id virtio_pci_id_table[] = {
>  
>  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.

> @@ -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

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Greg KH]

On Fri, Dec 05, 2008 at 04:25:31PM +0100, Jiri Slaby wrote:
> Mark McLoughlin wrote:
> >> 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?
> 
> > Here's one that does fix it.
> ...
> > From: Mark McLoughlin <markmc@redhat.com>
> > 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.

thanks,

greg k-h

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Fri, 2008-12-05 at 07:26 -0800, Greg KH wrote:
> On Fri, Dec 05, 2008 at 04:25:31PM +0100, Jiri Slaby wrote:
> > Mark McLoughlin wrote:
> > >> 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?
> > 
> > > Here's one that does fix it.
> > ...
> > > From: Mark McLoughlin <markmc@redhat.com>
> > > 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.

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)

Cheers,
Mark.

From: Mark McLoughlin <markmc@redhat.com>
Subject: [PATCH] virtio: add PCI 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

Move the code to free the resources associated with the device
from virtio_pci_remove() into this new function. virtio_pci_remove()
now merely unregisters the device which should cause the final
ref to be dropped and virtio_pci_release_dev() to be called.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 drivers/virtio/virtio_pci.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index c7dc37c..10d1547 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -305,6 +305,20 @@ static struct virtio_config_ops virtio_pci_config_ops = {
 	.finalize_features = vp_finalize_features,
 };
 
+static void virtio_pci_release_dev(struct device *_d)
+{
+	struct virtio_device *dev = container_of(_d, struct virtio_device, dev);
+	struct virtio_pci_device *vp_dev = to_vp_device(dev);
+	struct pci_dev *pci_dev = vp_dev->pci_dev;
+
+	free_irq(pci_dev->irq, vp_dev);
+	pci_set_drvdata(pci_dev, NULL);
+	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_regions(pci_dev);
+	pci_disable_device(pci_dev);
+	kfree(vp_dev);
+}
+
 /* the PCI probing function */
 static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 				      const struct pci_device_id *id)
@@ -328,6 +342,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);
@@ -387,12 +402,6 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
 
 	unregister_virtio_device(&vp_dev->vdev);
-	free_irq(pci_dev->irq, vp_dev);
-	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
-	pci_release_regions(pci_dev);
-	pci_disable_device(pci_dev);
-	kfree(vp_dev);
 }
 
 #ifdef CONFIG_PM
-- 
1.6.0.3

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Greg KH]

On Fri, Dec 05, 2008 at 06:30:17PM +0000, Mark McLoughlin wrote:
> On Fri, 2008-12-05 at 07:26 -0800, Greg KH wrote:
> > On Fri, Dec 05, 2008 at 04:25:31PM +0100, Jiri Slaby wrote:
> > > Mark McLoughlin wrote:
> > > >> 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?
> > > 
> > > > Here's one that does fix it.
> > > ...
> > > > From: Mark McLoughlin <markmc@redhat.com>
> > > > 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.
> 
> Okay, consider me "mocked mercilessly by the kobject maintainer" :-)

Heh, prepare for some more mocking below...

> 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)

You should NEVER declare a kobject statically.  There should be a check
in the kernel that complains about this on some arches in the -mm and
-next trees, I'm supprised you didn't hit it.

To quote from the kobject.txt documentation file:
	Because kobjects are dynamic, they must not be declared
	statically or on the stack, but instead, always allocated
	dynamically.  Future versions of the kernel will contain a
	run-time check for kobjects that are created statically and will
	warn the developer of this improper usage.

That is why you need a "real" release function.

So, care to respin this again please?

thanks,

greg k-h

[PATCH 1/2] virtio: add PCI device release() function

[Mark McLoughlin]

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

Move the code to free the resources associated with the device
from virtio_pci_remove() into this new function. virtio_pci_remove()
now merely unregisters the device which should cause the final
ref to be dropped and virtio_pci_release_dev() to be called.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 drivers/virtio/virtio_pci.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 7462a51..265fdf2 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -307,6 +307,20 @@ static struct virtio_config_ops virtio_pci_config_ops = {
 	.finalize_features = vp_finalize_features,
 };
 
+static void virtio_pci_release_dev(struct device *_d)
+{
+	struct virtio_device *dev = container_of(_d, struct virtio_device, dev);
+	struct virtio_pci_device *vp_dev = to_vp_device(dev);
+	struct pci_dev *pci_dev = vp_dev->pci_dev;
+
+	free_irq(pci_dev->irq, vp_dev);
+	pci_set_drvdata(pci_dev, NULL);
+	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_regions(pci_dev);
+	pci_disable_device(pci_dev);
+	kfree(vp_dev);
+}
+
 /* the PCI probing function */
 static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 				      const struct pci_device_id *id)
@@ -330,6 +344,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);
@@ -389,12 +404,6 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
 
 	unregister_virtio_device(&vp_dev->vdev);
-	free_irq(pci_dev->irq, vp_dev);
-	pci_set_drvdata(pci_dev, NULL);
-	pci_iounmap(pci_dev, vp_dev->ioaddr);
-	pci_release_regions(pci_dev);
-	pci_disable_device(pci_dev);
-	kfree(vp_dev);
 }
 
 #ifdef CONFIG_PM
-- 
1.5.4.3

[PATCH 2/2] virtio: do not statically allocate root device

[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:

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

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 drivers/virtio/virtio_pci.c |   48 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 265fdf2..939e0b4 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -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);
-- 
1.5.4.3

Re: [PATCH 2/2] virtio: do not statically allocate root device

[Anthony Liguori]

Mark McLoughlin wrote:
> 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
>   

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?

Regards,

Anthony Liguori

> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  drivers/virtio/virtio_pci.c |   48 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 265fdf2..939e0b4 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -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);
>   

Re: [PATCH 2/2] virtio: do not statically allocate root device

[Mark McLoughlin]

On Mon, 2008-12-08 at 08:43 -0600, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > 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
> >   
> 
> 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?

There are other statically allocated devices in tree[1]. It seems this
is actively discouraged, though.

AFAICT, the platform_bus device is there for a similar reason to ours -
i.e. to group devices together in sysfs.

> 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?  

I choose to keep it because:

  a) I'm trying to fix the warning spew from 'rmmod virtio_pci' without 
     breaking anything else

  b) The grouping under /sys/devices/{virtio-pci,lguest,kvm_s390} is
     useful IMHO and there are precedents (e.g. /sys/devices/platform)

  c) It's better to not gratuitously move stuff around where we know it 
     will break something

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

Being discussed elsewhere in the thread.

Cheers,
Mark.

[1] - git-grep 'struct device .* = {' next-20081204
next-20081204:arch/cris/arch-v32/drivers/iop_fw_load.c:static struct device iop_spu_device[2] = {
next-20081204:arch/cris/arch-v32/drivers/iop_fw_load.c:static struct device iop_mpu_device = {
next-20081204:arch/ia64/kernel/pci-dma.c:struct device fallback_dev = {
next-20081204:arch/parisc/kernel/drivers.c:static struct device root = {
next-20081204:arch/powerpc/kernel/ibmebus.c:static struct device ibmebus_bus_device = { /* fake "parent" device */
next-20081204:arch/powerpc/platforms/ps3/system-bus.c:static struct device ps3_system_bus = {
next-20081204:arch/x86/kernel/pci-dma.c:struct device x86_dma_fallback_dev = {
next-20081204:drivers/base/isa.c:static struct device isa_bus = {
next-20081204:drivers/base/platform.c:struct device platform_bus = {
next-20081204:drivers/idle/i7300_idle.c:static struct device dummy_dma_dev = {
next-20081204:drivers/ieee1394/nodemgr.c:static struct device nodemgr_dev_template_ud = {
next-20081204:drivers/ieee1394/nodemgr.c:static struct device nodemgr_dev_template_ne = {
next-20081204:drivers/ieee1394/nodemgr.c:struct device nodemgr_dev_template_host = {
next-20081204:drivers/lguest/lguest_device.c:static struct device lguest_root = {
next-20081204:drivers/misc/sgi-gru/grumain.c:static struct device gru_device = {
next-20081204:drivers/misc/sgi-xp/xp_main.c:struct device xp_dbg_subname = {
next-20081204:drivers/misc/sgi-xp/xpc_main.c:struct device xpc_part_dbg_subname = {
next-20081204:drivers/misc/sgi-xp/xpc_main.c:struct device xpc_chan_dbg_subname = {
next-20081204:drivers/misc/sgi-xp/xpnet.c:struct device xpnet_dbg_subname = {
...
next-20081204:drivers/rapidio/rio-driver.c:static struct device rio_bus = {
next-20081204:drivers/scsi/scsi_debug.c:static struct device pseudo_primary = {
next-20081204:drivers/sh/maple/maple.c:static struct device maple_bus = {
next-20081204:drivers/sh/superhyway/superhyway.c:static struct device superhyway_bus_device = {
next-20081204:drivers/virtio/virtio_pci.c:static struct device virtio_pci_root = {
next-20081204:drivers/w1/w1.c:struct device w1_master_device = {
next-20081204:drivers/w1/w1.c:struct device w1_slave_device = {
next-20081204:samples/firmware_class/firmware_sample_driver.c:static struct device ghost_device = {
next-20081204:samples/firmware_class/firmware_sample_firmware_class.c:static struct device my_device = {

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Fri, 2008-12-05 at 16:25 +0100, Jiri Slaby wrote:
> Mark McLoughlin wrote:
> >> 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?

A misguided attempt to create an artificial dependency between virtio
device drivers and the virtio bus implementation?

> > Here's one that does fix it.
> ...
> > From: Mark McLoughlin <markmc@redhat.com>
> > 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 <markmc@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index c7dc37c..7d4899c 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -70,12 +70,17 @@ static struct pci_device_id virtio_pci_id_table[] = {
> >  
> >  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.

Okay, see the other patch I just sent.

> > @@ -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

Why? Because dev.release should be set by the same place that does
device_register() or ...?

The resources being allocated here are virtio-pci specific, so if we
wanted to do something like this we'd perhaps need to add a ->release()
to struct virtio_device and just call that hook.

Cheers,
Mark.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Anthony Liguori]

Mark McLoughlin wrote:
> On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:
>   
> +static void virtio_pci_release_dev(struct device *dev)
> +{
> +}
> +
>  /* 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.

Regards,

Anthony Liguori



>  /* Convert a generic virtio device to our structure */
> @@ -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);
>   

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jiri Slaby]

Anthony Liguori napsal(a):
> Actually, we should be able to delete this virtio_pci_root entirely. 
> The device is a dummy one anyway.

But the bus is still to be fixed...

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Fri, 2008-12-05 at 09:43 -0600, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:
> >   
> > +static void virtio_pci_release_dev(struct device *dev)
> > +{
> > +}
> > +
> >  /* 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.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Anthony Liguori]

Mark McLoughlin wrote:
> On Fri, 2008-12-05 at 09:43 -0600, Anthony Liguori wrote:
>   
>> Mark McLoughlin wrote:
>>     
>>> On Fri, 2008-12-05 at 14:17 +0100, Jiri Slaby wrote:
>>>   
>>> +static void virtio_pci_release_dev(struct device *dev)
>>> +{
>>> +}
>>> +
>>>  /* 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.
>   

Basically, to get the neater sysfs hierarchy.  But it seems that this 
requires Evil Things so I'm inclined to say it's not worth it.

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

Yeah, that would be unfortunate.  Can the kludge be done differently?

Regards,

Anthony Liguori

> Cheers,
> mark.
>
>   

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Rusty Russell]

On Friday 05 December 2008 23:47:06 Jiri Slaby wrote:
> Michael Tokarev napsal(a):
> > Jiri Slaby wrote:
> >> On 12/04/2008 01:44 PM, 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.
> >> 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
> >  ...
> 
> 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.

Incoherent? CHECK
Rude to bug reporter? CHECK
Unhelpful? CHECK

Wow, you must be a *great* kernel maintainer!

Thanks for making us all so very, very proud.
Rusty.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jiri Slaby]

Rusty Russell wrote:
>> Fix the virtio bus instead.
> 
> Incoherent? CHECK

Sorry, I don't undesratnd here. Incoherent with what?

> Rude to bug reporter? CHECK

Maybe it's my english. I apologize all, who understood my replies that way,
sorry for that, it was not intentional.

> Unhelpful? CHECK

I don't know, I expect people to ask for concrete implementation details, if
they don't know. Ok, pointing to the relevant documentation next time would
be better, I agree.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Rusty Russell]

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.

Jesse?

Thanks,
Rusty.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

Hi Rusty,

On Fri, 2008-12-05 at 10:43 +1030, Rusty Russell wrote:
> 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.

Yeah, it certainly seems fairly unorthodox, alright. But then again,
virtio_pci is an odd creature anyway :-)

My thinking was that the virtio abstraction is preventing there being an
explicit dependency between e.g. virtio_net and virtio_pci. If we didn't
have the abstraction, virtio_net would be calling directly into
virtio_pci and we'd have an explicit dep. So, I was just trying to
artificially mimic that.

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.

Í'm thinking that maybe we should default to having virtio_pci built-in
if e.g. CONFIG_KVM_GUEST is set.

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

Nah, removing a module for device should actually work fine.

Anyway, with the root cause of Michael's traceback fixed, rmmod-ing
virtio_pci and re-loading it works just fine, so ...

Cheers,
Mark.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Rusty Russell]

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).

Cheers,
Rusty.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
> 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.

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. 

> 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.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Anthony Liguori]

Mark McLoughlin wrote:
> On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
>   
>> 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.
>>     
>
> 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. 
>   

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.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

>> 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.
>
>   

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
> >   
> >> 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.
> >>     
> >
> > 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. 
> >   
> 
> 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 ...

>   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

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 <markmc@redhat.com>
---
 drivers/virtio/virtio.c       |   21 ++++++++++++++++++++-
 drivers/virtio/virtio_pci.c   |    1 +
 include/linux/virtio_config.h |    2 ++
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 018c070..640ede8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -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);
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 939e0b4..59e928d 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -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,
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bf8ec28..0a01cda 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -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,
-- 
1.6.0.3

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Anthony Liguori]

Mark McLoughlin wrote:
> 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 <markmc@redhat.com>
>   

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

> ---
>  drivers/virtio/virtio.c       |   21 ++++++++++++++++++++-
>  drivers/virtio/virtio_pci.c   |    1 +
>  include/linux/virtio_config.h |    2 ++
>  3 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 018c070..640ede8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -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);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 939e0b4..59e928d 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -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,
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index bf8ec28..0a01cda 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -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,
>   

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Kay Sievers]

On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <markmc@redhat.com> wrote:
> On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote:
>> Mark McLoughlin wrote:
>> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
>> >> 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.
>> >>
>> >
>> > 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.
>> >
>>
>> 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 ...
>
>>   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

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

Thanks,
Kay

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Mark McLoughlin]

On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <markmc@redhat.com> wrote:
> > On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote:
> >> Mark McLoughlin wrote:
> >> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
> >> >> 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.
> >> >>
> >> >
> >> > 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.
> >> >
> >>
> >> 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 ...
> >
> >>   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
> 
> 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

Cheers,
Mark.

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Kay Sievers]

On Wed, Dec 10, 2008 at 10:49, Mark McLoughlin <markmc@redhat.com> wrote:
> On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
>> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <markmc@redhat.com> wrote:
>> > On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote:
>> >> Mark McLoughlin wrote:
>> >> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote:
>> >> >> 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.
>> >> >>
>> >> >
>> >> > 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.
>> >> >
>> >>
>> >> 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 ...
>> >
>> >>   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
>> >
>> > [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

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?

Kay

Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref

[Jesse Barnes]

On Thursday, December 04, 2008 4:13 pm Rusty Russell wrote:
> 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.
>
> Jesse?

Looks like the PCI bus type's probe function does take a device reference, 
which should get called from driver_register when the driver calls 
pci_register_driver...  Is that not happening in the virtio case?

-- 
Jesse Barnes, Intel Open Source Technology Center