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(-)
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
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 <[email protected]>
> Reported-by: Michael Tokarev <[email protected]>
> ---
> drivers/virtio/virtio_pci.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
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.
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 <[email protected]>
It was in my original report to kvm@vger.
Thanks!
/mjt
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.
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 <[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(-)
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
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.
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 <[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(-)
>
> 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
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 <[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.
thanks,
greg k-h
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);
>
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...
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 <[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.
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 <[email protected]>
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 <[email protected]>
Reported-by: Michael Tokarev <[email protected]>
Cc: Anthony Liguori <[email protected]>
---
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
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 <[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(-)
> >
> > 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.
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.
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 <[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.
>
> 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
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.
>
>
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.
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.
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.
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(-)
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
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 <[email protected]>
Reported-by: Michael Tokarev <[email protected]>
Cc: Anthony Liguori <[email protected]>
---
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
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.
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 <[email protected]>
> Cc: Anthony Liguori <[email protected]>
> ---
> 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);
>
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.
>
>
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 = {
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 <[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(-)
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
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 <[email protected]>
>
Acked-by: Anthony Liguori <[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(-)
>
> 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,
>
On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <[email protected]> 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
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
On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <[email protected]> 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.
On Wed, Dec 10, 2008 at 10:49, Mark McLoughlin <[email protected]> wrote:
> On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
>> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <[email protected]> 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
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(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 018c070..61e6597 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -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)
+{
+ struct virtio_root_device *root = to_virtio_root(dev);
+ if (root->owner)
+ sysfs_remove_link(&root->dev.kobj, "module");
+ kfree(root);
+}
+
+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);
+ if (err)
+ goto free_root;
+
+ root->dev.parent = NULL;
+ root->dev.release = release_virtio_root_device;
+
+ 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;
+ }
+
+ return &root->dev;
+
+free_root:
+ kfree(root);
+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)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 06005fa..66e6c67 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -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).
--
1.5.4.3
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 <[email protected]>
Reported-by: Michael Tokarev <[email protected]>
Cc: Anthony Liguori <[email protected]>
---
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
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.
Signed-off-by: Mark McLoughlin <[email protected]>
Cc: Christian Borntraeger <[email protected]>
---
drivers/s390/kvm/kvm_virtio.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index c79cf05..d3f7d94 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -335,7 +335,7 @@ static int __init kvm_devices_init(void)
if (!MACHINE_IS_KVM)
return -ENODEV;
- kvm_root = s390_root_dev_register("kvm_s390");
+ kvm_root = register_virtio_root_device("kvm_s390");
if (IS_ERR(kvm_root)) {
rc = PTR_ERR(kvm_root);
printk(KERN_ERR "Could not register kvm_s390 root device");
@@ -344,7 +344,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);
+ unregister_virtio_root_device(kvm_root);
return rc;
}
--
1.5.4.3
We shouldn't be statically allocating the root device object,
so dynamically allocate it using register_virtio_root_device()
instead.
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/lguest/lguest_device.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index b02f6bc..109b052 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -321,10 +321,7 @@ static struct virtio_config_ops lguest_config_ops = {
/* The root device for the lguest virtio devices. This makes them appear as
* /sys/devices/lguest/0,1,2 not /sys/devices/0,1,2. */
-static struct device lguest_root = {
- .parent = NULL,
- .bus_id = "lguest",
-};
+static struct device *lguest_root;
/*D:120 This is the core of the lguest bus: actually adding a new device.
* It's a separate function because it's neater that way, and because an
@@ -351,7 +348,7 @@ static void add_lguest_device(struct lguest_device_desc *d,
}
/* This devices' parent is the lguest/ dir. */
- ldev->vdev.dev.parent = &lguest_root;
+ ldev->vdev.dev.parent = lguest_root;
/* We have a unique device index thanks to the dev_index counter. */
ldev->vdev.id.device = d->type;
/* We have a simple set of routines for querying the device's
@@ -407,7 +404,8 @@ static int __init lguest_devices_init(void)
if (strcmp(pv_info.name, "lguest") != 0)
return 0;
- if (device_register(&lguest_root) != 0)
+ lguest_root = register_virtio_root_device("lguest");
+ if (IS_ERR(lguest_root))
panic("Could not register lguest root");
/* Devices are in a single page above top of "normal" mem */
--
1.5.4.3
bus_id is gradually being removed, so use dev_name() instead.
Signed-off-by: Mark McLoughlin <[email protected]>
Cc: Kay Sievers <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/lguest/lguest_device.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 109b052..81243f1 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -272,7 +272,7 @@ static struct virtqueue *lg_find_vq(struct virtio_device *vdev,
* the interrupt as a source of randomness: it'd be nice to have that
* back.. */
err = request_irq(lvq->config.irq, vring_interrupt, IRQF_SHARED,
- vdev->dev.bus_id, vq);
+ dev_name(&vdev->dev), vq);
if (err)
goto destroy_vring;
--
1.5.4.3
We shouldn't be statically allocating the root device object,
so dynamically allocate it using register_virtio_root_device()
instead.
Also avoids this warning from 'rmmod virtio_pci':
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 | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 265fdf2..9f40fbe 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -73,10 +73,7 @@ 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;
/* Convert a generic virtio device to our structure */
static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
@@ -343,7 +340,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 +434,13 @@ static int __init virtio_pci_init(void)
{
int err;
- err = device_register(&virtio_pci_root);
- if (err)
- return err;
+ virtio_pci_root = register_virtio_root_device("virtio-pci");
+ if (IS_ERR(virtio_pci_root))
+ return PTR_ERR(virtio_pci_root);
err = pci_register_driver(&virtio_pci_driver);
if (err)
- device_unregister(&virtio_pci_root);
+ device_unregister(virtio_pci_root);
return err;
}
@@ -452,8 +449,8 @@ module_init(virtio_pci_init);
static void __exit virtio_pci_exit(void)
{
- device_unregister(&virtio_pci_root);
pci_unregister_driver(&virtio_pci_driver);
+ unregister_virtio_root_device(virtio_pci_root);
}
module_exit(virtio_pci_exit);
--
1.5.4.3
(Moved from kvm@vger to virtualization@linux-foundation, changed
subject, cleaned up cc list)
On Wed, 2008-12-10 at 13:02 +0100, Kay Sievers wrote:
> On Wed, Dec 10, 2008 at 10:49, Mark McLoughlin <[email protected]> wrote:
> > On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
> >> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <[email protected]> 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?
Yeah, that sounds much better - a minor detail is that it'd be better to
hang the symlink off each virtio implementation's root object rather
than off each device.
To that end, I've hacked up register_virtio_root_device() which fixes
the fact that we statically allocate root objects and gives us a sane
place to add this generic symlink.
It might make sense to add this to the core, though - e.g.
device_register_root() - and that would also allow us use the same
approach as module_add_driver() to add the module symlink for built-in
modules.
Cheers,
Mark.
On Wed, Dec 10, 2008 at 18:44, Mark McLoughlin <[email protected]> wrote:
> (Moved from kvm@vger to virtualization@linux-foundation, changed
> subject, cleaned up cc list)
> On Wed, 2008-12-10 at 13:02 +0100, Kay Sievers wrote:
>> On Wed, Dec 10, 2008 at 10:49, Mark McLoughlin <[email protected]> wrote:
>> > On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote:
>> >> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <[email protected]> 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?
>
> Yeah, that sounds much better - a minor detail is that it'd be better to
> hang the symlink off each virtio implementation's root object rather
> than off each device.
Sounds good, if the devices below can never be from different modules
at the same time.
> To that end, I've hacked up register_virtio_root_device() which fixes
> the fact that we statically allocate root objects and gives us a sane
> place to add this generic symlink.
Fine.
> It might make sense to add this to the core, though - e.g.
> device_register_root() - and that would also allow us use the same
> approach as module_add_driver() to add the module symlink for built-in
> modules.
Sure, I see no problem moving that over when things work out as expected.
Looks all fine from a short look over it. Also thanks for converting
to allocated struct device, and the bus_id conversion.
Kay
Am Mittwoch, 10. Dezember 2008 schrieb Mark McLoughlin:
> 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.
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
[...]
> - kvm_root = s390_root_dev_register("kvm_s390");
> + kvm_root = register_virtio_root_device("kvm_s390");
[...]
> - s390_root_dev_unregister(kvm_root);
> + unregister_virtio_root_device(kvm_root);
You can now remove the include <asm/s390_rdev.h>
On Thu, 11 Dec 2008 10:05:49 +0100,
Christian Borntraeger <[email protected]> wrote:
> Am Mittwoch, 10. Dezember 2008 schrieb Mark McLoughlin:
> > 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.
Indeed, if this is a simple replacement, we want a generic function.
I'll take a look at the patches.
>
> > --- a/drivers/s390/kvm/kvm_virtio.c
> > +++ b/drivers/s390/kvm/kvm_virtio.c
> [...]
> > - kvm_root = s390_root_dev_register("kvm_s390");
> > + kvm_root = register_virtio_root_device("kvm_s390");
> [...]
> > - s390_root_dev_unregister(kvm_root);
> > + unregister_virtio_root_device(kvm_root);
>
> You can now remove the include <asm/s390_rdev.h>
On Wed, 10 Dec 2008 17:45:35 +0000,
Mark McLoughlin <[email protected]> wrote:
> 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.
>
> Signed-off-by: Mark McLoughlin <[email protected]>
> ---
> drivers/virtio/virtio.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/virtio.h | 10 ++++++
> 2 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 018c070..61e6597 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -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)
> +{
> + 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.
> +
> +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);
> + 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.
> +
> + 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;
> + }
> +
> + return &root->dev;
> +
> +free_root:
> + kfree(root);
You need to call device_put() if you called device_register().
> +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)
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 06005fa..66e6c67 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -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).
> --
> 1.5.4.3
Add support for allocating root device objects which group
device objects under /sys/devices directories.
Also add a sysfs 'module' symlink which points to the owner
of the root device object. This will be used in virtio to
allow userspace to determine which virtio bus implementation
a given device is associated with.
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/base/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 11 ++++++
2 files changed, 99 insertions(+), 0 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8c2cc26..db160a2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1196,6 +1196,94 @@ EXPORT_SYMBOL_GPL(put_device);
EXPORT_SYMBOL_GPL(device_create_file);
EXPORT_SYMBOL_GPL(device_remove_file);
+struct root_device
+{
+ struct device dev;
+ struct module *owner;
+};
+
+static void root_device_release(struct device *dev)
+{
+ struct root_device *root = container_of(dev, struct root_device, dev);
+
+ if (root->owner)
+ sysfs_remove_link(&root->dev.kobj, "module");
+
+ kfree(root);
+}
+
+/**
+ * __root_device_register - allocate and register a root device
+ * @name: root device name
+ * @owner: owner module of the root device, usually THIS_MODULE
+ *
+ * This function allocates a root device and registers it
+ * using device_register(). In order to free the returned
+ * device, use root_device_unregister().
+ *
+ * Root devices are dummy devices which allow other devices
+ * to be grouped under /sys/devices. Use this function to
+ * allocate a root device and then use it as the parent of
+ * any device which should appear under /sys/devices/{name}
+ *
+ * The /sys/devices/{name} directory will also contain a
+ * 'module' symlink which points to the @owner directory
+ * in sysfs.
+ */
+struct device *__root_device_register(const char *name, struct module *owner)
+{
+ struct root_device *root;
+ int err = -ENOMEM;
+
+ root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
+ if (!root)
+ return ERR_PTR(err);
+
+ err = dev_set_name(&root->dev, name);
+ if (err) {
+ kfree(root);
+ return ERR_PTR(err);
+ }
+
+ root->dev.release = root_device_release;
+
+ err = device_register(&root->dev);
+ if (err) {
+ put_device(&root->dev);
+ return ERR_PTR(err);
+ }
+
+ 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;
+ }
+
+ return &root->dev;
+}
+EXPORT_SYMBOL_GPL(__root_device_register);
+
+/**
+ * root_device_unregister - unregister and free a root device
+ * @root: device going away.
+ *
+ * We simply release @root using device_unregister(). If @root
+ * has a reference count of one, the device will be freed
+ * after it has been unregistered. Otherwise, the structure
+ * will stick around until the final reference is dropped
+ * using put_device().
+ */
+void root_device_unregister(struct device *root)
+{
+ device_unregister(root);
+}
+EXPORT_SYMBOL_GPL(root_device_unregister);
+
static void device_create_release(struct device *dev)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1a3686d..9e02980 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char *new_name);
extern int device_move(struct device *dev, struct device *new_parent);
/*
+ * Root device objects for grouping under /sys/devices
+ */
+extern struct device *__root_device_register(const char *name,
+ struct module *owner);
+static inline struct device *root_device_register(const char *name)
+{
+ return __root_device_register(name, THIS_MODULE);
+}
+extern void root_device_unregister(struct device *root);
+
+/*
* Manual binding of a device to driver. See drivers/base/bus.c
* for information on use.
*/
--
1.5.4.3
We shouldn't be statically allocating the root device object,
so dynamically allocate it using root_device_register()
instead.
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/lguest/lguest_device.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 915da6b..b4d44e5 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -321,10 +321,7 @@ static struct virtio_config_ops lguest_config_ops = {
/* The root device for the lguest virtio devices. This makes them appear as
* /sys/devices/lguest/0,1,2 not /sys/devices/0,1,2. */
-static struct device lguest_root = {
- .parent = NULL,
- .bus_id = "lguest",
-};
+static struct device *lguest_root;
/*D:120 This is the core of the lguest bus: actually adding a new device.
* It's a separate function because it's neater that way, and because an
@@ -351,7 +348,7 @@ static void add_lguest_device(struct lguest_device_desc *d,
}
/* This devices' parent is the lguest/ dir. */
- ldev->vdev.dev.parent = &lguest_root;
+ ldev->vdev.dev.parent = lguest_root;
/* We have a unique device index thanks to the dev_index counter. */
ldev->vdev.id.device = d->type;
/* We have a simple set of routines for querying the device's
@@ -407,7 +404,8 @@ static int __init lguest_devices_init(void)
if (strcmp(pv_info.name, "lguest") != 0)
return 0;
- if (device_register(&lguest_root) != 0)
+ lguest_root = root_device_register("lguest");
+ if (IS_ERR(lguest_root))
panic("Could not register lguest root");
/* Devices are in a single page above top of "normal" mem */
--
1.5.4.3
We shouldn't be statically allocating the root device object,
so dynamically allocate it using root_device_register()
instead.
Also avoids this warning from 'rmmod virtio_pci':
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 | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 265fdf2..bef6b45 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -73,10 +73,7 @@ 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;
/* Convert a generic virtio device to our structure */
static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
@@ -343,7 +340,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 +434,13 @@ static int __init virtio_pci_init(void)
{
int err;
- err = device_register(&virtio_pci_root);
- if (err)
- return err;
+ virtio_pci_root = root_device_register("virtio-pci");
+ if (IS_ERR(virtio_pci_root))
+ return PTR_ERR(virtio_pci_root);
err = pci_register_driver(&virtio_pci_driver);
if (err)
- device_unregister(&virtio_pci_root);
+ device_unregister(virtio_pci_root);
return err;
}
@@ -452,8 +449,8 @@ module_init(virtio_pci_init);
static void __exit virtio_pci_exit(void)
{
- device_unregister(&virtio_pci_root);
pci_unregister_driver(&virtio_pci_driver);
+ root_device_unregister(virtio_pci_root);
}
module_exit(virtio_pci_exit);
--
1.5.4.3
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
diff --git a/arch/s390/include/asm/s390_rdev.h b/arch/s390/include/asm/s390_rdev.h
deleted file mode 100644
index 6fa2044..0000000
--- a/arch/s390/include/asm/s390_rdev.h
+++ /dev/null
@@ -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_ */
diff --git a/drivers/s390/Makefile b/drivers/s390/Makefile
index 4f4e7cf..d0eae59 100644
--- a/drivers/s390/Makefile
+++ b/drivers/s390/Makefile
@@ -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
diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
index 63f26a1..20fca50 100644
--- a/drivers/s390/block/dcssblk.c
+++ b/drivers/s390/block/dcssblk.c
@@ -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;
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index e3fe683..0689c22 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -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)))
{
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index c79cf05..db61252 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -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;
}
diff --git a/drivers/s390/net/cu3088.c b/drivers/s390/net/cu3088.c
index f4a3237..4838345 100644
--- a/drivers/s390/net/cu3088.c
+++ b/drivers/s390/net/cu3088.c
@@ -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);
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 52d2659..ffeb47b 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -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);
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 1b1e803..33c8fe2 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -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"
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index ed59fed..5dd2210 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -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"
diff --git a/drivers/s390/s390_rdev.c b/drivers/s390/s390_rdev.c
deleted file mode 100644
index 64371c0..0000000
--- a/drivers/s390/s390_rdev.c
+++ /dev/null
@@ -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);
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index d7b54b5..6314e1b 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -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:
--
1.5.4.3
On Thu, 2008-12-11 at 13:59 +0100, Cornelia Huck wrote:
> On Wed, 10 Dec 2008 17:45:35 +0000,
> Mark McLoughlin <[email protected]> wrote:
>
> > 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.
Okay, coming up ...
> > Signed-off-by: Mark McLoughlin <[email protected]>
> > ---
> > drivers/virtio/virtio.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/virtio.h | 10 ++++++
> > 2 files changed, 81 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 018c070..61e6597 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -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)
> > +{
> > + 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.
Not sure I fully understand the issue here, but it won't be an problem
with it if we move to driver core.
> > +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);
> > + 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.
Okay.
> > + 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;
> > + }
> > +
> > + return &root->dev;
> > +
> > +free_root:
> > + kfree(root);
>
> You need to call device_put() if you called device_register().
Oh, I missed that subtlety. So the rules are:
1) To release before calling device_register(), use kfree()
2) To release if device_register() failed, put_device()
3) To release once device_register() succeeds, device_unregister()
Cheers,
Mark.
On Thu, 11 Dec 2008 16:16:53 +0000,
Mark McLoughlin <[email protected]> wrote:
> Add support for allocating root device objects which group
> device objects under /sys/devices directories.
>
> Also add a sysfs 'module' symlink which points to the owner
> of the root device object. This will be used in virtio to
> allow userspace to determine which virtio bus implementation
> a given device is associated with.
I was just hacking up a similar patch :)
>
> Signed-off-by: Mark McLoughlin <[email protected]>
> ---
> drivers/base/core.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/device.h | 11 ++++++
> 2 files changed, 99 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8c2cc26..db160a2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1196,6 +1196,94 @@ EXPORT_SYMBOL_GPL(put_device);
> EXPORT_SYMBOL_GPL(device_create_file);
> EXPORT_SYMBOL_GPL(device_remove_file);
>
> +struct root_device
> +{
> + struct device dev;
> + struct module *owner;
> +};
> +
> +static void root_device_release(struct device *dev)
> +{
> + struct root_device *root = container_of(dev, struct root_device, dev);
> +
> + if (root->owner)
> + sysfs_remove_link(&root->dev.kobj, "module");
I'd rather remove the link before you unregister.
> +
> + kfree(root);
> +}
> +
> +/**
> + * __root_device_register - allocate and register a root device
> + * @name: root device name
> + * @owner: owner module of the root device, usually THIS_MODULE
> + *
> + * This function allocates a root device and registers it
> + * using device_register(). In order to free the returned
> + * device, use root_device_unregister().
> + *
> + * Root devices are dummy devices which allow other devices
> + * to be grouped under /sys/devices. Use this function to
> + * allocate a root device and then use it as the parent of
> + * any device which should appear under /sys/devices/{name}
> + *
> + * The /sys/devices/{name} directory will also contain a
> + * 'module' symlink which points to the @owner directory
> + * in sysfs.
* Note: You probably want to use root_device_register().
> + */
> +struct device *__root_device_register(const char *name, struct module *owner)
> +{
> + struct root_device *root;
> + int err = -ENOMEM;
> +
> + root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
> + if (!root)
> + return ERR_PTR(err);
> +
> + err = dev_set_name(&root->dev, name);
> + if (err) {
> + kfree(root);
> + return ERR_PTR(err);
> + }
> +
> + root->dev.release = root_device_release;
> +
> + err = device_register(&root->dev);
> + if (err) {
> + put_device(&root->dev);
> + return ERR_PTR(err);
> + }
> +
> + if (owner) {
> + struct module_kobject *mk = &owner->mkobj;
> +
> + err = sysfs_create_link(&root->dev.kobj, &mk->kobj, "module");
> + if (err) {;
Stray ';'
> + device_unregister(&root->dev);
> + return ERR_PTR(err);
> + }
> + root->owner = owner;
> + }
> +
> + return &root->dev;
> +}
> +EXPORT_SYMBOL_GPL(__root_device_register);
> +
> +/**
> + * root_device_unregister - unregister and free a root device
> + * @root: device going away.
> + *
> + * We simply release @root using device_unregister(). If @root
> + * has a reference count of one, the device will be freed
> + * after it has been unregistered. Otherwise, the structure
> + * will stick around until the final reference is dropped
> + * using put_device().
I don't think you'll need to explain device handling here. How about
this:
root_device_unregister - unregister a root device
@root: device going away
This function unregisters and cleans up a device that was created by
root_device_register().
> + */
> +void root_device_unregister(struct device *root)
> +{
Clean up the symlink here.
> + device_unregister(root);
> +}
> +EXPORT_SYMBOL_GPL(root_device_unregister);
> +
>
> static void device_create_release(struct device *dev)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1a3686d..9e02980 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char *new_name);
> extern int device_move(struct device *dev, struct device *new_parent);
>
> /*
> + * Root device objects for grouping under /sys/devices
> + */
> +extern struct device *__root_device_register(const char *name,
> + struct module *owner);
> +static inline struct device *root_device_register(const char *name)
> +{
> + return __root_device_register(name, THIS_MODULE);
> +}
> +extern void root_device_unregister(struct device *root);
> +
> +/*
> * Manual binding of a device to driver. See drivers/base/bus.c
> * for information on use.
> */
> --
> 1.5.4.3
>
(adding cc:s)
On Thu, 11 Dec 2008 16:16:56 +0000,
Mark McLoughlin <[email protected]> wrote:
> Replace s390_root_dev_register() with root_device_register() etc.
Nice, one more special case generalized :) I'll give it a run.
>
> 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
>
> diff --git a/arch/s390/include/asm/s390_rdev.h b/arch/s390/include/asm/s390_rdev.h
> deleted file mode 100644
> index 6fa2044..0000000
> --- a/arch/s390/include/asm/s390_rdev.h
> +++ /dev/null
> @@ -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_ */
> diff --git a/drivers/s390/Makefile b/drivers/s390/Makefile
> index 4f4e7cf..d0eae59 100644
> --- a/drivers/s390/Makefile
> +++ b/drivers/s390/Makefile
> @@ -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
> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
> index 63f26a1..20fca50 100644
> --- a/drivers/s390/block/dcssblk.c
> +++ b/drivers/s390/block/dcssblk.c
> @@ -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;
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index e3fe683..0689c22 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -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)))
> {
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> index c79cf05..db61252 100644
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -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;
> }
>
> diff --git a/drivers/s390/net/cu3088.c b/drivers/s390/net/cu3088.c
> index f4a3237..4838345 100644
> --- a/drivers/s390/net/cu3088.c
> +++ b/drivers/s390/net/cu3088.c
> @@ -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);
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index 52d2659..ffeb47b 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -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);
> diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
> index 1b1e803..33c8fe2 100644
> --- a/drivers/s390/net/qeth_l2_main.c
> +++ b/drivers/s390/net/qeth_l2_main.c
> @@ -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"
>
> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> index ed59fed..5dd2210 100644
> --- a/drivers/s390/net/qeth_l3_main.c
> +++ b/drivers/s390/net/qeth_l3_main.c
> @@ -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"
>
> diff --git a/drivers/s390/s390_rdev.c b/drivers/s390/s390_rdev.c
> deleted file mode 100644
> index 64371c0..0000000
> --- a/drivers/s390/s390_rdev.c
> +++ /dev/null
> @@ -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);
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index d7b54b5..6314e1b 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -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:
> --
> 1.5.4.3
>
--
Cornelia Huck
Linux for zSeries Developer
Tel.: +49-7031-16-4837, Mail: [email protected]
Add support for allocating root device objects which group
device objects under /sys/devices directories.
Also add a sysfs 'module' symlink which points to the owner
of the root device object. This symlink will be used in virtio
to allow userspace to determine which virtio bus implementation
a given device is associated with.
[Includes suggestions from Cornelia Huck]
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/base/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 11 ++++++
2 files changed, 98 insertions(+), 0 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8c2cc26..20e5825 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1196,6 +1196,93 @@ EXPORT_SYMBOL_GPL(put_device);
EXPORT_SYMBOL_GPL(device_create_file);
EXPORT_SYMBOL_GPL(device_remove_file);
+struct root_device
+{
+ struct device dev;
+ struct module *owner;
+};
+
+static void root_device_release(struct device *dev)
+{
+ kfree(dev);
+}
+
+/**
+ * __root_device_register - allocate and register a root device
+ * @name: root device name
+ * @owner: owner module of the root device, usually THIS_MODULE
+ *
+ * This function allocates a root device and registers it
+ * using device_register(). In order to free the returned
+ * device, use root_device_unregister().
+ *
+ * Root devices are dummy devices which allow other devices
+ * to be grouped under /sys/devices. Use this function to
+ * allocate a root device and then use it as the parent of
+ * any device which should appear under /sys/devices/{name}
+ *
+ * The /sys/devices/{name} directory will also contain a
+ * 'module' symlink which points to the @owner directory
+ * in sysfs.
+ *
+ * Note: You probably want to use root_device_register().
+ */
+struct device *__root_device_register(const char *name, struct module *owner)
+{
+ struct root_device *root;
+ int err = -ENOMEM;
+
+ root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
+ if (!root)
+ return ERR_PTR(err);
+
+ err = dev_set_name(&root->dev, name);
+ if (err) {
+ kfree(root);
+ return ERR_PTR(err);
+ }
+
+ root->dev.release = root_device_release;
+
+ err = device_register(&root->dev);
+ if (err) {
+ put_device(&root->dev);
+ return ERR_PTR(err);
+ }
+
+ 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;
+ }
+
+ return &root->dev;
+}
+EXPORT_SYMBOL_GPL(__root_device_register);
+
+/**
+ * root_device_unregister - unregister and free a root device
+ * @root: device going away.
+ *
+ * This function unregisters and cleans up a device that was created by
+ * root_device_register().
+ */
+void root_device_unregister(struct device *dev)
+{
+ struct root_device *root = container_of(dev, struct root_device, dev);
+
+ if (root->owner)
+ sysfs_remove_link(&root->dev.kobj, "module");
+
+ device_unregister(dev);
+}
+EXPORT_SYMBOL_GPL(root_device_unregister);
+
static void device_create_release(struct device *dev)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1a3686d..9e02980 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char *new_name);
extern int device_move(struct device *dev, struct device *new_parent);
/*
+ * Root device objects for grouping under /sys/devices
+ */
+extern struct device *__root_device_register(const char *name,
+ struct module *owner);
+static inline struct device *root_device_register(const char *name)
+{
+ return __root_device_register(name, THIS_MODULE);
+}
+extern void root_device_unregister(struct device *root);
+
+/*
* Manual binding of a device to driver. See drivers/base/bus.c
* for information on use.
*/
--
1.5.4.3
On Thu, 11 Dec 2008 18:23:27 +0000,
Mark McLoughlin <[email protected]> wrote:
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8c2cc26..20e5825 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1196,6 +1196,93 @@ EXPORT_SYMBOL_GPL(put_device);
> EXPORT_SYMBOL_GPL(device_create_file);
> EXPORT_SYMBOL_GPL(device_remove_file);
>
> +struct root_device
> +{
> + struct device dev;
> + struct module *owner;
> +};
> +
> +static void root_device_release(struct device *dev)
> +{
You need to get the root device here and free that.
> + kfree(dev);
> +}
On Fri, 2008-12-12 at 09:42 +0100, Cornelia Huck wrote:
> On Thu, 11 Dec 2008 18:23:27 +0000,
> Mark McLoughlin <[email protected]> wrote:
>
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 8c2cc26..20e5825 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1196,6 +1196,93 @@ EXPORT_SYMBOL_GPL(put_device);
> > EXPORT_SYMBOL_GPL(device_create_file);
> > EXPORT_SYMBOL_GPL(device_remove_file);
> >
> > +struct root_device
> > +{
> > + struct device dev;
> > + struct module *owner;
> > +};
> > +
> > +static void root_device_release(struct device *dev)
> > +{
>
> You need to get the root device here and free that.
>
> > + kfree(dev);
> > +}
Yeah, I just figured it was a little overkill given the structure
definition is three lines away. Here it is, though.
Cheers,
Mark.
From: Mark McLoughlin <[email protected]>
Subject: [PATCH] driver core: add root_device_register()
Add support for allocating root device objects which group
device objects under /sys/devices directories.
Also add a sysfs 'module' symlink which points to the owner
of the root device object. This symlink will be used in virtio
to allow userspace to determine which virtio bus implementation
a given device is associated with.
[Includes suggestions from Cornelia Huck]
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/base/core.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 11 ++++++
2 files changed, 100 insertions(+), 0 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8c2cc26..05320af 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1196,6 +1196,95 @@ EXPORT_SYMBOL_GPL(put_device);
EXPORT_SYMBOL_GPL(device_create_file);
EXPORT_SYMBOL_GPL(device_remove_file);
+struct root_device
+{
+ struct device dev;
+ struct module *owner;
+};
+
+#define to_root_device(dev) container_of(dev, struct root_device, dev)
+
+static void root_device_release(struct device *dev)
+{
+ kfree(to_root_device(dev));
+}
+
+/**
+ * __root_device_register - allocate and register a root device
+ * @name: root device name
+ * @owner: owner module of the root device, usually THIS_MODULE
+ *
+ * This function allocates a root device and registers it
+ * using device_register(). In order to free the returned
+ * device, use root_device_unregister().
+ *
+ * Root devices are dummy devices which allow other devices
+ * to be grouped under /sys/devices. Use this function to
+ * allocate a root device and then use it as the parent of
+ * any device which should appear under /sys/devices/{name}
+ *
+ * The /sys/devices/{name} directory will also contain a
+ * 'module' symlink which points to the @owner directory
+ * in sysfs.
+ *
+ * Note: You probably want to use root_device_register().
+ */
+struct device *__root_device_register(const char *name, struct module *owner)
+{
+ struct root_device *root;
+ int err = -ENOMEM;
+
+ root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
+ if (!root)
+ return ERR_PTR(err);
+
+ err = dev_set_name(&root->dev, name);
+ if (err) {
+ kfree(root);
+ return ERR_PTR(err);
+ }
+
+ root->dev.release = root_device_release;
+
+ err = device_register(&root->dev);
+ if (err) {
+ put_device(&root->dev);
+ return ERR_PTR(err);
+ }
+
+ 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;
+ }
+
+ return &root->dev;
+}
+EXPORT_SYMBOL_GPL(__root_device_register);
+
+/**
+ * root_device_unregister - unregister and free a root device
+ * @root: device going away.
+ *
+ * This function unregisters and cleans up a device that was created by
+ * root_device_register().
+ */
+void root_device_unregister(struct device *dev)
+{
+ struct root_device *root = to_root_device(dev);
+
+ if (root->owner)
+ sysfs_remove_link(&root->dev.kobj, "module");
+
+ device_unregister(dev);
+}
+EXPORT_SYMBOL_GPL(root_device_unregister);
+
static void device_create_release(struct device *dev)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 1a3686d..9e02980 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char *new_name);
extern int device_move(struct device *dev, struct device *new_parent);
/*
+ * Root device objects for grouping under /sys/devices
+ */
+extern struct device *__root_device_register(const char *name,
+ struct module *owner);
+static inline struct device *root_device_register(const char *name)
+{
+ return __root_device_register(name, THIS_MODULE);
+}
+extern void root_device_unregister(struct device *root);
+
+/*
* Manual binding of a device to driver. See drivers/base/bus.c
* for information on use.
*/
--
1.6.0.3
On Fri, 12 Dec 2008 08:56:49 +0000,
Mark McLoughlin <[email protected]> wrote:
> Yeah, I just figured it was a little overkill given the structure
> definition is three lines away. Here it is, though.
As you use it twice, I think it makes the code more readable.
>
> Cheers,
> Mark.
>
> From: Mark McLoughlin <[email protected]>
> Subject: [PATCH] driver core: add root_device_register()
>
> Add support for allocating root device objects which group
> device objects under /sys/devices directories.
>
> Also add a sysfs 'module' symlink which points to the owner
> of the root device object. This symlink will be used in virtio
> to allow userspace to determine which virtio bus implementation
> a given device is associated with.
>
> [Includes suggestions from Cornelia Huck]
>
> Signed-off-by: Mark McLoughlin <[email protected]>
Acked-by: Cornelia Huck <[email protected]>
> ---
> drivers/base/core.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/device.h | 11 ++++++
> 2 files changed, 100 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8c2cc26..05320af 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1196,6 +1196,95 @@ EXPORT_SYMBOL_GPL(put_device);
> EXPORT_SYMBOL_GPL(device_create_file);
> EXPORT_SYMBOL_GPL(device_remove_file);
>
> +struct root_device
> +{
> + struct device dev;
> + struct module *owner;
> +};
> +
> +#define to_root_device(dev) container_of(dev, struct root_device, dev)
> +
> +static void root_device_release(struct device *dev)
> +{
> + kfree(to_root_device(dev));
> +}
> +
> +/**
> + * __root_device_register - allocate and register a root device
> + * @name: root device name
> + * @owner: owner module of the root device, usually THIS_MODULE
> + *
> + * This function allocates a root device and registers it
> + * using device_register(). In order to free the returned
> + * device, use root_device_unregister().
> + *
> + * Root devices are dummy devices which allow other devices
> + * to be grouped under /sys/devices. Use this function to
> + * allocate a root device and then use it as the parent of
> + * any device which should appear under /sys/devices/{name}
> + *
> + * The /sys/devices/{name} directory will also contain a
> + * 'module' symlink which points to the @owner directory
> + * in sysfs.
> + *
> + * Note: You probably want to use root_device_register().
> + */
> +struct device *__root_device_register(const char *name, struct module *owner)
> +{
> + struct root_device *root;
> + int err = -ENOMEM;
> +
> + root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
> + if (!root)
> + return ERR_PTR(err);
> +
> + err = dev_set_name(&root->dev, name);
> + if (err) {
> + kfree(root);
> + return ERR_PTR(err);
> + }
> +
> + root->dev.release = root_device_release;
> +
> + err = device_register(&root->dev);
> + if (err) {
> + put_device(&root->dev);
> + return ERR_PTR(err);
> + }
> +
> + 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;
> + }
> +
> + return &root->dev;
> +}
> +EXPORT_SYMBOL_GPL(__root_device_register);
> +
> +/**
> + * root_device_unregister - unregister and free a root device
> + * @root: device going away.
> + *
> + * This function unregisters and cleans up a device that was created by
> + * root_device_register().
> + */
> +void root_device_unregister(struct device *dev)
> +{
> + struct root_device *root = to_root_device(dev);
> +
> + if (root->owner)
> + sysfs_remove_link(&root->dev.kobj, "module");
> +
> + device_unregister(dev);
> +}
> +EXPORT_SYMBOL_GPL(root_device_unregister);
> +
>
> static void device_create_release(struct device *dev)
> {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1a3686d..9e02980 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -483,6 +483,17 @@ extern int device_rename(struct device *dev, char *new_name);
> extern int device_move(struct device *dev, struct device *new_parent);
>
> /*
> + * Root device objects for grouping under /sys/devices
> + */
> +extern struct device *__root_device_register(const char *name,
> + struct module *owner);
> +static inline struct device *root_device_register(const char *name)
> +{
> + return __root_device_register(name, THIS_MODULE);
> +}
> +extern void root_device_unregister(struct device *root);
> +
> +/*
> * Manual binding of a device to driver. See drivers/base/bus.c
> * for information on use.
> */
> --
> 1.6.0.3
>
On Thu, 11 Dec 2008 18:00:25 +0100,
Cornelia Huck <[email protected]> wrote:
> On Thu, 11 Dec 2008 16:16:56 +0000,
> Mark McLoughlin <[email protected]> wrote:
>
> > 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.
---
net/iucv/iucv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6.orig/net/iucv/iucv.c
+++ linux-2.6/net/iucv/iucv.c
@@ -1682,7 +1682,7 @@ static void __exit iucv_exit(void)
kfree(iucv_irq_data[cpu]);
iucv_irq_data[cpu] = NULL;
}
- s390_root_dev_unregister(iucv_root);
+ root_device_unregister(iucv_root);
bus_unregister(&iucv_bus);
unregister_external_interrupt(0x4000, iucv_external_interrupt);
}
On Fri, 2008-12-12 at 10:29 +0100, Cornelia Huck wrote:
> On Thu, 11 Dec 2008 18:00:25 +0100,
> Cornelia Huck <[email protected]> wrote:
> > On Thu, 11 Dec 2008 16:16:56 +0000,
> > Mark McLoughlin <[email protected]> wrote:
> >
> > > 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);
Oops.
> with that patch applied,
> everything seems to work as expected, both for root devices created by
> built-ins and by modules.
Thanks,
Mark.
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]>
---
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 | 7 ++---
11 files changed, 20 insertions(+), 96 deletions(-)
delete mode 100644 arch/s390/include/asm/s390_rdev.h
delete mode 100644 drivers/s390/s390_rdev.c
diff --git a/arch/s390/include/asm/s390_rdev.h b/arch/s390/include/asm/s390_rdev.h
deleted file mode 100644
index 6fa2044..0000000
--- a/arch/s390/include/asm/s390_rdev.h
+++ /dev/null
@@ -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_ */
diff --git a/drivers/s390/Makefile b/drivers/s390/Makefile
index 4f4e7cf..d0eae59 100644
--- a/drivers/s390/Makefile
+++ b/drivers/s390/Makefile
@@ -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
diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
index 63f26a1..20fca50 100644
--- a/drivers/s390/block/dcssblk.c
+++ b/drivers/s390/block/dcssblk.c
@@ -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;
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index e3fe683..0689c22 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -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)))
{
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index c79cf05..db61252 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -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;
}
diff --git a/drivers/s390/net/cu3088.c b/drivers/s390/net/cu3088.c
index f4a3237..4838345 100644
--- a/drivers/s390/net/cu3088.c
+++ b/drivers/s390/net/cu3088.c
@@ -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);
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index 52d2659..ffeb47b 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -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);
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 1b1e803..33c8fe2 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -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"
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index ed59fed..5dd2210 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -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"
diff --git a/drivers/s390/s390_rdev.c b/drivers/s390/s390_rdev.c
deleted file mode 100644
index 64371c0..0000000
--- a/drivers/s390/s390_rdev.c
+++ /dev/null
@@ -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);
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index d7b54b5..22bb67e 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -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:
@@ -1683,7 +1682,7 @@ static void __exit iucv_exit(void)
kfree(iucv_irq_data[cpu]);
iucv_irq_data[cpu] = NULL;
}
- s390_root_dev_unregister(iucv_root);
+ root_device_unregister(iucv_root);
bus_unregister(&iucv_bus);
unregister_external_interrupt(0x4000, iucv_external_interrupt);
}
--
1.6.0.3
On Fri, 2008-12-12 at 09:35 +0000, Mark McLoughlin wrote:
> 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]>
I can carry the patch in my patch queue. Thanks.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Fri, 12 Dec 2008 09:35:46 +0000,
Mark McLoughlin <[email protected]> wrote:
> 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]>
Acked-by: Cornelia Huck <[email protected]>
On Fri, 2008-12-12 at 10:45 +0100, Martin Schwidefsky wrote:
> On Fri, 2008-12-12 at 09:35 +0000, Mark McLoughlin wrote:
> > 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]>
>
> I can carry the patch in my patch queue. Thanks.
Oops, this depends on another patch. If the other patch is missing it
doesn't compile.. So I better not add that patch to my queue. It should
be sent together with the patch it depends on.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Fri, Dec 12, 2008 at 09:35:46AM +0000, Mark McLoughlin wrote:
> On Fri, 2008-12-12 at 10:29 +0100, Cornelia Huck wrote:
> > On Thu, 11 Dec 2008 18:00:25 +0100,
> > Cornelia Huck <[email protected]> wrote:
> > > On Thu, 11 Dec 2008 16:16:56 +0000,
> > > Mark McLoughlin <[email protected]> wrote:
> > >
> > > > 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);
>
> Oops.
>
> > with that patch applied,
> > everything seems to work as expected, both for root devices created by
> > built-ins and by modules.
>
> Thanks,
> Mark.
>
> From: Mark McLoughlin <[email protected]>
> Subject: [PATCH] s390: remove s390_root_dev_*()
>
> Replace s390_root_dev_register() with root_device_register() etc.
Can you resend this whole series, I see lots of replacements in this
thread and it would be good to know exactly what I should be
reviewing/applying.
thanks,
greg k-h
We shouldn't be statically allocating the root device object,
so dynamically allocate it using root_device_register()
instead.
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/lguest/lguest_device.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index 915da6b..b4d44e5 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -321,10 +321,7 @@ static struct virtio_config_ops lguest_config_ops = {
/* The root device for the lguest virtio devices. This makes them appear as
* /sys/devices/lguest/0,1,2 not /sys/devices/0,1,2. */
-static struct device lguest_root = {
- .parent = NULL,
- .bus_id = "lguest",
-};
+static struct device *lguest_root;
/*D:120 This is the core of the lguest bus: actually adding a new device.
* It's a separate function because it's neater that way, and because an
@@ -351,7 +348,7 @@ static void add_lguest_device(struct lguest_device_desc *d,
}
/* This devices' parent is the lguest/ dir. */
- ldev->vdev.dev.parent = &lguest_root;
+ ldev->vdev.dev.parent = lguest_root;
/* We have a unique device index thanks to the dev_index counter. */
ldev->vdev.id.device = d->type;
/* We have a simple set of routines for querying the device's
@@ -407,7 +404,8 @@ static int __init lguest_devices_init(void)
if (strcmp(pv_info.name, "lguest") != 0)
return 0;
- if (device_register(&lguest_root) != 0)
+ lguest_root = root_device_register("lguest");
+ if (IS_ERR(lguest_root))
panic("Could not register lguest root");
/* Devices are in a single page above top of "normal" mem */
--
1.5.4.3
We shouldn't be statically allocating the root device object,
so dynamically allocate it using root_device_register()
instead.
Also avoids this warning from 'rmmod virtio_pci':
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 | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 265fdf2..bef6b45 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -73,10 +73,7 @@ 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;
/* Convert a generic virtio device to our structure */
static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev)
@@ -343,7 +340,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 +434,13 @@ static int __init virtio_pci_init(void)
{
int err;
- err = device_register(&virtio_pci_root);
- if (err)
- return err;
+ virtio_pci_root = root_device_register("virtio-pci");
+ if (IS_ERR(virtio_pci_root))
+ return PTR_ERR(virtio_pci_root);
err = pci_register_driver(&virtio_pci_driver);
if (err)
- device_unregister(&virtio_pci_root);
+ device_unregister(virtio_pci_root);
return err;
}
@@ -452,8 +449,8 @@ module_init(virtio_pci_init);
static void __exit virtio_pci_exit(void)
{
- device_unregister(&virtio_pci_root);
pci_unregister_driver(&virtio_pci_driver);
+ root_device_unregister(virtio_pci_root);
}
module_exit(virtio_pci_exit);
--
1.5.4.3
Add support for allocating root device objects which group
device objects under /sys/devices directories.
Also add a sysfs 'module' symlink which points to the owner
of the root device object. This symlink will be used in virtio
to allow userspace to determine which virtio bus implementation
a given device is associated with.
[Includes suggestions from Cornelia Huck]
Signed-off-by: Mark McLoughlin <[email protected]>
---
drivers/base/core.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 11 ++++++
2 files changed, 100 insertions(+), 0 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14aa2b6..eb1b4cb 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1196,6 +1196,95 @@ EXPORT_SYMBOL_GPL(put_device);
EXPORT_SYMBOL_GPL(device_create_file);
EXPORT_SYMBOL_GPL(device_remove_file);
+struct root_device
+{
+ struct device dev;
+ struct module *owner;
+};
+
+#define to_root_device(dev) container_of(dev, struct root_device, dev)
+
+static void root_device_release(struct device *dev)
+{
+ kfree(to_root_device(dev));
+}
+
+/**
+ * __root_device_register - allocate and register a root device
+ * @name: root device name
+ * @owner: owner module of the root device, usually THIS_MODULE
+ *
+ * This function allocates a root device and registers it
+ * using device_register(). In order to free the returned
+ * device, use root_device_unregister().
+ *
+ * Root devices are dummy devices which allow other devices
+ * to be grouped under /sys/devices. Use this function to
+ * allocate a root device and then use it as the parent of
+ * any device which should appear under /sys/devices/{name}
+ *
+ * The /sys/devices/{name} directory will also contain a
+ * 'module' symlink which points to the @owner directory
+ * in sysfs.
+ *
+ * Note: You probably want to use root_device_register().
+ */
+struct device *__root_device_register(const char *name, struct module *owner)
+{
+ struct root_device *root;
+ int err = -ENOMEM;
+
+ root = kzalloc(sizeof(struct root_device), GFP_KERNEL);
+ if (!root)
+ return ERR_PTR(err);
+
+ err = dev_set_name(&root->dev, name);
+ if (err) {
+ kfree(root);
+ return ERR_PTR(err);
+ }
+
+ root->dev.release = root_device_release;
+
+ err = device_register(&root->dev);
+ if (err) {
+ put_device(&root->dev);
+ return ERR_PTR(err);
+ }
+
+ 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;
+ }
+
+ return &root->dev;
+}
+EXPORT_SYMBOL_GPL(__root_device_register);
+
+/**
+ * root_device_unregister - unregister and free a root device
+ * @root: device going away.
+ *
+ * This function unregisters and cleans up a device that was created by
+ * root_device_register().
+ */
+void root_device_unregister(struct device *dev)
+{
+ struct root_device *root = to_root_device(dev);
+
+ if (root->owner)
+ sysfs_remove_link(&root->dev.kobj, "module");
+
+ device_unregister(dev);
+}
+EXPORT_SYMBOL_GPL(root_device_unregister);
+
static void device_create_release(struct device *dev)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 4e14fad..9008c79 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -484,6 +484,17 @@ extern int device_rename(struct device *dev, char *new_name);
extern int device_move(struct device *dev, struct device *new_parent);
/*
+ * Root device objects for grouping under /sys/devices
+ */
+extern struct device *__root_device_register(const char *name,
+ struct module *owner);
+static inline struct device *root_device_register(const char *name)
+{
+ return __root_device_register(name, THIS_MODULE);
+}
+extern void root_device_unregister(struct device *root);
+
+/*
* Manual binding of a device to driver. See drivers/base/bus.c
* for information on use.
*/
--
1.5.4.3
Replace s390_root_dev_register() with root_device_register() etc.
[Includes fix from Cornelia Huck]
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 | 7 ++---
11 files changed, 20 insertions(+), 96 deletions(-)
delete mode 100644 arch/s390/include/asm/s390_rdev.h
delete mode 100644 drivers/s390/s390_rdev.c
diff --git a/arch/s390/include/asm/s390_rdev.h b/arch/s390/include/asm/s390_rdev.h
deleted file mode 100644
index 6fa2044..0000000
--- a/arch/s390/include/asm/s390_rdev.h
+++ /dev/null
@@ -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_ */
diff --git a/drivers/s390/Makefile b/drivers/s390/Makefile
index 4f4e7cf..d0eae59 100644
--- a/drivers/s390/Makefile
+++ b/drivers/s390/Makefile
@@ -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
diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c
index 26ffc6a..cfdcf1a 100644
--- a/drivers/s390/block/dcssblk.c
+++ b/drivers/s390/block/dcssblk.c
@@ -18,7 +18,6 @@
#include <asm/io.h>
#include <linux/completion.h>
#include <linux/interrupt.h>
-#include <asm/s390_rdev.h>
#define DCSSBLK_NAME "dcssblk"
#define DCSSBLK_MINORS_PER_DISK 1
@@ -946,7 +945,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);
}
@@ -955,22 +954,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;
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index 1f5f5d2..9c14840 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -36,7 +36,6 @@
#include <linux/notifier.h>
#include <linux/kthread.h>
#include <linux/mutex.h>
-#include <asm/s390_rdev.h>
#include <asm/reset.h>
#include <asm/airq.h>
#include <asm/atomic.h>
@@ -1522,7 +1521,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;
@@ -1565,7 +1564,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]);
@@ -1600,7 +1599,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)))
{
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index c79cf05..db61252 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -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;
}
diff --git a/drivers/s390/net/cu3088.c b/drivers/s390/net/cu3088.c
index f4a3237..4838345 100644
--- a/drivers/s390/net/cu3088.c
+++ b/drivers/s390/net/cu3088.c
@@ -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);
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index bbd6788..c8fea52 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -24,7 +24,6 @@
#include <asm/ebcdic.h>
#include <asm/io.h>
-#include <asm/s390_rdev.h>
#include "qeth_core.h"
#include "qeth_core_offl.h"
@@ -4497,7 +4496,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;
@@ -4511,7 +4510,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);
@@ -4529,7 +4528,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);
diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c
index 2c48591..54759b7 100644
--- a/drivers/s390/net/qeth_l2_main.c
+++ b/drivers/s390/net/qeth_l2_main.c
@@ -20,8 +20,6 @@
#include <linux/mii.h>
#include <linux/ip.h>
-#include <asm/s390_rdev.h>
-
#include "qeth_core.h"
#include "qeth_core_offl.h"
diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
index c0b30b2..fec383f 100644
--- a/drivers/s390/net/qeth_l3_main.c
+++ b/drivers/s390/net/qeth_l3_main.c
@@ -26,8 +26,6 @@
#include <net/ip.h>
#include <net/arp.h>
-#include <asm/s390_rdev.h>
-
#include "qeth_l3.h"
#include "qeth_core_offl.h"
diff --git a/drivers/s390/s390_rdev.c b/drivers/s390/s390_rdev.c
deleted file mode 100644
index 64371c0..0000000
--- a/drivers/s390/s390_rdev.c
+++ /dev/null
@@ -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);
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 8f57d4f..b237b52 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -50,7 +50,6 @@
#include <asm/ebcdic.h>
#include <asm/io.h>
#include <asm/s390_ext.h>
-#include <asm/s390_rdev.h>
#include <asm/smp.h>
/*
@@ -1692,7 +1691,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;
@@ -1736,7 +1735,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:
@@ -1766,7 +1765,7 @@ static void __exit iucv_exit(void)
kfree(iucv_irq_data[cpu]);
iucv_irq_data[cpu] = NULL;
}
- s390_root_dev_unregister(iucv_root);
+ root_device_unregister(iucv_root);
bus_unregister(&iucv_bus);
unregister_external_interrupt(0x4000, iucv_external_interrupt);
}
--
1.5.4.3
On Monday 15 December 2008 23:28:27 Mark McLoughlin wrote:
> We shouldn't be statically allocating the root device object,
> so dynamically allocate it using root_device_register()
> instead.
This and 3/4 which would normally go through me:
Acked-by: Rusty Russell <[email protected]>
Thanks Greg,
Rusty.