Re: [PATCH 4/6] net: add NL802154 interface for configuration of 802.15.4 devices

[Johannes Berg <johannes@sipsolutions.net>]

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

On Wed, 2009-06-03 at 14:52 +0400, Dmitry Eremin-Solenikov wrote:

> +	IEEE802154_ATTR_CAPABILITY, /* FIXME: this is association */

Fix what?

> +#define IEEE802154_ATTR_MAX (__IEEE802154_ATTR_MAX - 1)
> +#define NLA_HW_ADDR	NLA_U64
> +#define NLA_GET_HW_ADDR(attr, addr) do { u64 _temp = nla_get_u64(attr); memcpy(addr, &_temp, 8); } while (0)
> +#define NLA_PUT_HW_ADDR(msg, attr, addr) do { u64 _temp; memcpy(&_temp, addr, 8); NLA_PUT_U64(msg, attr, _temp); } while (0)

I really don't like this namespace pollution.

> +#ifdef IEEE802154_NL_WANT_POLICY
> +static struct nla_policy ieee802154_policy[IEEE802154_ATTR_MAX + 1] = {

Ho humm. This shouldn't be in a header file. Not even with an #ifdef
that exactly one C file then sets.

> +	[IEEE802154_ATTR_DURATION] = { .type = NLA_U8, },
> +#ifdef __KERNEL__
> +	[IEEE802154_ATTR_ED_LIST] = { .len = 27 },
> +#else

Ick.

> +/* commands */
> +/* REQ should be responded with CONF
> + * and INDIC with RESP
> + */
> +enum {

kernel-doc explaining the commands would be immensely helpful.


> +	IEEE802154_GTS_REQ, /* Not supported yet */
> +	IEEE802154_GTS_INDIC, /* Not supported yet */
> +	IEEE802154_GTS_CONF, /* Not supported yet */
> +	IEEE802154_RX_ENABLE_REQ, /* Not supported yet */
> +	IEEE802154_RX_ENABLE_CONF, /* Not supported yet */

Just leave it out then. You're fixing ABI here.

> +#ifdef __KERNEL__
> +struct net_device;
> +
> +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap);
> +int ieee802154_nl_assoc_confirm(struct net_device *dev, u16 short_addr, u8 status);
> +int ieee802154_nl_disassoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 reason);
> +int ieee802154_nl_disassoc_confirm(struct net_device *dev, u8 status);
> +int ieee802154_nl_scan_confirm(struct net_device *dev, u8 status, u8 scan_type, u32 unscanned,
> +		u8 *edl/*, struct list_head *pan_desc_list */);
> +int ieee802154_nl_beacon_indic(struct net_device *dev, u16 panid, u16 coord_addr); /* TODO */
> +#endif

Why not just use two header files, one in net/ and one in linux/?

> -obj-$(CONFIG_IEEE802154) +=	af_802154.o
> +obj-$(CONFIG_IEEE802154) +=	nl802154.o af_802154.o
> +nl802154-objs		:= netlink.o

That doesn't make any sense. Why the indirection?

> +#include <net/ieee802154/af_ieee802154.h>
> +#define IEEE802154_NL_WANT_POLICY
> +#include <net/ieee802154/nl802154.h>

Like I thought. That's ugly.

> +static int ieee802154_nl_put_failure(struct sk_buff *msg)
> +{
> +	void *hdr = genlmsg_data(NLMSG_DATA(msg->data)); /* XXX: nlh is right at the start of msg */
> +	genlmsg_cancel(msg, hdr);
> +	nlmsg_free(msg);
> +	return -ENOBUFS;
> +}

This seems weird.

> +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap)
> +{
> +	struct sk_buff *msg;
> +
> +	pr_debug("%s\n", __func__);
> +
> +	msg = ieee802154_nl_create(/* flags*/ 0, IEEE802154_ASSOCIATE_INDIC);
> +	if (!msg)
> +		return -ENOBUFS;
> +
> +	NLA_PUT_STRING(msg, IEEE802154_ATTR_DEV_NAME, dev->name);
> +	NLA_PUT_U32(msg, IEEE802154_ATTR_DEV_INDEX, dev->ifindex);
> +	NLA_PUT_HW_ADDR(msg, IEEE802154_ATTR_HW_ADDR, dev->dev_addr);
> +
> +	/* FIXME: check that we really received hw address */
> +	NLA_PUT_HW_ADDR(msg, IEEE802154_ATTR_SRC_HW_ADDR, addr->hwaddr);

?


> +static int ieee802154_associate_req(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net_device *dev;
> +	struct ieee802154_addr addr;
> +	int ret = -EINVAL;
> +
> +	if (!info->attrs[IEEE802154_ATTR_CHANNEL]
> +	 || !info->attrs[IEEE802154_ATTR_COORD_PAN_ID]
> +	 || (!info->attrs[IEEE802154_ATTR_COORD_HW_ADDR] && !info->attrs[IEEE802154_ATTR_COORD_SHORT_ADDR])
> +	 || !info->attrs[IEEE802154_ATTR_CAPABILITY])
> +		return -EINVAL;

That's some odd coding style.

johannes

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