lkml.org 
[lkml]   [2008]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-06-27 00:29    [W:0.065 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site