linux-kernel - Re: [PATCH 1/6] firewire: handling of cards, buses, nodes

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070502192214.GA1248@infradead.org>
Date:	Wed, 2 May 2007 20:22:14 +0100
From:	Christoph Hellwig <hch@...radead.org>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
Cc:	linux-kernel@...r.kernel.org, Kristian H??gsberg <krh@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux1394-devel <linux1394-devel@...ts.sourceforge.net>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 1/6] firewire: handling of cards, buses, nodes

On Wed, May 02, 2007 at 02:15:42PM +0200, Stefan Richter wrote:
> +/*						-*- c-basic-offset: 8 -*-

Please don't pollute the code with annotation for some editors.

> + * fw-card.c - card level functions

Please don't put the filename into a comment inside the filename.
It's not actually useful and it gets out of date very easily.

> +static DECLARE_RWSEM(card_rwsem);

this one is only ever used with down_read/up_read so a normal
mutex would probably better suited.

> +static LIST_HEAD(card_list);

> +#define bib_pmc			((1) << 27)
> +#define bib_bmc			((1) << 28)
> +#define bib_isc			((1) << 29)
> +#define bib_cmc			((1) << 30)
> +#define bib_imc			((1) << 31)

These are rather odd names for constants.  They should at least
start with an uppercase letter if not be all uppercase.  Also
an enum might be right here.

> +static u32 *
> +generate_config_rom (struct fw_card *card, size_t *config_rom_length)

Please don't put white spaces after the function identifier.

> +	/* Initialize contents of config rom buffer.  On the OHCI
> +	 * controller, block reads to the config rom accesses the host
> +	 * memory, but quadlet read access the hardware bus info block
> +	 * registers.  That's just crack, but it means we should make
> +	 * sure the contents of bus info block in host memory mathces
> +	 * the version stored in the OHCI registers. */

Normal style for block comments is:

	/*
	 * Initialize contents of config rom buffer.  On the OHCI
	 * controller, block reads to the config rom accesses the host
	 * memory, but quadlet read access the hardware bus info block
	 * registers.  That's just crack, but it means we should make
	 * sure the contents of bus info block in host memory mathces
	 * the version stored in the OHCI registers.
	 */

> +struct fw_node {
> +	u16 node_id;
> +	u8 color;
> +	u8 port_count;
> +	unsigned link_on : 1;
> +	unsigned initiated_reset : 1;
> +	unsigned b_path : 1;
> +	u8 phy_speed : 3; /* As in the self ID packet. */
> +	u8 max_speed : 5; /* Minimum of all phy-speeds and port speeds on
> +			   * the path from the local node to this node. */
> +	u8 max_depth : 4; /* Maximum depth to any leaf node */
> +	u8 max_hops : 4;  /* Max hops in this sub tree */

I don't think we need to save the few bits here and can just use
u8 instead of bitfields.

> +		/* Pop the child nodes off the stack and push the new node. */
> +		__list_del(h->prev, &stack);

Please don't ever use __list_del directly, it's an interna implementation
detail.

I also notices that close to none of the export functions have kerneldoc
comment blocks.  I think we really should have them on something that
is a driver API.
-
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/