linux-kernel - Re: [PATCH v6 1/4] genalloc: add a global pool list, allow to find pools by phys address

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20121120234613.47d8183c.akpm@linux-foundation.org>
Date:	Tue, 20 Nov 2012 23:46:13 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Richard Zhao <richard.zhao@...escale.com>,
	Huang Shijie <shijie8@...il.com>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Matt Porter <mporter@...com>,
	Fabio Estevam <fabio.estevam@...escale.com>,
	Javier Martin <javier.martin@...ta-silicon.com>,
	kernel@...gutronix.de, devicetree-discuss@...ts.ozlabs.org
Subject: Re: [PATCH v6 1/4] genalloc: add a global pool list, allow to find
 pools by phys address

On Fri, 16 Nov 2012 11:30:14 +0100 Philipp Zabel <p.zabel@...gutronix.de> wrote:

> This patch keeps all created pools in a global list and adds two
> functions that allow to retrieve the gen_pool pointer from a known
> physical address and from a device tree node.
>
> ...
>
> +/*
> + * gen_pool_find_by_phys - find a pool by physical start address
> + * @phys: physical address as added with gen_pool_add_virt
> + *
> + * Returns the pool that contains the chunk starting at phys,
> + * or NULL if not found.
> + */
> +struct gen_pool *gen_pool_find_by_phys(phys_addr_t phys)
> +{
> +	struct gen_pool *pool, *found = NULL;
> +	struct gen_pool_chunk *chunk;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(pool, &pools, next_pool) {
> +		list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
> +			if (phys == chunk->phys_addr) {
> +				found = pool;
> +				break;
> +			}
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(gen_pool_find_by_phys);

It is rather pointless to use the fancy super-fast RCU locking
around a linear search!  We have various data structures which can be
used to make this search much more efficient.  radix-tree is one, if
the search keys are unique (which is the case here).

Secondly, that whole "phys" concept doesn't need to be in there.  It
would be better to implement a far more general
gen_pool_find_by_key(unsigned long key) and then do the phys->ulong
specialization elsewhere.

Finally the changelog gives no indication *why* you feel the kernel
needs this feature.  What is it for?  What are the use cases?  This is
the most important information for reviewers, hence it should be up
there front and center, in lavish detail.

Because once this is understood:

a) people might be able to suggest alternatives.  Can't do that
   without the required info and

b) people might then be interested in merging the patch into a kernel!

> +#ifdef CONFIG_OF
> +/**
> + * of_get_named_gen_pool - find a pool by phandle property
> + * @np: device node
> + * @propname: property name containing phandle(s)
> + * @index: index into the phandle array
> + *
> + * Returns the pool that contains the chunk starting at the physical
> + * address of the device tree node pointed at by the phandle property,
> + * or NULL if not found.
> + */
> +struct gen_pool *of_get_named_gen_pool(struct device_node *np,
> +	const char *propname, int index)
> +{
> +	struct device_node *np_pool;
> +	struct resource res;
> +	int ret;
> +
> +	np_pool = of_parse_phandle(np, propname, index);
> +	if (!np_pool)
> +		return NULL;
> +	ret = of_address_to_resource(np_pool, 0, &res);
> +	if (ret < 0)
> +		return NULL;
> +	return gen_pool_find_by_phys((phys_addr_t) res.start);
> +}
> +EXPORT_SYMBOL_GPL(of_get_named_gen_pool);

Seems rather inappropriate that this should be in lib/genpool.c. 
Put it somewhere such as drivers/of/base.c, perhaps.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/