Mark McLoughlin
2008-Dec-10 17:44 UTC
[PATCH 0/6] Clean up virtio device object handling [was Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref]
(Moved from kvm at vger to virtualization at 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 <markmc at redhat.com> wrote: > > On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote: > >> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <markmc at redhat.com> wrote: > >> > On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote: > >> >> Mark McLoughlin wrote: > >> >> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote: > >> >> >> On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote: > >> >> >> > >> >> >>> Another example of a lack of an explicit dependency causing problems is > >> >> >>> Fedora's mkinitrd having this hack: > >> >> >>> > >> >> >>> if echo $PWD | grep -q /virtio-pci/ ; then > >> >> >>> findmodule virtio_pci > >> >> >>> fi > >> >> >>> > >> >> >>> which basically says "if this is a virtio device, don't forget to > >> >> >>> include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, > >> >> >>> but this is a particularly unusual one. > >> >> >>> > >> >> >> Um, I don't know what this does, sorry. > >> >> >> > >> >> >> I have no idea how Fedora chooses what to put in an initrd; I can't think > >> >> >> of a sensible way of deciding what goes in and what doesn't other than > >> >> >> lists and heuristics. > >> >> >> > >> >> > > >> >> > Fedora's mkinitrd creates an initrd suitable to boot the machine you run > >> >> > mkinitrd on, rather than creating an initrd suitable to boot any > >> >> > machine. > >> >> > > >> >> > So, it goes "ah, / is mounted from /dev/vda, we need to include > >> >> > virtio_blk and it's dependencies". It does that in a generic way that > >> >> > works well for most setups: > >> >> > > >> >> > 1) Find the device name (e.g. vda) below /sys/block > >> >> > > >> >> > 2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1 > >> >> > > >> >> > 3) Find the module need for this through either 'modalias' or the > >> >> > 'driver/module' symlink > >> >> > > >> >> > 4) Use modprobe to list any dependencies of that module > >> >> > > >> >> > Clearly, virtio-pci won't be pulled in by any of this so we've added a > >> >> > hack to say "oh, it's a virtio device, let's include virtio_pci just in > >> >> > case". > >> >> > > >> >> > It's not even the case that mkinitrd needs to know how to include the > >> >> > the module for the bus, because in our case that's virtio.ko ... we've > >> >> > pretty effectively hidden the the bus *implementation* from userspace. > >> >> > > >> >> > I don't think this is worth wasting too much time fixing, that's why I'm > >> >> > thinking we should just make virtio_pci built-in by default with > >> >> > CONFIG_KVM_GUEST. > >> >> > > >> >> > >> >> What if we have multiple virtio transports? > >> > > >> > I don't think that's so much an an issue (just build in any transport > >> > supported by KVM), but rather that you might build a non-pv_ops kernel > >> > to run on QEMU which would benefit from using virtio drivers ... > >> > > >> >> Is there a way that we can > >> >> expose the relationship with virtio-blk and virtio-pci in sysfs? We > >> >> have a struct device for the PCI device, it's just a matter of making > >> >> the link visible. > >> > > >> > It feels a bit like busy work to generalise this since only virtio_pci > >> > can be built as a module, but here's a patch. > >> > > >> > The mkinitrd hack turns into: > >> > > >> > # Handle finding virtio bus implementations > >> > if [ -L ./virtio_module ] ; then > >> > findmodule $(basename $(readlink ./virtio_module)) > >> > else if echo $PWD | grep -q /virtio-pci/ ; then > >> > findmodule virtio_pci > >> > fi; fi > >> > > >> > [PATCH] virtio: add a 'virtio_module' sysfs symlink > >> > >> Doesn't the device have a "driver" link already? If yes, the driver it > >> points to should have a "module" link. > > > > The virtio bus is an abstraction that has several different backend > > implementations - currently virtio-pci, lguest and kvm-s390. > > > > So yes, the driver/module link gives us the device driver, but the > > virtio_module link is to the virtio bus driver (aka implementation, > > transport, backend, ...): > > > > $> basename $(readlink virtio_module) > > virtio_pci > > $> basename $(readlink driver/module) > > virtio_net > > I see. But why not just call it "module", like we do in all other > places, when it points to /sys/module/. > > To find dependent modules, you would walk up the chain of parents, and > include everything that is found by looking for "driver/module" and > "module" links? > > Wouldn't that make it completely generic, without any virtio specific hacks?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.
Kay Sievers
2008-Dec-10 18:07 UTC
[PATCH 0/6] Clean up virtio device object handling [was Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref]
On Wed, Dec 10, 2008 at 18:44, Mark McLoughlin <markmc at redhat.com> wrote:> (Moved from kvm at vger to virtualization at 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 <markmc at redhat.com> wrote: >> > On Tue, 2008-12-09 at 19:16 +0100, Kay Sievers wrote: >> >> On Tue, Dec 9, 2008 at 17:41, Mark McLoughlin <markmc at redhat.com> wrote: >> >> > On Mon, 2008-12-08 at 08:46 -0600, Anthony Liguori wrote: >> >> >> Mark McLoughlin wrote: >> >> >> > On Sun, 2008-12-07 at 18:52 +1030, Rusty Russell wrote: >> >> >> >> On Saturday 06 December 2008 01:37:06 Mark McLoughlin wrote: >> >> >> >> >> >> >> >>> Another example of a lack of an explicit dependency causing problems is >> >> >> >>> Fedora's mkinitrd having this hack: >> >> >> >>> >> >> >> >>> if echo $PWD | grep -q /virtio-pci/ ; then >> >> >> >>> findmodule virtio_pci >> >> >> >>> fi >> >> >> >>> >> >> >> >>> which basically says "if this is a virtio device, don't forget to >> >> >> >>> include virtio_pci in the initrd too!". Now, mkinitrd is full of hacks, >> >> >> >>> but this is a particularly unusual one. >> >> >> >>> >> >> >> >> Um, I don't know what this does, sorry. >> >> >> >> >> >> >> >> I have no idea how Fedora chooses what to put in an initrd; I can't think >> >> >> >> of a sensible way of deciding what goes in and what doesn't other than >> >> >> >> lists and heuristics. >> >> >> >> >> >> >> > >> >> >> > Fedora's mkinitrd creates an initrd suitable to boot the machine you run >> >> >> > mkinitrd on, rather than creating an initrd suitable to boot any >> >> >> > machine. >> >> >> > >> >> >> > So, it goes "ah, / is mounted from /dev/vda, we need to include >> >> >> > virtio_blk and it's dependencies". It does that in a generic way that >> >> >> > works well for most setups: >> >> >> > >> >> >> > 1) Find the device name (e.g. vda) below /sys/block >> >> >> > >> >> >> > 2) Follow the 'device' link to e.g. /sys/devices/virtio-pci/virtio1 >> >> >> > >> >> >> > 3) Find the module need for this through either 'modalias' or the >> >> >> > 'driver/module' symlink >> >> >> > >> >> >> > 4) Use modprobe to list any dependencies of that module >> >> >> > >> >> >> > Clearly, virtio-pci won't be pulled in by any of this so we've added a >> >> >> > hack to say "oh, it's a virtio device, let's include virtio_pci just in >> >> >> > case". >> >> >> > >> >> >> > It's not even the case that mkinitrd needs to know how to include the >> >> >> > the module for the bus, because in our case that's virtio.ko ... we've >> >> >> > pretty effectively hidden the the bus *implementation* from userspace. >> >> >> > >> >> >> > I don't think this is worth wasting too much time fixing, that's why I'm >> >> >> > thinking we should just make virtio_pci built-in by default with >> >> >> > CONFIG_KVM_GUEST. >> >> >> > >> >> >> >> >> >> What if we have multiple virtio transports? >> >> > >> >> > I don't think that's so much an an issue (just build in any transport >> >> > supported by KVM), but rather that you might build a non-pv_ops kernel >> >> > to run on QEMU which would benefit from using virtio drivers ... >> >> > >> >> >> Is there a way that we can >> >> >> expose the relationship with virtio-blk and virtio-pci in sysfs? We >> >> >> have a struct device for the PCI device, it's just a matter of making >> >> >> the link visible. >> >> > >> >> > It feels a bit like busy work to generalise this since only virtio_pci >> >> > can be built as a module, but here's a patch. >> >> > >> >> > The mkinitrd hack turns into: >> >> > >> >> > # Handle finding virtio bus implementations >> >> > if [ -L ./virtio_module ] ; then >> >> > findmodule $(basename $(readlink ./virtio_module)) >> >> > else if echo $PWD | grep -q /virtio-pci/ ; then >> >> > findmodule virtio_pci >> >> > fi; fi >> >> > >> >> > [PATCH] virtio: add a 'virtio_module' sysfs symlink >> >> >> >> Doesn't the device have a "driver" link already? If yes, the driver it >> >> points to should have a "module" link. >> > >> > The virtio bus is an abstraction that has several different backend >> > implementations - currently virtio-pci, lguest and kvm-s390. >> > >> > So yes, the driver/module link gives us the device driver, but the >> > virtio_module link is to the virtio bus driver (aka implementation, >> > transport, backend, ...): >> > >> > $> basename $(readlink virtio_module) >> > virtio_pci >> > $> basename $(readlink driver/module) >> > virtio_net >> >> I see. But why not just call it "module", like we do in all other >> places, when it points to /sys/module/. >> >> To find dependent modules, you would walk up the chain of parents, and >> include everything that is found by looking for "driver/module" and >> "module" links? >> >> Wouldn't that make it completely generic, without any virtio specific hacks? > > 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
Christian Borntraeger
2008-Dec-11 09:05 UTC
[PATCH 5/6] kvm-s390: use register_virtio_root_device()
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 Wed, 10 Dec 2008 17:45:35 +0000, Mark McLoughlin <markmc at redhat.com> 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 <markmc at redhat.com> > --- > 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
On Thu, 11 Dec 2008 16:16:53 +0000, Mark McLoughlin <markmc at redhat.com> 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 <markmc at redhat.com> > --- > 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 <markmc at redhat.com> 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 <markmc at redhat.com> > --- > 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 <cornelia.huck at de.ibm.com> > - * Carsten Otte <cotte at de.ibm.com> > - * > - * 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 (cornelia.huck at de.ibm.com) > - * Carsten Otte (cotte at de.ibm.com) > - */ > - > -#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: cornelia.huck at de.ibm.com
Rusty Russell
2008-Dec-15 22:27 UTC
[PATCH 2/4] virtio: do not statically allocate root device
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 <rusty at rustcorp.com.au> Thanks Greg, Rusty.
Apparently Analagous Threads
- [PATCH 0/6] Clean up virtio device object handling [was Re: [PATCH] virtio: make PCI devices take a virtio_pci module ref]
- [PATCH 10/21] Staging: hv: Cleanup root device handling
- [PATCH 10/21] Staging: hv: Cleanup root device handling
- [PATCH 3/5] cpumask: convert misc driver functions
- [PATCH 3/5] cpumask: convert misc driver functions