On Sat, 2007-10-27 at 23:08 +0200, Kay Sievers wrote:
> On Sat, 2007-10-27 at 09:02 -0700, Greg KH wrote:
> > Ah, I see a few problems. Here, try this version instead. It's
> > compile-tested only, and should be a lot simpler.
> >
> > Note, we still are not setting the parent to the new bdi structure
> > properly, so the devices will show up in /sys/devices/virtual/ instead
> > of in their proper location. To do this, we need the parent of the
> > device, which I'm not so sure what it should be (block device? block
> > device controller?)
>
> Assigning a parent device will only work with the upcoming conversion of
> the raw kobjects in the block subsystem to "struct device".
>
> A few comments to the patch:
>
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -8,6 +8,7 @@
> > #include <linux/compiler.h> /* for inline */
> > #include <linux/types.h> /* for size_t */
> > #include <linux/stddef.h> /* for NULL */
> > +#include <stdarg.h>
> >
> > #ifdef __cplusplus
> > extern "C" {
> > @@ -111,6 +112,9 @@ extern void *kmemdup(const void *src, si
> > extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> > extern void argv_free(char **argv);
> >
> > +char *kvprintf(const char *fmt, va_list args);
> > +char *kprintf(const char *fmt, ...);
>
> Why is that here? I don't think we need this when we use the existing:
> kvasprintf(GFP_KERNEL, fmt, args)
Ignorance of the existance of said function. Thanks for pointing it out.
(kobject_set_name ought to use it too I guess)
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
>
> > +
> > +static struct device_attribute bdi_dev_attrs[] = {
> > + __ATTR(readahead, 0644, readahead_show, readahead_store),
> > + __ATTR_RO(reclaimable),
> > + __ATTR_RO(writeback),
> > + __ATTR_RO(dirty),
> > + __ATTR_RO(bdi_dirty),
> > +};
>
> Default attributes will need the NULL termination back (see below).
>
> > +static __init int bdi_class_init(void)
> > +{
> > + bdi_class = class_create(THIS_MODULE, "bdi");
> > + return 0;
> > +}
> > +
> > +__initcall(bdi_class_init);
> > +
> > +int bdi_register(struct backing_dev_info *bdi, const char *fmt, ...)
>
> This function should accept a: "struct device *parent" and all callers
> just pass NULL until the block layer conversion gets merged.
Yeah, you're right, but I wanted to just get something working before
bothering with the parent thing.
> > +{
> > + char *name;
> > + va_list args;
> > + int ret = -ENOMEM;
> > + int i;
> > +
> > + va_start(args, fmt);
> > + name = kvprintf(fmt, args);
>
> kvasprintf(GFP_KERNEL, fmt, args);
>
> > + va_end(args);
> > +
> > + if (!name)
> > + return -ENOMEM;
> > +
> > + bdi->dev = device_create(bdi_class, NULL, MKDEV(0,0), name);
>
> The parent should be passed here.
>
> > + for (i = 0; i < ARRAY_SIZE(bdi_dev_attrs); i++) {
> > + ret = device_create_file(bdi->dev, &bdi_dev_attrs[i]);
> > + if (ret)
> > + break;
> > + }
> > + if (ret) {
> > + while (--i >= 0)
> > + device_remove_file(bdi->dev, &bdi_dev_attrs[i]);
> > + device_unregister(bdi->dev);
> > + bdi->dev = NULL;
> > + }
>
> All this open-coded attribute stuff should go away and be replaced by:
> bdi_class->dev_attrs = bdi_dev_attrs;
> Otherwise at event time the attributes are not created and stuff hooking
> into the events will not be able to set values. Also, the core will do
> proper add/remove and error handling then.
ok, that's good to know. someone ought to write a book on how to use all
this... really, even the functions are bare of documentation or
comments.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
