Messages in this thread | | | Date | Thu, 26 Jun 2008 15:24:58 -0700 | From | Greg KH <> | Subject | Re: [PATCH 1/2] Add /sys/firmware/memmap |
| |
On Thu, Jun 26, 2008 at 10:19:01PM +0200, Bernhard Walle wrote: > This patch adds /sys/firmware/memmap interface that represents the BIOS > (or Firmware) provided memory map. The tree looks like: > > /sys/firmware/memmap/0/start (hex number) > end (hex number) > type (string) > ... /1/start > end > type
Please provide new entries in Documentation/ABI/ for these new sysfs files with all of this information.
> +/* > + * Firmware memory map entries > + */ > +LIST_HEAD(map_entries);
Should this be static?
> +int firmware_map_add(resource_size_t start, resource_size_t end, > + const char *type) > +{ > + struct firmware_map_entry *entry; > + > + entry = kmalloc(sizeof(struct firmware_map_entry), GFP_ATOMIC); > + WARN_ON(!entry); > + if (!entry) > + return -ENOMEM; > + > + return firmware_map_add_entry(start, end, type, entry);
Where is the kobject initialized properly?
Ah, later on, that's scary...
> +static struct kobj_type memmap_ktype = { > + .sysfs_ops = &memmap_attr_ops, > + .default_attrs = def_attrs, > +};
Do you really need your own kobj_type here? What you want is just a directory, and some attributes assigned to the kobject, can't you use the default kobject attributes for them?
I'm not saying this is incorrect, it looks implemented properly, just curious.
> +static int __init memmap_init(void) > +{ > + int i = 0; > + struct firmware_map_entry *entry; > + struct kset *memmap_kset; > + > + memmap_kset = kset_create_and_add("memmap", NULL, firmware_kobj); > + WARN_ON(!memmap_kset); > + if (!memmap_kset) > + return -ENOMEM; > + > + list_for_each_entry(entry, &map_entries, list) {
So the list is supposed to be set up before this function is called? Is that because of early boot issues?
You should document this somehow.
> +/* > + * Firmware map entry. Because firmware memory maps are flat and not > + * hierarchical, it's ok to organise them in a linked list. No parent > + * information is necessary as for the resource tree. > + */ > +struct firmware_map_entry { > + resource_size_t start; /* start of the memory range */ > + resource_size_t end; /* end of the memory range (incl.) */ > + const char *type; /* type of the memory range */ > + struct list_head list; /* entry for the linked list */ > + struct kobject kobj; /* kobject for each entry */ > +};
Does this really need to be in the .h file?
thanks,
greg k-h
| |