Re: [PATCH 2/5] net: add IEEE 802.15.4 partial implementation

[Ben Hutchings <ben-/+tVBieCtBitmTQ+vhA3Yw@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5818 bytes --]

On Tue, 2009-05-26 at 15:23 +0400, Dmitry Eremin-Solenikov wrote:
[...]
> diff --git a/include/net/ieee802154/af_ieee802154.h b/include/net/ieee802154/af_ieee802154.h
> new file mode 100644
> index 0000000..6eb7f51
> --- /dev/null
> +++ b/include/net/ieee802154/af_ieee802154.h
[...]
> +#ifdef __KERNEL__
> +#include <linux/skbuff.h>
> +#include <linux/netdevice.h>

The struct declarations would be sufficient.

> +extern struct proto ieee802154_raw_prot;
> +extern struct proto ieee802154_dgram_prot;
> +void ieee802154_raw_deliver(struct net_device *dev, struct sk_buff *skb);
> +int ieee802154_dgram_deliver(struct net_device *dev, struct sk_buff *skb);
> +#endif
> +
> +#endif
> diff --git a/include/net/ieee802154/beacon.h b/include/net/ieee802154/beacon.h
> new file mode 100644
> index 0000000..baca263
> --- /dev/null
> +++ b/include/net/ieee802154/beacon.h
[...]
> +/* Per spec; optimizations are needed */
> +struct ieee802154_pandsc {
> +	struct list_head	list;
> +	struct ieee802154_addr	addr; /* Contains panid */
> +	int			channel;
> +	u16			sf;
> +	bool			gts_permit;
> +	u8			lqi;
> +	u32			timestamp; /* FIXME */

When and how does this need to be fixed, if at all?

> +	bool			security;
> +	u8			mac_sec;
> +	bool			sec_fail;
> +};
[...]
> --- /dev/null
> +++ b/include/net/ieee802154/const.h
[...]
> +/* Time related constants, in microseconds.
> + *
> + * The 1SYM_TIME values are shown how much time is needed to transmit one
> + * symbol across media.
> + * The callculation is following:
> + * For a 2450 MHZ radio freq rate is 62,5 ksym/sec. A byte (8 bit) transfered
> + * by low 4 bits in first symbol, high 4 bits in next symbol. So, to transmit
> + * 1 byte in 2450Mhz freq 2 symbols are needed. Therefore we have 31,25 kbyte/sec
> + * rate. 1 symbol transfered in 16*10^(-6) sec, or 16 microseconds.
> + * For a 868Mhz and 915Mhz, 1 symbol is equal to 1 byte. So, we have 20kbyte/sec
> + * and 40 kbyte/sec respectively. And 5*10^(-5) sec and 2,5*10(-5) sec,
> + * or 50 and 25 microseconds respectively for 868Mhz and 915Mhz freq.
> + */

This is so verbose as to be unhelpful.  Since the following constants
only refer to symbol time, it should be sufficient to refer to the
specified symbol rates.

> +#define IEEE802154_2450MHZ_1SYM_TIME	16
> +#define IEEE802154_868MHZ_1SYM_TIME	50
> +#define IEEE802154_915MHZ_1SYM_TIME	25
> +
> +/******************************************************************************/
> +/* MAC constants */
> +/******************************************************************************/
> +/** The number of symbols forming a superframe slot when the superframe order
> + * is equal to 0.
> + */

Don't use two asterisks at the start of a comment unless it's in
kernel-doc format.

[...]
> +/**
> + * \brief PHY return codes description
> + *
> + * The return values of PHY operations
> + */

...and definitely don't use Doxygen format.

[...]
> +/* #define ZB_ED_EDGE	0x7f */

Define it or delete it.

> +/* I've got 0xBE on idle channel; let threshold be a little higher */
> +#define ZB_ED_EDGE	0xc8
> +/* In an ideal world this should be 1 */
> +#define IEEE802154_SLOW_SERIAL_FIXUP	75

Yes but what are these *for*?

> +#endif /* IEEE802154_CONST_H */
> diff --git a/include/net/ieee802154/crc.h b/include/net/ieee802154/crc.h
> new file mode 100644
> index 0000000..5b37218
> --- /dev/null
> +++ b/include/net/ieee802154/crc.h
> @@ -0,0 +1,38 @@
> +/*
> + * based on crc-itu-t.c.
> + * Basically it's CRC-ITU-T but with inverted bit numbering

This can probably be handled by crc_itu_t_bitreversed():

http://svn.debian.org/wsvn/kernel/dists/trunk/linux-2.6/debian/patches/features/all/lib-crcitut-bit-reversed.patch

(That patch hasn't gone anywhere yet as Greg K-H is dragging his heels
over actually fixing his crap, but feel free to resubmit it as part of
your own patch series.)

[...]
> --- /dev/null
> +++ b/include/net/ieee802154/nl.h
[...]
> +/* commands */
> +/* REQ should be responded with CONF
> + * and INDIC with RESP
> + */
> +enum {
> +	__IEEE802154_COMMAND_INVALID,
> +
> +	IEEE802154_ASSOCIATE_REQ,
> +	IEEE802154_ASSOCIATE_CONF,
> +	IEEE802154_DISASSOCIATE_REQ,
> +	IEEE802154_DISASSOCIATE_CONF,
> +	IEEE802154_GET_REQ,
> +	IEEE802154_GET_CONF,
> +/* 	IEEE802154_GTS_REQ, */
> +/* 	IEEE802154_GTS_CONF, */

Why are some of these commented out?  Since they're apparently exposed
to user-space you can't insert them later.

[...]
> --- /dev/null
> +++ b/net/ieee802154/beacon.c
[...]
> +	/* TODO GTS */
> +	gts = 0;
> +
> +	if (flags & IEEE802154_BEACON_FLAG_GTSPERMIT)
> +		gts |= IEEE802154_BEACON_GTS_PERMIT;
> +	memcpy(skb_put(skb, sizeof(gts)), &gts, sizeof(gts));
> +
> +	/* FIXME pending address */
> +	addr16_cnt = 0;
> +	addr64_cnt = 0;
> +#if 0
> +	/* need more thinking about this */
> +	list_for_each_entry(l, al->list, list) {
> +		struct ieee802154_addr *s = container_of(l, struct list_head, list);
> +		if (s->addr_type == IEEE802154_ADDR_LONG)
> +			addr16_cnt++;
> +
> +		if (s->addr_type == IEEE802154_ADDR_SHORT)
> +			addr64_cnt++;
> +	}
> +#endif

This all looks very unfinished - as do several other sections with
FIXME, #if 0, commented-out code...

[...]
> +#define IEEE802154_FETCH_U64(skb, var)			\
> +	do {						\
> +		if (skb->len < IEEE802154_ADDR_LEN)	\

I see what you did there...

> +			goto exit_error;		\
> +		fetch_skb_u64(skb, &var);		\
> +	} while (0)
[...]
> --- /dev/null
> +++ b/net/ieee802154/netlink.c
> @@ -0,0 +1,637 @@
> +/*
> + * Netlink intefcace for IEEE 802.15.4 stack
[...]

Speling misstake.

Ben.

-- 
Ben Hutchings
Teamwork is essential - it allows you to blame someone else.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]