Re: [patch] genalloc for 2.6.12-rc-mm3

[Christoph Hellwig <hch@infradead.org>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 2520 bytes --]

On Tue, Apr 12, 2005 at 05:55:01AM -0400, Jes Sorensen wrote:
> Hi Andrew,
> 
> This patch provides the generic allocator needed for the ia64 mspec
> driver. Any chance you could add it to the mm tree?
> 
> Thanks,
> Jes
> 
> Generic allocator that can  be used by device driver to manage special
> memory etc. in particular it's used to manage uncached memory on ia64
> for the mspec driver. The allocator is based on the allocator from the
> sym53c8xx_2 driver.

So maybe as an example that your driver is usefull and not just additional
bloat you could convert sym53c8xx_2 (and ncr53c8xxx) to use it?

> +/*
> + *  Memory pool of a given kind.
> + *  Ideally, we want to use:
> + *  1) 1 pool for memory we donnot need to involve in DMA.
> + *  2) The same pool for controllers that require same DMA 
> + *     constraints and features.
> + *     The OS specific m_pool_id_t thing and the gen_pool_match() 
> + *     method are expected to tell the driver about.
> + */

these comments don't make any sense.

> +unsigned long gen_pool_alloc(struct gen_pool *poolp, int size);
> +void gen_pool_free(struct gen_pool *mp, unsigned long ptr, int size);
> +struct gen_pool *alloc_gen_pool(int nr_chunks, int max_chunk_shift,
> +				unsigned long (*fp)(struct gen_pool *),
> +				unsigned long data);

shouldn't there be a way to release the pool again?  Also we usuælly
call these _create/_destroy

> +#ifdef CONFIG_GENERIC_ALLOCATOR
> +	gen_pool_init();
> +#endif

please avoid hardcoded initcalls.

> +config GENERIC_ALLOCATOR
> +	boolean

	bool

> +#include <linux/config.h>

not needed.

> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/mm.h>
> +#include <linux/spinlock.h>
> +#include <linux/genalloc.h>
> +
> +#include <asm/page.h>
> +#include <asm/pal.h>
> +
> +

> +	/*
> +	 * This is really an arbitrary limit, +10 is enough for
> +	 * IA64_GRANULE_SHIFT.
> +	 */

What's IA64_GRANULE_SHIFT and why do we care?

> +#if DEBUG
> +	printk(KERN_DEBUG "gen_pool_alloc: s %02x, i %i, h %p\n", s, i, h);
> +#endif

please avoid ifdefs in the middle of the code.  if you think keeping this
trivial debug code in is so valueable add a helper that gets defined away
for the non-debug case.

> +int __init gen_pool_init(void)
> +{
> +	printk(KERN_INFO "Generic memory pool allocator v1.0\n");
> +	return 0;
> +}

no need to print a init message for a set of trivial library function