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: <8D983423E7EDF846BB3056827B8CC5D1499FC823@corpmail2.na.ads.idt.com>
Date:	Thu, 4 Oct 2012 20:39:01 +0000
From:	"Bounine, Alexandre" <Alexandre.Bounine@....com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	Matt Porter <mporter@...nel.crashing.org>,
	Li Yang <leoli@...escale.com>
Subject: RE: [PATCH 5/5] rapidio: add destination ID allocation mechanism

On Wed, October 03, 2012 6:36 PM
Andrew Morton <akpm@...ux-foundation.org> wrote:
> 
> On Wed,  3 Oct 2012 15:18:43 -0400
> Alexandre Bounine <alexandre.bounine@....com> wrote:
> 
> > ...
> >
> > +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.

This is to make debugging easier. Having fresh new destID assigned to
a device after insertion helps to analyze switch routing table updates.
Yes, find-first is sufficient and better understandable (I had it in
early version).
I will switch to find-first scenario to make things clear.   

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

Idea here was that keeping static functions out of kerneldoc may
have sense and result in cleaner doc output. This was my first attempt
to take that path. Probably, kerneldoc adjustment patch for entire
file (or even all RapidIO files) would be more appropriate instead of
changing style half-way.
As you noticed, these comments are similar to kerneldoc - easy to get back
to old style. I will restore kerneldoc style for affected functions. 

> 
> >
> > ...
> >
> > -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.

Agree. Will change.

> 
> > +		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);
> > +		}
> > +	}
> > +
> >
> > ...
> >

--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ