lkml.org 
[lkml]   [2009]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4/6] net: add NL802154 interface for configuration of 802.15.4 devices
From
Date
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2009-06-03 13:09    [W:0.658 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site