Re: [PATCH 5/5] rapidio: add destination ID allocation mechanism

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

On Wed,  3 Oct 2012 15:18:43 -0400
Alexandre Bounine <alexandre.bounine@idt.com> wrote:

> Replace the single global destination ID counter with per-net allocation
> mechanism to allow independent destID management for each available RapidIO
> network. Using bitmap based mechanism instead of counters allows
> destination ID release and reuse in systems that support hot-swap.
> 
>
> ...
>
> +static u16 rio_destid_alloc(struct rio_net *net)
> +{
> +	int destid;
> +	struct rio_id_table *idtab = &net->destid_table;
> +
> +	spin_lock(&idtab->lock);
> +	destid = find_next_zero_bit(idtab->table, idtab->max, idtab->next);
> +	if (destid >= idtab->max)
> +		destid = find_first_zero_bit(idtab->table, idtab->max);
> +
> +	if (destid < idtab->max) {
> +		idtab->next = destid + 1;
> +		if (idtab->next >= idtab->max)
> +			idtab->next = 0;
> +		set_bit(destid, idtab->table);
> +		destid += idtab->start;
> +	} else
> +		destid = RIO_INVALID_DESTID;
> +
> +	spin_unlock(&idtab->lock);
> +	return (u16)destid;
> +}

This is round-robin rather than the simpler first-fit, and this reader
doesn't know why.  Suggest the addition of a code comment explaining
this decision.

> +/*
> + * rio_destid_reserve - Reserve the specivied destID
> + * net: RIO network
> + * destid: destID to reserve
> + *
> + * Tries to reserve the specified destID.
> + * Returns 0 if successfull.
> + */
> +static int rio_destid_reserve(struct rio_net *net, u16 destid)
> +{
> +	int oldbit;
> +	struct rio_id_table *idtab = &net->destid_table;
> +
> +	destid -= idtab->start;
> +	spin_lock(&idtab->lock);
> +	oldbit = test_and_set_bit(destid, idtab->table);
> +	spin_unlock(&idtab->lock);
> +	return oldbit;
> +}
> +
> +/*
> + * rio_destid_free - free a previously allocated destID
> + * net: RIO network
> + * destid: destID to free
> + *
> + * Makes the specified destID available for use.
> + */
> +static void rio_destid_free(struct rio_net *net, u16 destid)
> +{
> +	struct rio_id_table *idtab = &net->destid_table;
> +
> +	destid -= idtab->start;
> +	spin_lock(&idtab->lock);
> +	clear_bit(destid, idtab->table);
> +	spin_unlock(&idtab->lock);
> +}
> +
> +/*
> + * rio_destid_first - return first destID in use
> + * net: RIO network
> + */
> +static u16 rio_destid_first(struct rio_net *net)
> +{
> +	int destid;
> +	struct rio_id_table *idtab = &net->destid_table;
> +
> +	spin_lock(&idtab->lock);
> +	destid = find_first_bit(idtab->table, idtab->max);
> +	if (destid >= idtab->max)
> +		destid = RIO_INVALID_DESTID;
> +	else
> +		destid += idtab->start;
> +	spin_unlock(&idtab->lock);
> +	return (u16)destid;
> +}
> +
> +/*
> + * rio_destid_next - return next destID in use
> + * net: RIO network
> + * from: destination ID from which search shall continue
> + */

All these code comments look like kerneldoc, but they aren't.  kerneldoc
uses /** and identifiers have a leading `@'.  And that's OK - one
doesn't *have* to use kerneldoc.  But a lot of
drivers/rapidio/rio-scan.c is already using kerneldoc so the
inconsistency is odd.

>
> ...
>
> -static struct rio_net __devinit *rio_alloc_net(struct rio_mport *port)
> +static struct rio_net __devinit *rio_alloc_net(struct rio_mport *port,
> +					       int do_enum, u16 start)
>  {
>  	struct rio_net *net;
>  
>  	net = kzalloc(sizeof(struct rio_net), GFP_KERNEL);
> +	if (net && do_enum) {
> +		net->destid_table.table = kzalloc(
> +			BITS_TO_LONGS(RIO_MAX_ROUTE_ENTRIES(port->sys_size)) *
> +			sizeof(long),
> +			GFP_KERNEL);

kcalloc() would be idiomatic here.

> +		if (net->destid_table.table == NULL) {
> +			pr_err("RIO: failed to allocate destID table\n");
> +			kfree(net);
> +			net = NULL;
> +		} else {
> +			net->destid_table.start = start;
> +			net->destid_table.next = 0;
> +			net->destid_table.max =
> +					RIO_MAX_ROUTE_ENTRIES(port->sys_size);
> +			spin_lock_init(&net->destid_table.lock);
> +		}
> +	}
> +
>
> ...
>