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/