Re: [PATCH 1/2] Add /sys/firmware/memmap

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 27 Jun 2008 13:12:54 +0200 Bernhard Walle <bwalle@suse.de> 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
> 
> With the following shell snippet one can print the memory map in the same form
> the kernel prints itself when booting on x86 (the E820 map).
> 
>   --------- 8< --------------------------
>     #!/bin/sh
>     cd /sys/firmware/memmap
>     for dir in * ; do
>         start=$(cat $dir/start)
>         end=$(cat $dir/end)
>         type=$(cat $dir/type)
>         printf "%016x-%016x (%s)\n" $start $[ $end +1] "$type"
>     done
>   --------- >8 --------------------------
> 
> That patch only provides the needed interface:
> 
>  1. The sysfs interface.
>  2. The structure and enumeration definition.
>  3. The function firmware_map_add() and firmware_map_add_early()
>     that should be called from architecture code (E820/EFI, for
>     example) to add the contents to the interface.
> 
> If the kernel is compiled without CONFIG_FIRMWARE_MEMMAP, the interface does
> nothing without cluttering the architecture-specific code with #ifdef's.
> 
> The purpose of the new interface is kexec: While /proc/iomem represents
> the *used* memory map (e.g. modified via kernel parameters like 'memmap'
> and 'mem'), the /sys/firmware/memmap tree represents the unmodified memory
> map provided via the firmware. So kexec can:
> 
>  - use the original memory map for rebooting,
>  - use the /proc/iomem for setting up the ELF core headers for kdump
>    case that should only represent the memory of the system.
> 
> The patch has been tested on i386 and x86_64.
> 
> ...
>
> +/*
> + * Firmware memory map entries
> + */
> +static LIST_HEAD(map_entries);

Alarm bells.  Please add a comment explaining why no locking is needed
for this.

> +/**
> + * Common implementation of firmware_map_add() and firmware_map_add_early()
> + * which expects a pre-allocated struct firmware_map_entry.
> + *
> + * @start: Start of the memory range.
> + * @end:   End of the memory range (inclusive).
> + * @type:  Type of the memory range.
> + * @entry: Pre-allocated (either kmalloc() or bootmem allocator), uninitialised
> + *         entry.
> + */

Busted kerneldoc.  It should start with

/**
 * firmware_map_add_entry - <stuff>

> +static int firmware_map_add_entry(resource_size_t start, resource_size_t end,
> +				  const char *type,
> +				  struct firmware_map_entry *entry)
> +{
> +	BUG_ON(start > end);
> +
> +	entry->start = start;
> +	entry->end = end;
> +	entry->type = type;
> +	INIT_LIST_HEAD(&entry->list);
> +	kobject_init(&entry->kobj, &memmap_ktype);
> +
> +	list_add_tail(&entry->list, &map_entries);
> +
> +	return 0;
> +}
> +
> +/*
> + * See <linux/firmware-map.h> for documentation.
> + */
> +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);

Actually, kmalloc() would have warned already.

> +	if (!entry)
> +		return -ENOMEM;
> +
> +	return firmware_map_add_entry(start, end, type, entry);
> +}
> +
> 
> ...
>
> +static ssize_t start_show(struct firmware_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->start);
> +}
> +
> +static ssize_t end_show(struct firmware_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->end);
> +}

Printing a resource_size_t needs a cast to `unsigned long long' to
avoid printk warnings.

> +static ssize_t type_show(struct firmware_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%s\n", entry->type);
> +}
> +
> +#define to_memmap_attr(_attr) container_of(_attr, struct memmap_attribute, attr)
> +#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)
> +
> +static ssize_t memmap_attr_show(struct kobject *kobj,
> +				struct attribute *attr, char *buf)
> +{
> +	struct firmware_map_entry *entry = to_memmap_entry(kobj);
> +	struct memmap_attribute *memmap_attr = to_memmap_attr(attr);
> +
> +	return memmap_attr->show(entry, buf);
> +}
> +
> +/*
> + * Initialises stuff and adds the entries in the map_entries list to
> + * sysfs. Important is that firmware_map_add() and firmware_map_add_early()
> + * must be called before late_initcall.

Please update the comment to provide the reason why this is important.

> + */
> +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;

Actually, you can do

	if (WARN_ON(!memmap_kset))
		return -ENOMEM;

(multiple instances)

> +	list_for_each_entry(entry, &map_entries, list) {
> +		entry->kobj.kset = memmap_kset;
> +		kobject_add(&entry->kobj, NULL, "%d", i++);

kobject_add() can fail.

I'd suggest that you enable CONFIG_ENABLE_MUST_CHECK in your usualconfig.

> +	}
> +
> +	return 0;
> +}
> +late_initcall(memmap_init);
> +
> 
> ...
>
> +/**
> + * Adds a firmware mapping entry. This function uses kmalloc() for memory
> + * allocation. Use firmware_map_add_early() if you want to use the bootmem
> + * allocator.
> + *
> + * That function must be called before late_initcall.
> + *
> + * @start: Start of the memory range.
> + * @end:   End of the memory range (inclusive).
> + * @type:  Type of the memory range.
> + *
> + * Returns 0 on success, or -ENOMEM if no memory could be allocated.
> + */
> +int firmware_map_add(resource_size_t start, resource_size_t end,
> +		     const char *type);

More busted kernedoc (multiple instances)

Please document C functions at the definition site, not in the header file.

a) because otherwise the documentation gets splattered across
   different files (eg - static functions?)

b) principle of least surprise: that's where people expect to find it.

> +/**
> + * Adds a firmware mapping entry. This function uses the bootmem allocator
> + * for memory allocation. Use firmware_map_add() if you want to use kmalloc().
> + *
> + * That function must be called before late_initcall.
> + *
> + * @start: Start of the memory range.
> + * @end:   End of the memory range (inclusive).
> + * @type:  Type of the memory range.
> + *
> + * Returns 0 on success, or -ENOMEM if no memory could be allocated.
> + */
> +int firmware_map_add_early(resource_size_t start, resource_size_t end,
> +			   const char *type);
> +
> +#else /* CONFIG_FIRMWARE_MEMMAP */
> +
> +static inline int firmware_map_add(resource_size_t start, resource_size_t end,
> +				   const char *type)
> +{
> +	return 0;
> +}
> +
> +static inline int firmware_map_add_early(resource_size_t start,
> +					 resource_size_t end, const char *type)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_FIRMWARE_MEMMAP */
> +
> +#endif /* _LINUX_FIRMWARE_MAP_H */